Don't use make for autogenerated modules in sagelib

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

[mkoeppe]

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?

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

[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()?

[embray]

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

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

[embray]

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.

[mkoeppe]

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

[mkoeppe]

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

[embray]

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

[git]

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

[mkoeppe]

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

[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?

[mkoeppe]

Probably easiest to just merge current develop, not rebase.

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

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

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

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

[embray]

How about this: As a special case, when sys.argv[0] == 'setup.py' we could set SAGE_SRC in sage.env to '.'?

[mkoeppe]

Why not just set the environment variable in setup.py? That's what was done (well, for a subprocess) before this ticket.

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

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

[mkoeppe]

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

[git]

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

[embray]

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

[git]

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

​74f1dad	Remove generate_py_source.mk from MANIFEST.in as it no longer exists
​618a235	Use sage.env instead of os.environ for path configuration

[git]

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

​0a5730a	Remove generate_py_source.mk from MANIFEST.in as it no longer exists
​fcb4c23	Use sage.env instead of os.environ for path configuration

[mkoeppe]

Looks good.

[vbraun]

Merge conflict

[mkoeppe]

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

[vbraun]

Just wait for the next beta

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

[jdemeyer]

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

[embray]

Or, since this ticket already had a positive review pending resolution of merge conflicts, you could make #21820 depend on this.

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

[git]

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

[embray]

Rebased--this was just based on a slightly older version of #21600 that was ultimately merged.

[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?

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

[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)?

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

[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=....

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

[mkoeppe]

needs rebasing...

[git]

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

[git]

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

[jdemeyer]

Somewhere you should add a Python equivalent of

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

[embray]

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

[git]

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

​e45216f

Clean up old autogenerated files before outputting new ones.

[embray]

New commits:

​e45216f

Clean up old autogenerated files before outputting new ones.

[jdemeyer]

In #21820, some paths changed. In particular, the generated files are now in src/sage/libs/cypari2.

[embray]

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

[git]

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.

[git]

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

​34ca46c

Don't look for the decl.pxi file anymore

[embray]

New commits:

​91fb12e	Use sage_src_pari to get the output path for generated files.
​34ca46c	Don't look for the decl.pxi file anymore

[mkoeppe]

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

[chapoton]

Some changes made here are not py3 compatible, and need to be corrected to be so.

try:

export SAGE_PYTHON3=yes 
make build

[chapoton]

proposal fix in #22044

[jdemeyer]

Breakage: #22094.