Opened 2 years ago

Closed 2 years ago

#23781 closed defect (fixed)

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

Reported by: embray Owned by:
Priority: major Milestone: sage-8.1
Component: packages: standard Keywords: python2 python3
Cc: Merged in:
Authors: Erik Bray Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: f326c1a (Commits) Commit: f326c1acfa9804d583f03611e729c3bebeff4083
Dependencies: Stopgaps:

Description

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.

Change History (43)

comment:1 Changed 2 years ago by embray

  • Status changed from new to needs_review

comment:2 follow-up: Changed 2 years ago by 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?

comment:3 follow-up: Changed 2 years ago by 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.

comment:4 Changed 2 years ago by jdemeyer

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

comment:5 in reply to: ↑ 2 ; follow-up: Changed 2 years ago by 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...

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

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.

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

  • Status changed from needs_review to needs_work

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.

comment:8 Changed 2 years ago by embray

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.

comment:9 Changed 2 years ago by git

  • Commit changed from 1257d1ea0809a9afbabdba2168012090f19d183b to cab490ec9f568b6bf70ede31f93ba86d71c865e8

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

fea2a29Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://trac.sagemath.org/ticket/21726
e38d730Move the post-build tests of module imports to spkg-build instead of
392d42fFix Python build on non-Cygwin systems (namely OSX) with case-insensitive filesystems
e6d29d7On OSX DYLD_LIBRARY_PATH should be set a la LD_LIBRARY_PATH to run Python from its build directory
cab490eFix a bug when building python2/3 and there's already a libpython in

comment:10 Changed 2 years ago by git

  • Commit changed from cab490ec9f568b6bf70ede31f93ba86d71c865e8 to 56408511f00875d68b76c150455b2bcfb5f0174f

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

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

comment:11 Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

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.

comment:12 follow-up: Changed 2 years ago by 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

comment:13 Changed 2 years ago by jdemeyer

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

comment:14 follow-up: Changed 2 years ago by 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...

comment:15 follow-up: Changed 2 years ago by jdemeyer

$CUR is undefined here:

    mkdir "$CUR/include"

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

comment:16 in reply to: ↑ 14 Changed 2 years ago by embray

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: https://git.sagemath.org/sage.git/commit/?h=56408511f00875d68b76c150455b2bcfb5f0174f&id=cab490ec9f568b6bf70ede31f93ba86d71c865e8

comment:17 in reply to: ↑ 12 ; follow-up: Changed 2 years ago by embray

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?

comment:18 in reply to: ↑ 15 Changed 2 years ago by embray

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.

comment:19 Changed 2 years ago by git

  • Commit changed from 56408511f00875d68b76c150455b2bcfb5f0174f to 10807ff837153ad81fd2d3c73e611acd4db3d499

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

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

comment:20 in reply to: ↑ 17 ; follow-up: Changed 2 years ago by jdemeyer

Replying to embray:

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?

Well, the old install script worked like:

  1. Build Python
  1. Check that _scproxy.so was compiled (Python does not consider it an error if this optional module was not compiled)
  1. Install Python
  1. Check that _scproxy can be imported.

Earlier versions of the install script did not have step B. This means that a broken Python could be installed in step C. So step B was added as a way of doing step D before installing. But now that you have arranged things like

  1. Build Python
  1. Check that _scproxy.so was compiled (Python does not consider it an error if this optional module was not compiled)
  1. Check that _scproxy can be imported.
  1. Install Python

there is no longer any reason for B: if _scproxy can be imported, surely it was compiled.

comment:21 Changed 2 years ago by embray

I'm going to try testing this on OSX. I now supposedly have access to an OSX machine now, but I haven't tried using it for anything yet... Would be great if there were a reliable OSX patchbot (maybe once I get things up and running there will be :)

comment:22 in reply to: ↑ 20 ; follow-up: Changed 2 years ago by embray

Replying to jdemeyer:

Replying to embray: there is no longer any reason for B: if _scproxy can be imported, surely it was compiled.

I see. I'll remove "B" then.

I just meant I don't know what the _scproxy module is or why it's checked for in the first place.

comment:23 Changed 2 years ago by git

  • Commit changed from 10807ff837153ad81fd2d3c73e611acd4db3d499 to d227caf4c453df5c7af4a5d88ef52e8fda65874a

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

d227cafThis check is unnecessary since we test importing the _scproxy module just below--if it can be imported then surely it exists.

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

Replying to embray:

I just meant I don't know what the _scproxy module is or why it's checked for in the first place.

Well, I don't know it either. But clearly people consider it important.


New commits:

d227cafThis check is unnecessary since we test importing the _scproxy module just below--if it can be imported then surely it exists.

comment:25 follow-up: Changed 2 years ago by jdemeyer

I still don't fully understand the -L. issue. Under which scenario would things break without it?

comment:26 in reply to: ↑ 25 Changed 2 years ago by embray

Replying to jdemeyer:

I still don't fully understand the -L. issue. Under which scenario would things break without it?

Sure, I'll demonstrate. Say I disable that, and I also (for some reason) change --enable-unicode=ucs4 to --enable-unicode=ucs2 (this is just an easy way to wind up with an incompatible libpython). Then I run ./sage -f python2. Soon as it goes to build the first extension module:

[python2-2.7.13.p1] gcc -shared -Wl,--enable-auto-image-base -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib -L/home/embray/src/sagemath/sage/local/lib -Wl,-rpath,/home/embray/src/sagemath/sage/local/lib build/temp.cygwin-2.8.0-x86_64-2.7/home/embray/src/sagemath/sage/local/var/tmp/sage/build/python2-2.7.13.p1/src/Modules/_struct.o -L/home/embray/src/sagemath/sage/local/lib -L/usr/local/lib -L. -L. -lpython2.7 -o build/lib.cygwin-2.8.0-x86_64-2.7/_struct.dll
[python2-2.7.13.p1] build/temp.cygwin-2.8.0-x86_64-2.7/home/embray/src/sagemath/sage/local/var/tmp/sage/build/python2-2.7.13.p1/src/Modules/_struct.o: In function `s_init':
[python2-2.7.13.p1] /home/embray/src/sagemath/sage/local/var/tmp/sage/build/python2-2.7.13.p1/src/Modules/_struct.c:1383: undefined reference to `PyUnicodeUCS2_AsEncodedString'
[python2-2.7.13.p1] /home/embray/src/sagemath/sage/local/var/tmp/sage/build/python2-2.7.13.p1/src/Modules/_struct.c:1383:(.text+0x1234): relocation truncated to fit: R_X86_64_PC32 against undefined symbol `PyUnicodeUCS2_AsEncodedString'
[python2-2.7.13.p1] collect2: error: ld returned 1 exit status

Those gcc flags are already something of a mess. Several of the flags are duplicated (I'm not sure why). But also, as I explained in the commit message, our LDFLAGS are inserted very early on, so the linker finds -lpython2.7 in $SAGE_LOCAL/lib, when it should be finding it in the current directory--the libpython that was just built.

comment:27 Changed 2 years ago by embray

I'll note it's possible this isn't as often a problem on Linux since it's not necessary to resolve all symbols at link time when linking shared libraries.

Last edited 2 years ago by embray (previous) (diff)

comment:28 Changed 2 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to needs_work

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.

comment:29 Changed 2 years ago by jdemeyer

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

comment:30 follow-up: Changed 2 years ago by embray

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.

Last edited 2 years ago by embray (previous) (diff)

comment:31 in reply to: ↑ 30 ; follow-up: Changed 2 years ago by 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.

comment:32 in reply to: ↑ 31 Changed 2 years ago by embray

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.

comment:33 Changed 2 years ago by embray

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.

comment:34 Changed 2 years ago by git

  • Commit changed from d227caf4c453df5c7af4a5d88ef52e8fda65874a to 72f5630ad4ba182aec27155ff48334a2da34a8fc

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

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

comment:35 follow-up: Changed 2 years ago by embray

  • Status changed from needs_work to needs_review

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.

comment:36 Changed 2 years ago by jdemeyer

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

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

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.

comment:38 Changed 2 years ago by jdemeyer

  • Branch changed from u/embray/build/python to u/jdemeyer/build/python

comment:39 Changed 2 years ago by jdemeyer

  • Commit changed from 72f5630ad4ba182aec27155ff48334a2da34a8fc to 43bf6e732d4b38e7f6b49dc1fa31a65b81dc676e

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:

5c57844Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://trac.sagemath.org/ticket/21726
43bf6e7Increase patchlevel of Python packages

comment:40 Changed 2 years ago by git

  • Commit changed from 43bf6e732d4b38e7f6b49dc1fa31a65b81dc676e to f326c1acfa9804d583f03611e729c3bebeff4083

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

cbd9323Split spkg-install for python2/3 into spkg-build and spkg-install as recommended in https://trac.sagemath.org/ticket/21726
f326c1aIncrease patchlevel of Python packages

comment:41 Changed 2 years ago by embray

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.

comment:42 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:43 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/build/python to f326c1acfa9804d583f03611e729c3bebeff4083
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.