python2/3: split spkg-install to spkg-build; fix various build issues

[embray]

This ticket starts by splitting the spkg-install for python2 and python3 into separate spkg-build and spkg-install scripts as recommended by #21726 (we said we would apply this change to individual packages on a case-by-case basis, and I figured as long as I was making changes to these scripts it would be good to do).

This also fixes a general bug with the build, where building a new Python can link the python executable and extension modules with an old libpython left in $SAGE_LOCAL, leading to possible errors.

Rather than deleting the old libpython first, I think it's cleaner to pass the correct $LDFLAGS to prevent this from happening.

Finally, this moves the tests that certain extension modules built to after building, but before installing, and runs the tests out of the source directory. This is a change extracted from #22509.

Component: packages: standard

Keywords: python2 python3

Author: Erik Bray

Branch/Commit: f326c1a

Reviewer: Jeroen Demeyer

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

[jdemeyer]

comment:2

Given that these scripts are mostly the same for Python 2 and Python 3, would it be possible to have just one script (say, by symlinking) and then use $PKG_NAME inside the script where needed?

[jdemeyer]

comment:3

Looking at the difference between the Python 2 and Python 3 scripts, I wonder why this only occurs for Python 2:

  # Use EXTRA_CFLAGS for user-defined CFLAGS since Python puts its own
  # default flags like -O3 after CFLAGS but before EXTRA_CFLAGS.
  # We also disable warnings about unused variables/functions which are
  # common in Cython-generated code.
  export EXTRA_CFLAGS="`testcflags.sh -Wno-unused` $CFLAGS"
  unset CFLAGS

I think this is just an omission and it should be there for Python 3 also.

[jdemeyer]

comment:4

Apart from that, everything looks fine from looking at the diff.

[embray]

comment:5

Replying to @jdemeyer:

Given that these scripts are mostly the same for Python 2 and Python 3, would it be possible to have just one script (say, by symlinking) and then use $PKG_NAME inside the script where needed?

I've wondered that myself. It's tempting--maybe this ticket would be a good place to do it as long as I'm making changes...

[embray]

comment:6

Replying to @jdemeyer:

Looking at the difference between the Python 2 and Python 3 scripts, I wonder why this only occurs for Python 2:

  # Use EXTRA_CFLAGS for user-defined CFLAGS since Python puts its own
  # default flags like -O3 after CFLAGS but before EXTRA_CFLAGS.
  # We also disable warnings about unused variables/functions which are
  # common in Cython-generated code.
  export EXTRA_CFLAGS="`testcflags.sh -Wno-unused` $CFLAGS"
  unset CFLAGS

I think this is just an omission and it should be there for Python 3 also.

Apparently you added that in #19899, but didn't add it to python3, presumably just due to lack of python3 testing at the time. Probably it applies there too.

[jdemeyer]

comment:7

Replying to @embray:

Replying to @jdemeyer:

Given that these scripts are mostly the same for Python 2 and Python 3, would it be possible to have just one script (say, by symlinking) and then use $PKG_NAME inside the script where needed?

I've wondered that myself. It's tempting--maybe this ticket would be a good place to do it as long as I'm making changes...

Great! Please do.

[embray]

comment:8

For some reason I thought I already did this. I guess I'll finish this up now since this is a dependency of #22509, which I'd also really like to move forward on.

[sagetrac-git]

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

fea2a29	Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://github.com/sagemath/sage/issues/21726
e38d730	Move the post-build tests of module imports to spkg-build instead of
392d42f	Fix Python build on non-Cygwin systems (namely OSX) with case-insensitive filesystems
e6d29d7	On OSX DYLD_LIBRARY_PATH should be set a la LD_LIBRARY_PATH to run Python from its build directory
cab490e	Fix a bug when building python2/3 and there's already a libpython in

[sagetrac-git]

Changed commit from 1257d1e to cab490e

[sagetrac-git]

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

38bf1bf	Dereference symlinks while copying package build files.
5640851	Merge python2 and python3 build scripts as they are mostly identical to

[sagetrac-git]

Changed commit from cab490e to 5640851

[embray]

comment:11

As suggested, merged the mostly duplicate scripts into one (this will be much better maintenance-wise going forward). This required one small change to sage-spkg to ensure that symlinks are dereferenced when copying to the build directory.

[jdemeyer]

comment:12

I see that you are checking for the importability of various modules before installing Python. That makes sense, but it also means that you can remove the duplicate check

if [ -z `find build -name _scproxy.so` ]; then

[jdemeyer]

comment:13

Maybe also fold both import checks into one by doing something like

testmodules="ctypes math hashlib crypt readline socket"
if ...; then
    testmodules="_scproxy $testmodules"
fi
for modules in $testmodules; do

[jdemeyer]

comment:14

Please explain this:

# Make sure -L. is placed before -L$SAGE_LOCAL/lib so that python and extension
# modules are linked with the right libpython
export LDFLAGS="-L. $LDFLAGS"

We didn't need this before...

[jdemeyer]

comment:15

$CUR is undefined here:

    mkdir "$CUR/include"

Just replace it with .. since that's what it was.

[embray]

comment:16

Replying to @jdemeyer:

Please explain this:

# Make sure -L. is placed before -L$SAGE_LOCAL/lib so that python and extension
# modules are linked with the right libpython
export LDFLAGS="-L. $LDFLAGS"

We didn't need this before...

Actually you did, you just didn't know it :) I try to write reasonably descriptive commit messages: sagemath/sagetrac-mirror@cab490e

[embray]

comment:17

Replying to @jdemeyer:

I see that you are checking for the importability of various modules before installing Python. That makes sense, but it also means that you can remove the duplicate check

if [ -z `find build -name _scproxy.so` ]; then

I'm not honestly even sure what that check is for (or why it was ever separate to begin with). Could you provide some background?

[embray]

comment:18

Replying to @jdemeyer:

$CUR is undefined here:

    mkdir "$CUR/include"

Just replace it with .. since that's what it was.

Oops--carry over from a merge conflict resolution. I'll fix that.

[sagetrac-git]

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

2b686f2	I don't know what the _scproxy module is or why we test it on OSX, but we can clearly clean this up a bit
10807ff	At some point I dropped used of the $CUR variable

[jdemeyer]

Reviewer: Jeroen Demeyer

[jdemeyer]

comment:28

I don't agree with the -L. thing. I suggest to take it out and create a separate ticket of it (if you think that it is important enough).

The problem with the current approach is that those LDFLAGS are inherited by other Python packages. I don't know the exact mechanism, since it doesn't happen with the Sage library for example. But this is an example command line from scipy with this patch:

gcc -pthread -shared -L. -L/home/jdemeyer/sage/local/lib -Wl,-rpath,/home/jdemeyer/sage/local/lib -shared -L/home/jdemeyer/sage/local/lib -Wl,-rpath,/home/jdemeyer/sage/local/lib build/temp.linux-x86_64-2.7/scipy/cluster/_vq.o -L/home/jdemeyer/sage/local/lib -L/home/jdemeyer/sage/local/lib -Lbuild/temp.linux-x86_64-2.7 -lopenblas -lopenblas -lpython2.7 -o build/lib.linux-x86_64-2.7/scipy/cluster/_vq.so

Without this patch, this -L. wasn't there.

[jdemeyer]

comment:29

If you want to fix the -L. issue, I think it's better to fix the upstream build system instead.

[embray]

comment:30

I see what you're saying, but this is a problem that needs to be solved somehow.

I don't want to make a separate ticket because

a) I'm sick of refactoring

b) At some point (weeks ago, so I can't remember why) this was important enough to add to this ticket, because without it something was breaking.

[jdemeyer]

comment:31

Replying to @embray:

I see what you're saying, but this is a problem that needs to be solved somehow.

What do you think of my proposal to fix the Python upstream build system to put -L. in the correct place? That looks like the best solution. And it doesn't affect distros because this is a pure build issue.

[embray]

comment:32

Replying to @jdemeyer:

Replying to @embray:

I see what you're saying, but this is a problem that needs to be solved somehow.

What do you think of my proposal to fix the Python upstream build system to put -L. in the correct place? That looks like the best solution. And it doesn't affect distros because this is a pure build issue.

I'd definitely be fine with that--I'm just going to look around a bit more to see if there isn't already a solution to this. It seems odd to me that there wouldn't be.

[embray]

comment:33

Okay, there is a better solution, which in retrospect is obvious--instead of exporting LDFLAGS="-L. ...", just pass the additional flags at make time:

`make LDFLAGS="-L. $LDFLAGS"`

This does not change the defaults that are saved to the Makefile by configure (and hence does not impact the flags used in the future for building extension modules). But it does ensure that the correct flags are added for building the extension modules included with Python.

Whether or not this is an upstream bug is debatable, but I'll ask on python-dev.

[sagetrac-git]

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

72f5630

Don't export new LDFLAGS; instead override LDFLAGS by passing it in when running make

[sagetrac-git]

Changed commit from d227caf to 72f5630

[embray]

comment:35

How does this looks? It seems to work for me.

If you still really want me to make a separate ticket for the link issue I can do so; I would just prefer not to bother.

[jdemeyer]

comment:36

I just need to test this, but your explanation sounds OK.

[jdemeyer]

comment:37

Replying to @embray:

If you still really want me to make a separate ticket for the link issue I can do so; I would just prefer not to bother.

If there is an easy fix, that's fine. It's just that I'd rather finish this ticket instead of keeping struggling with this one issue.

[jdemeyer]

Changed branch from u/embray/build/python to u/jdemeyer/build/python

[jdemeyer]

Changed commit from 72f5630 to 43bf6e7

[jdemeyer]

comment:39

Looks good to me. One final thing: when making such substantial changes to the build/install scripts, it is safer to increase the patchlevel of the packages (even if one could argue that this is not strictly needed). I did that in a follow-up commit. Do you agree?

---

New commits:

5c57844	Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://github.com/sagemath/sage/issues/21726
43bf6e7	Increase patchlevel of Python packages

[sagetrac-git]

Changed commit from 43bf6e7 to f326c1a

[sagetrac-git]

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

cbd9323	Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://github.com/sagemath/sage/issues/21726
f326c1a	Increase patchlevel of Python packages

[embray]

comment:41

Yes--I think that's an unfortunate quirk of how we do continuous integration for Sage, but under the current circumstances I think it makes sense.

[vbraun]

Changed branch from u/jdemeyer/build/python to f326c1a