Make sagelib setup.py self-contained and independent of SAGE_ROOT

mkoeppe commented 8 years ago

This ticket changes the build process of sagelib in the following way:

src/Makefile delegates ALL building to src/setup.py
src/setup.py no longer depends on environment variables $SAGE_ROOT, $SAGE_SRC, $SAGE_DOC_SRC etc. (to demonstrate this, Makefile poisons these environment variables). It still depends on $SAGE_LOCAL and environment variables that point below it.

This ticket is meant as:

preparation for VPATH builds of sage-the-distribution (#21469)
working towards the goal of making sagelib pip-installable -- see #21507 for the eventual goal of having sagelib on PyPI
making the flow of directory information at build time clearer for developers

More specifically, the goal of this ticket is that only SAGE_LOCAL needs to be set when the user does 'pip install' of the sagelib. (This ticket almost achieves this, except it also needs SAGE_PKGS and SAGE_CYTHONIZED to be set. The hope is that #20382 and other future tickets will develop better mechanisms to communicate package and directory information to the build.)

. . . . . . .

Some possibly useful information:

Documentation on distutils (https://docs.python.org/2/install/), describing use of --build-base to do VPATH builds.
pip install keeps the source directory clean, building instead in a temporary directory, by copying the sources. pip install also offers options --build to select a build directory, but there are some pip issues: 2060, 2053, 804 that affect this
14807 has some tricks to making VPATH builds work without copying all python source files. But it uses automake instead of setup.sh; we will not do this in our ticket.

configure tarball: http://sage.ugent.be/www/jdemeyer/sage/configure-185.tar.gz

CC: @sagetrac-felixs @jdemeyer @kiwifb @embray @nexttime @vbraun @dimpase @jhpalmieri @videlec @saraedum @seblabbe @nthiery @mezzarobba

Component: build

Author: Matthias Koeppe

Branch/Commit: 0c2ac95

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/21480

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

`853a1aa`	`Make src/Makefile a generated file`
`1931b43`	`Define SAGE_PKGS in sage.env, not sage.misc.package`
`c1dc4f8`	`Set SAGE_CYTHONIZED earlier`
`d829eb4`	`Build in $(abs_builddir)/build-sagelib. Poison directory env variables`

mkoeppe commented 8 years ago

comment:44

I've changed the description of this ticket, as it has changed its focus. Ready for review.

mkoeppe commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,10 +1,14 @@
-Currently, building sagelib creates the `src/build` directory, with subdirectories `cython_debug`, `cythonized`, `lib.UNAME`, `temp.UNAME`. 
+This ticket changes the build process of sagelib in the following way:
+- `src/Makefile` delegates ALL building to `src/setup.sh`
+- `src/setup.sh` no longer depends on environment variables `$SAGE_ROOT`, `$SAGE_SRC`, `$SAGE_DOC_SRC`, `$SAGE_CYTHONIZED` etc. (to demonstrate this, `Makefile` poisons these environment variables). It still depends on `$SAGE_LOCAL` and environment variables that point below it.
+- `src/setup.sh` accepts `build --build-base=BUILD-BASE`. This is where building takes place (where subdirectories `cython_debug`, `cythonized`, `lib.UNAME`, `temp.UNAME` are created). The default is the `build` subdirectory (of `src`).
+- `src/Makefile` sets `--build-base` to the `build-sagelib` subdirectory (of `src`).

-In preparation for VPATH builds of sage-the-distribution (#21469), let's keep `src/` clean by using `setup.py --build-base=$SAGE_BUILD_DIR/sagelib`
+This ticket is meant as:
+- preparation for VPATH builds of sage-the-distribution (#21469)
+- working towards the goal of making `sagelib` pip-installable 

-(`$SAGE_BUILD_DIR` defaults to `$SAGE_ROOT/var/tmp/sage/build/`)
-
-As a second goal of this ticket, `setup.sh` is put in charge of ALL sagelib building. 
+. . . . . . . 

 Some possibly useful information:
 - Documentation on distutils (https://docs.python.org/2/install/), describing use of `--build-base` to do VPATH builds.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

`57db699`	`Revert "Make SAGE_BUILD_DIR variable available in sage-env, not just in sage-spkg"`
`ef0919c`	`Fixup for SAGE_CYTHONIZED`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from d829eb4 to ef0919c

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from ef0919c to bd670af

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

`8ccd440`	`Add src/generate_py_source.mk`
`bd670af`	`Ignore generated files`

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from bd670af to 751bd0f

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

751bd0f Reword TODO item

jdemeyer commented 8 years ago

Description changed:

--- 
+++ 
@@ -1,7 +1,7 @@
 This ticket changes the build process of sagelib in the following way:
-- `src/Makefile` delegates ALL building to `src/setup.sh`
-- `src/setup.sh` no longer depends on environment variables `$SAGE_ROOT`, `$SAGE_SRC`, `$SAGE_DOC_SRC`, `$SAGE_CYTHONIZED` etc. (to demonstrate this, `Makefile` poisons these environment variables). It still depends on `$SAGE_LOCAL` and environment variables that point below it.
-- `src/setup.sh` accepts `build --build-base=BUILD-BASE`. This is where building takes place (where subdirectories `cython_debug`, `cythonized`, `lib.UNAME`, `temp.UNAME` are created). The default is the `build` subdirectory (of `src`).
+- `src/Makefile` delegates ALL building to `src/setup.py`
+- `src/setup.py` no longer depends on environment variables `$SAGE_ROOT`, `$SAGE_SRC`, `$SAGE_DOC_SRC`, `$SAGE_CYTHONIZED` etc. (to demonstrate this, `Makefile` poisons these environment variables). It still depends on `$SAGE_LOCAL` and environment variables that point below it.
+- `src/setup.py` accepts `build --build-base=BUILD-BASE`. This is where building takes place (where subdirectories `cython_debug`, `cythonized`, `lib.UNAME`, `temp.UNAME` are created). The default is the `build` subdirectory (of `src`).
 - `src/Makefile` sets `--build-base` to the `build-sagelib` subdirectory (of `src`).

 This ticket is meant as:

jdemeyer commented 8 years ago

comment:51

I would prefer to do this ticket after #21479 because there might be non-trivial interactions.

jdemeyer commented 8 years ago

comment:52

Also in the code setup.sh -> setup.py

jdemeyer commented 8 years ago

comment:53

Regarding

(cd $(srcdir) && export SAGE_ROOT=/doesnotexist SAGE_SRC=/doesnotexist SAGE_SRC_ROOT=/doesnotexist SAGE_DOC_SRC=/doesnotexist SAGE_SCRIPTS_DIR=/doesnotexist SAGE_BUILD_DIR=/doesnotexist SAGE_CYTHONIZED=/doesnotexist SAGE_PKGS=$(abs_top_srcdir)/build/pkgs && python -u setup.py build --build-base=$(abs_builddir)/build-sagelib install)

Did you know that make supports backslash-continued lines? :-)

jdemeyer commented 8 years ago

comment:54

For backwards compatibility, can we please keep the name build instead of changing it to build-sagelib?

jdemeyer commented 8 years ago

comment:55

What is the rationale for splitting the Makefile in two (Makefile and generate_py_source.mk)? That seems like additional complication without reason.

In any case, using make is wrong. You need to use os.environ["MAKE"].

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 751bd0f to 3a8cc0e

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

3a8cc0e Fix typo in comment

mkoeppe commented 8 years ago

comment:57

Replying to @jdemeyer:

For backwards compatibility, can we please keep the name build instead of changing it to build-sagelib?

If I keep the default, it's hard to demonstrate that "--build-base" actually works.

Moreover, there are too many things already called "build" -- better be specific.

mkoeppe commented 8 years ago

comment:58

Replying to @jdemeyer:

What is the rationale for splitting the Makefile in two (Makefile and generate_py_source.mk)? That seems like additional complication without reason.

setup.sh needs to be in charge of building everything, if sagelib is to become a well-behaved Python package.

mkoeppe commented 8 years ago

comment:59

generate_py_source.mk could certainly be implemented in pure Python and could become a part of setup.py. But I don't want to work on this.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 3a8cc0e to 0dd9c50

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

0dd9c50 Respect environment variable MAKE

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

17f90d8 beautification

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 0dd9c50 to 17f90d8

jdemeyer commented 8 years ago

comment:62

Replying to @mkoeppe:

Replying to @jdemeyer:

For backwards compatibility, can we please keep the name build instead of changing it to build-sagelib?

If I keep the default, it's hard to demonstrate that "--build-base" actually works.

Well, it's easy for the reviewer to test a different value of --build-base.

Moreover, there are too many things already called "build" -- better be specific.

But src/build is pretty clear: it is the build directory corresponding to src, which is the Sage library. I don't like to change it just for the sake of changing it.

jdemeyer commented 8 years ago

comment:63

Replying to @mkoeppe:

generate_py_source.mk could certainly be implemented in pure Python and could become a part of setup.py

That doesn't answer my question. Why do you not keep this in src/Makefile?

jdemeyer commented 8 years ago

comment:64

Replying to @mkoeppe:

setup.sh needs to be in charge of building everything, if sagelib is to become a well-behaved Python package.

Sorry, I didn't see this comment. Okay, that makes sense but it wasn't clear to me,

I guess you should add some comments to src/Makefile to explain this better than just

## All sagelib-building is done by setup.py.
## DON'T ADD ADDITIONAL STEPS TO THIS MAKEFILE.

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

Changed commit from 17f90d8 to e5f9065

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 8 years ago

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

e5f9065 More comments

jdemeyer commented 8 years ago

comment:66

Replying to @jdemeyer:

I would prefer to do this ticket after #21479 because there might be non-trivial interactions.

Or at least I will test it with #21501 applied.

mkoeppe commented 8 years ago

comment:67

OK, I'll have #21479 ready in a few days.

jdemeyer commented 8 years ago

comment:68

I just tested this with #21501. It mostly works but there are these two doctest failures:

sage -t src/sage_setup/find.py
**********************************************************************
File "src/sage_setup/find.py", line 107, in sage_setup.find.find_extra_files
Failed example:
    find_extra_files(["sage.modular.arithgroup"], SAGE_SRC, SAGE_CYTHONIZED, ".")
Expected:
    [('./sage/modular/arithgroup',
      ['.../src/sage/modular/arithgroup/farey.pxd', ...farey_symbol.h...])]
Got:
    [('./sage/modular/arithgroup',
      ['/usr/local/src/sage-config/src/sage/modular/arithgroup/farey.pxd'])]
**********************************************************************
File "src/sage_setup/clean.py", line 87, in sage_setup.clean._find_stale_files
Failed example:
    for f in stale_iter:
        if f.endswith(skip_extensions): continue
        print('Found stale file: ' + f)
Expected nothing
Got:
    Found stale file: sage/stats/distributions/dgs_gauss.h
    Found stale file: sage/libs/lcalc/lcalc_sage.h
    Found stale file: sage/geometry/triangulation/data.h
    Found stale file: sage/misc/cython_metaclass.h
    Found stale file: sage/rings/finite_rings/stdint.h
    Found stale file: sage/stats/distributions/dgs_misc.h
    Found stale file: sage/misc/darwin_memory_usage.h
    Found stale file: sage/stats/distributions/dgs.h
    Found stale file: sage/ext/python_debug.h
    Found stale file: sage/libs/eclsig.h
    Found stale file: sage/ext/solaris_fixes.h
    Found stale file: sage/ext/pyx_visit.h
    Found stale file: sage/matroids/minorfix.h
    Found stale file: sage/modular/arithgroup/farey_symbol.h
    Found stale file: sage/ext/mod_int.h
    Found stale file: sage/combinat/matrices/dancing_links_c.h
    Found stale file: sage/ext/interpreters/wrapper_rr.h
    Found stale file: sage/symbolic/ginac_wrap.h
    Found stale file: sage/ext/interpreters/wrapper_cdf.h
    Found stale file: sage/ext/interpreters/wrapper_el.h
    Found stale file: sage/groups/perm_gps/partn_ref2/refinement_generic.h
    Found stale file: sage/libs/polybori/pb_wrap.h
    Found stale file: sage/sat/solvers/cryptominisat/solverconf_helper.h
    Found stale file: sage/combinat/partitions_c.h
    Found stale file: sage/ext/ccobject.h
    Found stale file: sage/geometry/triangulation/triangulations.h
    Found stale file: sage/libs/ntl/ntlwrap.h
    Found stale file: sage/stats/distributions/dgs_bern.h
    Found stale file: sage/sat/solvers/cryptominisat/cryptominisat_helper.h
    Found stale file: sage/libs/pari/parisage.h
    Found stale file: sage/geometry/triangulation/functions.h
**********************************************************************

jdemeyer commented 8 years ago

comment:69

One thing I don't like about the approach of this ticket is that SAGE_CYTHONIZED is set by the value of --build-base. I would much rather have it the other way around: that some environment variable is set by configure which is then used to define the value for --build-base and for the environment variable SAGE_CYTHONIZED.

This would be more analogous to #21501 where I want to set $SAGE_LOCAL just once and then derive everything from that.

mkoeppe commented 8 years ago

Description changed:

--- 
+++ 
@@ -6,7 +6,9 @@

 This ticket is meant as:
 - preparation for VPATH builds of sage-the-distribution (#21469)
-- working towards the goal of making `sagelib` pip-installable 
+- working towards the goal of making `sagelib` pip-installable.
+
+ More specifically, the goal of this ticket is that only SAGE_LOCAL needs to be set when the user does 'pip install' of the sagelib. (This ticket almost achieves this, except it also needs SAGE_PKGS to be set. The hope is that #20382 or a future ticket will develop a better mechanism to communicate package information to the build.)

 . . . . . . .

mkoeppe commented 8 years ago

comment:71

Replying to @jdemeyer:

One thing I don't like about the approach of this ticket is that SAGE_CYTHONIZED is set by the value of --build-base. I would much rather have it the other way around: that some environment variable is set by configure which is then used to define the value for --build-base and for the environment variable SAGE_CYTHONIZED.

This would be more analogous to #21501 where I want to set $SAGE_LOCAL just once and then derive everything from that.

I've updated the description to explain better why --build-base needs to be in charge of setting all build directories.

mkoeppe commented 8 years ago

comment:72

But yes, I'll introduce something like SAGE_SRC_BUILDBASE that can be used in src/Makefile.in and to define SAGE_CYTHONIZED for that doctest.

jdemeyer commented 8 years ago

comment:73

Replying to @mkoeppe:

I've updated the description to explain better why --build-base needs to be in charge of setting all build directories.

Sorry, but the paragraph you added does not explain this. It explains better the what but not the why. This reminds me of what I recently said at [#21469 comment:17]

jdemeyer commented 8 years ago

comment:74

Replying to @mkoeppe:

But yes, I'll introduce something like SAGE_SRC_BUILDBASE that can be used in src/Makefile.in and to define SAGE_CYTHONIZED for that doctest.

Now I am even more lost...

First you say that --build-base should determine everything but this comment seems to contradict that.

mkoeppe commented 8 years ago

Description changed:

--- 
+++ 
@@ -6,7 +6,7 @@

 This ticket is meant as:
 - preparation for VPATH builds of sage-the-distribution (#21469)
-- working towards the goal of making `sagelib` pip-installable.
+- working towards the goal of making `sagelib` pip-installable -- see #21507 for the eventual goal of having sagelib on PyPI

  More specifically, the goal of this ticket is that only SAGE_LOCAL needs to be set when the user does 'pip install' of the sagelib. (This ticket almost achieves this, except it also needs SAGE_PKGS to be set. The hope is that #20382 or a future ticket will develop a better mechanism to communicate package information to the build.)

mkoeppe commented 8 years ago

comment:76

Replying to @jdemeyer:

Replying to @mkoeppe:

I've updated the description to explain better why --build-base needs to be in charge of setting all build directories.

Sorry, but the paragraph you added does not explain this. It explains better the what but not the why.

I've created ticket #21507 to put the technical goals of the present ticket in the context of a bigger goal. Please take a look.

jdemeyer commented 8 years ago

comment:77

For this ticket, can you please keep SAGE_CYTHONIZED independent of --build-base (and set them both from the common environment variable you mention in [comment:72]). That will also solve the doctest failures from [comment:68].

The current code which figures out SAGE_CYTHONIZED from the command line is quite ugly and fragile. I think that we can do a bigger reorganization of setup.py which will make things simpler. Also Cython 0.25 (not released yet) has some potential to simplify cythonization.

PS: I like the fact that you are splitting this up over many tickets, it gets things moving more quickly.

jdemeyer commented 8 years ago

comment:78

Replying to @mkoeppe:

OK, I'll have #21479 ready in a few days.

Actually, maybe this doesn't really need to depend on #21479, since it doesn't involve ./configure. If you address the points I made, then this might be good to go.

mkoeppe commented 8 years ago

comment:79

Replying to @jdemeyer:

For this ticket, can you please keep SAGE_CYTHONIZED independent of --build-base (and set them both from the common environment variable you mention in [comment:72]). That will also solve the doctest failures from [comment:68].

I disagree. I consider setup.py deriving SAGE_CYTHONIZED from --build-base a crucial feature of this patch. What I meant above is simply that setup.py will set an intermediate variable, rather than SAGE_CYTHONIZED, and SAGE_CYTHONIZED would be determined by env.py.

The current code which figures out SAGE_CYTHONIZED from the command line is quite ugly and fragile. I think that we can do a bigger reorganization of setup.py which will make things simpler. Also Cython 0.25 (not released yet) has some potential to simplify cythonization.

I am aware that the arg parsing code is ad hoc, but it's easy to understand code.

I do not want to defer getting --build-base working to some distant future, when someone finds the time to clean up setup.py.

jdemeyer commented 8 years ago

comment:80

Replying to @mkoeppe:

I do not want to defer getting --build-base working to some distant future, when someone finds the time to clean up setup.py.

At least you shouldn't determine the directory from the command line. That's too fragile to work correctly. You should find a way to get the directory from distutils which is going to require some refactoring anyway.

mkoeppe commented 8 years ago

comment:81

Replying to @jdemeyer:

At least you shouldn't determine the directory from the command line. That's too fragile to work correctly. You should find a way to get the directory from distutils which is going to require some refactoring anyway.

The current structure of setup.py does not make this easy. SAGE_CYTHONIZED is used before setup is even invoked. It can't be the job of this ticket to bring setup.py to standard distutils behavior.

mkoeppe commented 8 years ago

comment:82

I've created a ticket #21508 for setup.py cleanup issues.

jdemeyer commented 8 years ago

comment:83

Replying to @mkoeppe:

The current structure of setup.py does not make this easy.

I know! That exactly why I proposed in [comment:77] to postpone this piece of your patch.

If you really want to go for #21507, you will need a lot of changes to setup.py anyway. So the cleanup will need to happen sooner or later.

mkoeppe commented 8 years ago

comment:84

Replying to @jdemeyer:

Replying to @mkoeppe:

The current structure of setup.py does not make this easy.

I know! That exactly why I proposed in [comment:77] to postpone this piece of your patch.

I want the feature of --build-base now, not in the distant future. It is easily achieved with this patch. The arg parsing is ad hoc; but given the current state of setup.py, which does not take care at all about standard distutils behavior, it is an improvement.

If you really want to go for #21507, you will need a lot of changes to setup.py anyway. So the cleanup will need to happen sooner or later.

I prefer to do one concrete step (technical goal) at a time. Let's get --build-base working right now.

jdemeyer commented 8 years ago

comment:85

Replying to @mkoeppe:

Let's get --build-base working right now.

In my opinion, this branch is not making --build-base work. This branch is adding hacks to pretend like --build-base is working.

In general, hacks can have their place, but only if there is a clear reason for them. In this case, I see no reason. This ticket has no reason to exist except for going towards #21507. And this hack that you added will get us no closer to #21507 since we will need to refactor the SAGE_CYTHONIZED code anyway.

jdemeyer commented 8 years ago

comment:86

Anyway, I will leave this issue for other people to comment, since we probably won't come to an agreement.

sagemath / sage

Make sagelib setup.py self-contained and independent of SAGE_ROOT #21480

14807 has some tricks to making VPATH builds work without copying all python source files. But it uses automake instead of setup.sh; we will not do this in our ticket.