Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#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) Commit:
Dependencies: Stopgaps:

Description (last modified by embray)

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 3 years ago by mkoeppe

  • Cc mkoeppe added

comment:2 follow-up: Changed 3 years ago by mkoeppe

  • Cc jdemeyer added

From the description:

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).

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?

comment:3 in reply to: ↑ 2 Changed 3 years ago by jdemeyer

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 3 years ago by jdemeyer

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 3 years ago by embray

  • Description modified (diff)

I think it does match but I'm not sure what's unclear.

comment:6 Changed 3 years ago by embray

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 3 years ago by embray

  • Dependencies changed from #21480 to #21600

comment:8 Changed 3 years ago by embray

  • Owner changed from (none) to embray

comment:9 Changed 3 years ago by embray

  • Authors set to Erik Bray
  • 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:

7791cd9Remove --buildbase code
74169e7Pass SAGE_SRC to generate_py_source.mk
0394333Add new file to MANIFEST.in
fdedb02Install SAGE_LOCAL/bin/sage instead of SAGE_ROOT/sage as Jupyter kernel
5ba95edClean up stale installed files in install command
35ecf4bRun cythonize() inside build_ext command
2b5fa98Copy extra files to build directory instead of using data_files
7e3a492Rename extra_files -> sage_extra_files
237e965Replace some implicit relative imports with explicit imports.
edb3cf8Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.

comment:10 Changed 3 years ago by mkoeppe

This looks nice and easy to understand. But let's get #21600 done before we full review this one.

comment:11 Changed 3 years ago by mkoeppe

  • Status changed from needs_review to needs_work

On this ticket, 0394333 needs to be reverted, generate_py_source removed again from MANIFEST.in

(Of course, the manifest is out-of-date in many respects (#21516).)

comment:12 Changed 3 years ago by embray

Indeed. I just noticed that MANIFEST.in contains MANIFEST.in, which it really doesn't need to do :)

comment:13 Changed 3 years ago by git

  • Commit changed from edb3cf8eb8461768836c5aae71ecf5119d0c830f to 9f92cb37d3c6d407d7ffa9999aa14d0b25f67571

Branch pushed to git repo; I updated commit sha1. New commits:

9f92cb3Remove generate_py_source.mk from MANIFEST.in as it no longer exists

comment:14 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

comment:15 Changed 3 years ago by mkoeppe

  • 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 3 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe

comment:17 Changed 3 years ago by embray

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 3 years ago by mkoeppe

Probably easiest to just merge current develop, not rebase.

comment:19 Changed 3 years ago by embray

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: Changed 3 years ago by 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 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 3 years ago by mkoeppe

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 have SAGE_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 3 years ago by embray

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 3 years ago by embray

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 3 years ago by mkoeppe

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 3 years ago by embray

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 3 years ago by embray

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 3 years ago by mkoeppe

This is exactly what you need to debug; it worked before.

comment:28 Changed 3 years ago by git

  • Commit changed from 9f92cb37d3c6d407d7ffa9999aa14d0b25f67571 to fbe85beac217277437f61bb1bfbbbe07d839ef40

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0687297Clean up stale installed files in install command
8b44a9cRun cythonize() inside build_ext command
89dc422Copy extra files to build directory instead of using data_files
da8dda5Rename extra_files -> sage_extra_files
20b9ecfReplace some implicit relative imports with explicit imports.
af94fe9Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
fe541baRemove generate_py_source.mk from MANIFEST.in as it no longer exists
fbe85beUse sage.env instead of os.environ for path configuration

comment:29 Changed 3 years ago by embray

Fixed--there were some dangling uses of os.environ is sage_setup.autogen.pari.

comment:30 Changed 3 years ago by git

  • Commit changed from fbe85beac217277437f61bb1bfbbbe07d839ef40 to 618a2357dc5bf7eeba6417ce870a4ab09a9724a7

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

74f1dadRemove generate_py_source.mk from MANIFEST.in as it no longer exists
618a235Use sage.env instead of os.environ for path configuration

comment:31 Changed 3 years ago by git

  • Commit changed from 618a2357dc5bf7eeba6417ce870a4ab09a9724a7 to fcb4c2393394be1aa27769deb099cda2902d2158

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

0a5730aRemove generate_py_source.mk from MANIFEST.in as it no longer exists
fcb4c23Use sage.env instead of os.environ for path configuration

comment:32 Changed 3 years ago by mkoeppe

  • Status changed from needs_work to positive_review

Looks good.

comment:33 Changed 3 years ago by vbraun

  • Status changed from positive_review to needs_work

Merge conflict

comment:34 Changed 3 years ago by mkoeppe

This is not actionable unless you tell us with which ticket it conflicts.

comment:35 Changed 3 years ago by vbraun

Just wait for the next beta

comment:36 Changed 3 years ago by mkoeppe

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 3 years ago by jdemeyer

  • Dependencies changed from #21600 to #21820

I'm adding #21820 as dependency because this ticket would conflict with that.

comment:38 follow-up: Changed 3 years ago by embray

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 3 years ago by jdemeyer

Replying to embray:

Or, since this ticket already had a positive review

"had" is the important word here. Adding a dependency on a needs_work ticket with no activity for 2 weeks did not seem like a wise move to me.

But I don't want to upset you. So feel free to undo 37 if you fix this ticket.

comment:40 Changed 3 years ago by git

  • Commit changed from fcb4c2393394be1aa27769deb099cda2902d2158 to 4b797a71287b4c64dfb4b1eebbc031921c00e195

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

96e2836Replace some implicit relative imports with explicit imports.
273bde0Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
2f86ff5Remove generate_py_source.mk from MANIFEST.in as it no longer exists
4b797a7Use sage.env instead of os.environ for path configuration

comment:41 Changed 3 years ago by embray

  • 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 3 years ago by jdemeyer

In order to at least minimize the conflict with #21820 and #21821, can you revert the unneeded changes to src/sage_setup/autogen/pari/parser.py?

Last edited 3 years ago by jdemeyer (previous) (diff)

comment:43 Changed 3 years ago by jdemeyer

  • Status changed from needs_review to needs_work

comment:44 follow-up: Changed 3 years ago by embray

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: Changed 3 years ago by jdemeyer

Replying to embray:

I would be happy to, but as it stands that change is needed, per 15.

I am missing something. Why doesn't the same code fail right now (without applying this branch)?

comment:46 Changed 3 years ago by jdemeyer

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: Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

I would be happy to, but as it stands that change is needed, per 15.

I am missing something. Why doesn't the same code fail right now (without applying this branch)?

Previously, when make was invoked, it was passed SAGE_SRC=....

comment:48 in reply to: ↑ 47 Changed 3 years ago by jdemeyer

Replying to embray:

Previously, when make was invoked, it was passed SAGE_SRC=....

Right, I see. That's an ugly hack though.

I changed #21821 such that it doesn't need SAGE_SRC.

comment:49 Changed 3 years ago by mkoeppe

needs rebasing...

comment:50 Changed 3 years ago by git

  • Commit changed from 4b797a71287b4c64dfb4b1eebbc031921c00e195 to e9a40f82b2bb9af674599c3b7b5a786b5ae3dff9

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

f26b322Replace some implicit relative imports with explicit imports.
a83e251Do effectively exactly what the makefile for the autogen sources was doing, but purely in Python, removing the use of make.
8249bedRemove generate_py_source.mk from MANIFEST.in as it no longer exists
e9a40f8Use sage.env instead of os.environ for path configuration

comment:51 Changed 3 years ago by git

  • Commit changed from e9a40f82b2bb9af674599c3b7b5a786b5ae3dff9 to 9750ba7f6c0d745430211d0c060c5e015374182e

Branch pushed to git repo; I updated commit sha1. New commits:

9750ba7Don't use SAGE_SRC here a la https://trac.sagemath.org/ticket/21821

comment:52 Changed 3 years ago by jdemeyer

Somewhere you should add a Python equivalent of

rm -f sage/libs/pari/auto_*.pxi

comment:53 Changed 3 years ago by embray

I don't really think that belongs here in the first place but okay.

comment:54 Changed 3 years ago by git

  • Commit changed from 9750ba7f6c0d745430211d0c060c5e015374182e to e45216fcc5472bcaf5bd85318fd8e25acffde923

Branch pushed to git repo; I updated commit sha1. New commits:

e45216fClean up old autogenerated files before outputting new ones.

comment:55 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

e45216fClean up old autogenerated files before outputting new ones.

comment:56 Changed 3 years ago by jdemeyer

  • 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 3 years ago by embray

Yes, I see that now. Incidentally building still worked, it just built the files in the wrong place.

comment:58 Changed 3 years ago by git

  • Commit changed from e45216fcc5472bcaf5bd85318fd8e25acffde923 to 91fb12ed8d96bede142c7ee4b62540e4f99c827c

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

91fb12eUse sage_src_pari to get the output path for generated files.

comment:59 Changed 3 years ago by git

  • Commit changed from 91fb12ed8d96bede142c7ee4b62540e4f99c827c to 34ca46c4a44f25cdd076522a99ceaf215653d205

Branch pushed to git repo; I updated commit sha1. New commits:

34ca46cDon't look for the decl.pxi file anymore

comment:60 Changed 3 years ago by embray

  • Status changed from needs_work to needs_review

New commits:

91fb12eUse sage_src_pari to get the output path for generated files.
34ca46cDon't look for the decl.pxi file anymore
Last edited 3 years ago by embray (previous) (diff)

comment:61 Changed 3 years ago by mkoeppe

  • Status changed from needs_review to positive_review

Looks good to me; though the description of this ticket could use updating.

comment:62 Changed 3 years ago by vbraun

  • Branch changed from u/embray/ticket-21613 to 34ca46c4a44f25cdd076522a99ceaf215653d205
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:63 Changed 3 years ago by chapoton

  • 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 3 years ago by chapoton

proposal fix in #22044

comment:65 Changed 3 years ago by jdemeyer

Breakage: #22094.

Note: See TracTickets for help on using tickets.