#21613 closed task (fixed)
Don't use make for autogenerated modules in sagelib
Reported by: | embray | Owned by: | embray |
---|---|---|---|
Priority: | major | Milestone: | sage-7.5 |
Component: | build | Keywords: | |
Cc: | mkoeppe, jdemeyer | Merged in: | |
Authors: | Erik Bray | Reviewers: | Matthias Koeppe |
Report Upstream: | N/A | Work issues: | |
Branch: | 34ca46c (Commits, GitHub, GitLab) | Commit: | |
Dependencies: | Stopgaps: |
Description (last modified by )
Currently there is a src/Makefile
at the root of the sagelib sources that is required to be invoked in order to make sure the packages in sage_setup.autogen
and that autogenerated sources are placed in the sage sources before running its setup.py install
.
#21480 inverts things somewhat by removing the old Makefile (really making it into a no-op for now), but pulling out the sage_setup.autogen
stuff into a separate makefile that is still invoked by calling make
from the setup.py
(in the current version of that ticket it is even called unconditionally). This is pretty non-standard.
The appropriate time to do this would be either just before the build_py
command is run, or the build_ext
command is run (in the case of autogenerated sources for extension modules).
In the current cases we have its very trivial to check whether or not any of the sources actually need to be regenerated, and this is a pretty normal thing to do in some other complex Python packages. For example, the package responsible for generating the source files (e.g. currently the sage_setup.autogen
sub-packages) simply needs to be responsible for knowing what its own source code files are, and knowing the paths for its generated sources--then just compare modtimes. Invoking make is not required at all.
One can do even better by having one of the generated files be a hash of the source files, and compare the hashes instead (this avoids rebuilds when switching between git branches but where the files didn't otherwise change).
Change History (65)
comment:1 Changed 6 years ago by
- Cc mkoeppe added
comment:2 follow-up: ↓ 3 Changed 6 years ago by
- Cc jdemeyer added
comment:3 in reply to: ↑ 2 Changed 6 years ago by
Replying to mkoeppe:
Could you make a ticket on top of #21480 (and probably #21604) that moves the invocation of
make -f generate_py_source.mk
to the right place?
That's not so easy, for the same reason that #21600 is not so easy. We need to pass the list of Python packages, Cython extensions and data_files to setup()
. It is harder to get this right if not everything has been generated at that point. We should probably compute the list of packages dynamically in setup()
.
comment:4 Changed 6 years ago by
Again, the title of this ticket does not match the long description. Do you want to get rid of make
or just move the invocation of make -f generate_py_source.mk
inside setup()
?
comment:5 Changed 6 years ago by
- Description modified (diff)
I think it does match but I'm not sure what's unclear.
comment:6 Changed 6 years ago by
I feel like this is not hard, having done it many times before. There's nothing particularly special about sage either in any of these regards.
comment:7 Changed 6 years ago by
- Dependencies changed from #21480 to #21600
comment:8 Changed 6 years ago by
- Owner changed from (none) to embray
comment:9 Changed 6 years ago by
- Branch set to u/embray/ticket-21613
- Commit set to edb3cf8eb8461768836c5aae71ecf5119d0c830f
- Status changed from new to needs_review
Determine directly in the Python code whether or not autogenerated sources need to be built, without requiring a makefile or anything. I think this is simple and clear to anyone who looks at the code.
If in the future we want more full make-like capabilities I've got something in the works for that, but it seems unlikely that anything more complicated than this is needed any time soon.
Last 10 new commits:
7791cd9 | Remove --buildbase code
|
74169e7 | Pass SAGE_SRC to generate_py_source.mk
|
0394333 | Add new file to MANIFEST.in
|
fdedb02 | Install SAGE_LOCAL/bin/sage instead of SAGE_ROOT/sage as Jupyter kernel
|
5ba95ed | Clean up stale installed files in install command
|
35ecf4b | Run cythonize() inside build_ext command
|
2b5fa98 | Copy extra files to build directory instead of using data_files
|
7e3a492 | Rename extra_files -> sage_extra_files
|
237e965 | Replace some implicit relative imports with explicit imports.
|
edb3cf8 | Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
|
comment:10 Changed 6 years ago by
This looks nice and easy to understand. But let's get #21600 done before we full review this one.
comment:11 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:12 Changed 6 years ago by
Indeed. I just noticed that MANIFEST.in contains MANIFEST.in, which it really doesn't need to do :)
comment:13 Changed 6 years ago by
- Commit changed from edb3cf8eb8461768836c5aae71ecf5119d0c830f to 9f92cb37d3c6d407d7ffa9999aa14d0b25f67571
Branch pushed to git repo; I updated commit sha1. New commits:
9f92cb3 | Remove generate_py_source.mk from MANIFEST.in as it no longer exists
|
comment:14 Changed 6 years ago by
- Status changed from needs_work to needs_review
comment:15 Changed 6 years ago by
- Milestone changed from sage-7.4 to sage-7.5
- Status changed from needs_review to needs_work
This needs rebasing on top of current develop and possibly some work there. Building from scratch (with 7.4.rc2 merged) fails for me with:
No such file or directory: '/doesnotexist/sage/libs/pari/paridecl.pxd'
(this is from #21480).
comment:16 Changed 6 years ago by
- Reviewers set to Matthias Koeppe
comment:17 Changed 6 years ago by
Ok, but I'm a little confused--this branch is based on the branch in #21600. Was that rebased, or in need of rebasing too?
comment:18 Changed 6 years ago by
Probably easiest to just merge current develop, not rebase.
comment:19 Changed 6 years ago by
Oh I see, this is because sage_setup.autogen
uses SAGE_SRC
. I guess it would be fine to just remove that, under the assumption that sage's setup.py
is always run from the root of the sagelib source directory.
comment:20 follow-up: ↓ 21 Changed 6 years ago by
Alternatively, would you be opposed to changing the Makefile so that SAGE_SRC
is just set to .
? There are places in the code where it makes sense to have SAGE_SRC
set outside the context of building (e.g. testing), but where when building it should just be .
.
comment:21 in reply to: ↑ 20 Changed 6 years ago by
Replying to embray:
Alternatively, would you be opposed to changing the Makefile so that
SAGE_SRC
is just set to.
? There are places in the code where it makes sense to haveSAGE_SRC
set outside the context of building (e.g. testing), but where when building it should just be.
.
Please don't do that. setup.py needs to be able to run without SAGE_SRC set. That's the whole point of #21480.
comment:22 Changed 6 years ago by
That's sort of what I thought. The problem is that there is code in this package that needs to be able to run in both a "build" environment and a "runtime" environment. I agree that in the "build" environment it should not depend on SAGE_SRC
.
However, prior to this ticket the setup.py
was depending on SAGE_SRC
anyways, implicitly, through running Python from the makefile.
I could modify that code to only use SAGE_SRC
if it is set, but that's not going to work because it is set to "/doesnotexist". I could check explicitly for that path but that's rather ugly too.
comment:23 Changed 6 years ago by
How about this: As a special case, when sys.argv[0] == 'setup.py'
we could set SAGE_SRC
in sage.env
to '.'
?
comment:24 Changed 6 years ago by
Why not just set the environment variable in setup.py? That's what was done (well, for a subprocess) before this ticket.
comment:25 Changed 6 years ago by
Ah, so that's ok so long as it's not coming from the outer environment in which setup.py
is run. That makes sense--thanks for the suggestion.
comment:26 Changed 6 years ago by
Ok, I'm confused again because setup.py
is setting SAGE_SRC
to os.getcwd()
, quite early on too. So I'm not sure why that's being dropped. The autogen code isn't being run in a subprocess, at least I don't think.
comment:27 Changed 6 years ago by
This is exactly what you need to debug; it worked before.
comment:28 Changed 6 years ago by
- Commit changed from 9f92cb37d3c6d407d7ffa9999aa14d0b25f67571 to fbe85beac217277437f61bb1bfbbbe07d839ef40
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
0687297 | Clean up stale installed files in install command
|
8b44a9c | Run cythonize() inside build_ext command
|
89dc422 | Copy extra files to build directory instead of using data_files
|
da8dda5 | Rename extra_files -> sage_extra_files
|
20b9ecf | Replace some implicit relative imports with explicit imports.
|
af94fe9 | Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
|
fe541ba | Remove generate_py_source.mk from MANIFEST.in as it no longer exists
|
fbe85be | Use sage.env instead of os.environ for path configuration
|
comment:29 Changed 6 years ago by
Fixed--there were some dangling uses of os.environ
is sage_setup.autogen.pari
.
comment:30 Changed 6 years ago by
- Commit changed from fbe85beac217277437f61bb1bfbbbe07d839ef40 to 618a2357dc5bf7eeba6417ce870a4ab09a9724a7
comment:31 Changed 6 years ago by
- Commit changed from 618a2357dc5bf7eeba6417ce870a4ab09a9724a7 to fcb4c2393394be1aa27769deb099cda2902d2158
comment:34 Changed 6 years ago by
This is not actionable unless you tell us with which ticket it conflicts.
comment:35 Changed 6 years ago by
Just wait for the next beta
comment:36 Changed 6 years ago by
Erik: The branch on this ticket also has some of Jeroen's changes in it that are not yet in 7.5.beta0. From which ticket are they? It should be added to dependencies.
comment:37 Changed 6 years ago by
- Dependencies changed from #21600 to #21820
I'm adding #21820 as dependency because this ticket would conflict with that.
comment:38 follow-up: ↓ 39 Changed 6 years ago by
Or, since this ticket already had a positive review pending resolution of merge conflicts, you could make #21820 depend on this.
comment:39 in reply to: ↑ 38 Changed 6 years ago by
comment:40 Changed 6 years ago by
- Commit changed from fcb4c2393394be1aa27769deb099cda2902d2158 to 4b797a71287b4c64dfb4b1eebbc031921c00e195
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
96e2836 | Replace some implicit relative imports with explicit imports.
|
273bde0 | Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
|
2f86ff5 | Remove generate_py_source.mk from MANIFEST.in as it no longer exists
|
4b797a7 | Use sage.env instead of os.environ for path configuration
|
comment:41 Changed 6 years ago by
- Status changed from needs_work to needs_review
Rebased--this was just based on a slightly older version of #21600 that was ultimately merged.
comment:42 Changed 6 years ago by
comment:43 Changed 6 years ago by
- Status changed from needs_review to needs_work
comment:44 follow-up: ↓ 45 Changed 6 years ago by
I would be happy to, but as it stands that change is needed, per 15. Now that I'm back from vacation and we can sync up better we can keep the dependencies if you want. I just figured it would be easier for you to understand this change and build on top of it, rather than for me to do the same with #21820 and #21821.
comment:45 in reply to: ↑ 44 ; follow-up: ↓ 47 Changed 6 years ago by
comment:46 Changed 6 years ago by
Besides the fact that the code using sage.env
conflicts with #21821, I think it is also bad style for setup.py
to import sage.env
while building sage
.
comment:47 in reply to: ↑ 45 ; follow-up: ↓ 48 Changed 6 years ago by
comment:48 in reply to: ↑ 47 Changed 6 years ago by
comment:49 Changed 6 years ago by
needs rebasing...
comment:50 Changed 6 years ago by
- Commit changed from 4b797a71287b4c64dfb4b1eebbc031921c00e195 to e9a40f82b2bb9af674599c3b7b5a786b5ae3dff9
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
f26b322 | Replace some implicit relative imports with explicit imports.
|
a83e251 | Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
|
8249bed | Remove generate_py_source.mk from MANIFEST.in as it no longer exists
|
e9a40f8 | Use sage.env instead of os.environ for path configuration
|
comment:51 Changed 6 years ago by
- Commit changed from e9a40f82b2bb9af674599c3b7b5a786b5ae3dff9 to 9750ba7f6c0d745430211d0c060c5e015374182e
Branch pushed to git repo; I updated commit sha1. New commits:
9750ba7 | Don't use SAGE_SRC here a la https://trac.sagemath.org/ticket/21821
|
comment:52 Changed 6 years ago by
Somewhere you should add a Python equivalent of
rm -f sage/libs/pari/auto_*.pxi
comment:53 Changed 5 years ago by
I don't really think that belongs here in the first place but okay.
comment:54 Changed 5 years ago by
- Commit changed from 9750ba7f6c0d745430211d0c060c5e015374182e to e45216fcc5472bcaf5bd85318fd8e25acffde923
Branch pushed to git repo; I updated commit sha1. New commits:
e45216f | Clean up old autogenerated files before outputting new ones.
|
comment:55 Changed 5 years ago by
- Status changed from needs_work to needs_review
New commits:
e45216f | Clean up old autogenerated files before outputting new ones.
|
comment:56 Changed 5 years ago by
- Dependencies #21820 deleted
- Status changed from needs_review to needs_work
In #21820, some paths changed. In particular, the generated files are now in src/sage/libs/cypari2
.
comment:57 Changed 5 years ago by
Yes, I see that now. Incidentally building still worked, it just built the files in the wrong place.
comment:58 Changed 5 years ago by
- Commit changed from e45216fcc5472bcaf5bd85318fd8e25acffde923 to 91fb12ed8d96bede142c7ee4b62540e4f99c827c
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
91fb12e | Use sage_src_pari to get the output path for generated files.
|
comment:59 Changed 5 years ago by
- Commit changed from 91fb12ed8d96bede142c7ee4b62540e4f99c827c to 34ca46c4a44f25cdd076522a99ceaf215653d205
Branch pushed to git repo; I updated commit sha1. New commits:
34ca46c | Don't look for the decl.pxi file anymore
|
comment:60 Changed 5 years ago by
- Status changed from needs_work to needs_review
comment:61 Changed 5 years ago by
- Status changed from needs_review to positive_review
Looks good to me; though the description of this ticket could use updating.
comment:62 Changed 5 years ago by
- Branch changed from u/embray/ticket-21613 to 34ca46c4a44f25cdd076522a99ceaf215653d205
- Resolution set to fixed
- Status changed from positive_review to closed
comment:63 Changed 5 years ago by
- Commit 34ca46c4a44f25cdd076522a99ceaf215653d205 deleted
Some changes made here are not py3 compatible, and need to be corrected to be so.
try:
export SAGE_PYTHON3=yes make build
comment:64 Changed 5 years ago by
proposal fix in #22044
comment:65 Changed 5 years ago by
Breakage: #22094.
From the description:
Could you make a ticket on top of #21480 (and probably #21604) that moves the invocation of
make -f generate_py_source.mk
to the right place?