Opened 3 years ago
Closed 3 years ago
#22764 closed defect (fixed)
Remove the link SAGE_LOCAL/lib/python
Reported by:  jhpalmieri  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  build  Keywords:  
Cc:  Merged in:  
Authors:  John Palmieri  Reviewers:  Volker Braun 
Report Upstream:  N/A  Work issues:  
Branch:  fb90d5e (Commits)  Commit:  fb90d5e3667237f2d9fc7ba982b466bc52802b1d 
Dependencies:  Stopgaps: 
Description
We do not need the link SAGE_LOCAL/lib/python
, pointing to python2.7
, and it can interfere with the Python 3 build, at least on OS X. (See #22756.)
Change History (27)
comment:1 Changed 3 years ago by
 Branch set to u/jhpalmieri/nolocallibpython
comment:2 Changed 3 years ago by
 Commit set to 0e90d5294f2dd1f95f2e96bf34830d1256a3f211
 Status changed from new to needs_review
comment:3 Changed 3 years ago by
 Commit changed from 0e90d5294f2dd1f95f2e96bf34830d1256a3f211 to f33e86f76463c02dee30513fd5553ebbe5a228d5
Branch pushed to git repo; I updated commit sha1. New commits:
f33e86f  trac 22764: replace a few instances of

comment:4 Changed 3 years ago by
 Commit changed from f33e86f76463c02dee30513fd5553ebbe5a228d5 to b02ecf34018f48b24012623c5388a34d589bb3d2
Branch pushed to git repo; I updated commit sha1. New commits:
b02ecf3  trac 22764: in a few comments and print statements, fix a few

comment:5 Changed 3 years ago by
see also #22638
comment:7 Changed 3 years ago by
 Commit changed from b02ecf34018f48b24012623c5388a34d589bb3d2 to df6a763f1c6190cbcb33501fdf7b6f4e5ee46400
Branch pushed to git repo; I updated commit sha1. New commits:
df6a763  trac 22764: rebase to Sage 8.0.beta1

comment:8 Changed 3 years ago by
 Status changed from needs_work to needs_review
This should apply now. The old version changed src/sage/all.py
to fix a doctest. The conflict replaced the old doctest with the one from Sage 8.0.beta1 that looks more robust to me. I haven't built with this change, though, so I can't promise no doctest failures.
comment:9 Changed 3 years ago by
 Commit changed from df6a763f1c6190cbcb33501fdf7b6f4e5ee46400 to 7cd0d62ad1fe72747813b665682fea591aaf2a80
Branch pushed to git repo; I updated commit sha1. New commits:
7cd0d62  trac 22764: merge with 8.0.beta1

comment:10 Changed 3 years ago by
Edit: moved the comment to #22756.
comment:11 Changed 3 years ago by
 Status changed from needs_review to needs_work
I don't agree with the change to src/bin/sage
. It needs to check whether Sage was built inside $SAGE_LOCAL
and the new code doesn't do that (it could potentially import a different version of Sage installed on the system).
comment:12 Changed 3 years ago by
I wasn't completely satisfied with that change, either. We could do something like this instead:

src/bin/sage
diff git a/src/bin/sage b/src/bin/sage index cc6e41cbed..b26618a3b8 100755
a b fi 383 383 # Prepare for running Sage, either interactively or noninteractively. 384 384 sage_setup() { 385 385 # Check that we're not in a source tarball which hasn't been built yet (#13561). 386 if ! python c 'import sage.version' 2> /dev/null ; then 386 if [ ! x "$SAGE_LOCAL/bin/python" ]; then 387 built=no 388 else 389 PYTHON_VERSION=$("$SAGE_LOCAL/bin/python" c 'import sys; print("%d.%d" % sys.version_info[:2])') 390 if [ ! d "$SAGE_LOCAL/lib/python$PYTHON_VERSION/sitepackages/sage" ]; then 391 built=no 392 fi 393 fi 394 if [ $built = no ]; then 387 395 echo >&2 '************************************************************************' 388 396 echo >&2 'It seems that you are attempting to run Sage from an unpacked source' 389 397 echo >&2 'tarball, but you have not compiled it yet (or maybe the build has not'
How about that?
comment:13 Changed 3 years ago by
Can't we just check for the presence of SAGE_LOCAL/bin/sage instead of messing around with python minutae?
comment:14 Changed 3 years ago by
I think that SAGE_LOCAL/bin/sage
can get installed well before the Sage library.
comment:15 Changed 3 years ago by
Yes, but so what? Its only a sanity check that you don't try to run an unpacked source tarball.
comment:16 Changed 3 years ago by
Either way is fine with me: check for local/lib/python$PYTHON_VERSION/sitepackages/sage/
or for local/bin/sage
.
comment:17 Changed 3 years ago by
 Commit changed from 7cd0d62ad1fe72747813b665682fea591aaf2a80 to ab49413b6ee1f7eebcc9044caf2c0abc45aad746
Branch pushed to git repo; I updated commit sha1. New commits:
ab49413  trac 22764: change how check whether Sage is built.

comment:18 Changed 3 years ago by
 Status changed from needs_work to needs_review
I decided that I like the more involved check. It is not complicated and it is more in line with the printed message, especially the part "(or maybe the build has not finished)".
comment:19 followups: ↓ 20 ↓ 21 Changed 3 years ago by
Thats hardcoding the assumption that python is a symlink to python3 when built with SAGE_PYTHON3=yes and would conflict with #22582
comment:20 in reply to: ↑ 19 Changed 3 years ago by
Replying to vbraun:
Thats hardcoding the assumption that python is a symlink to python3 when built with SAGE_PYTHON3=yes
Yes, you're right.
and would conflict with #22582
It would conflict with your current branch at #22582, but there is another option: making a reliable symlink python > python3
when SAGE_PYTHON3=yes
. As I said at #22582, I would like to hear some other opinions about those two options. (As I also said at #22582, maybe SAGE_PYTHON3
should control some runtime aspects of Sage, not just buildtime. If such were available and did not just include the symlink, we could use those for this check.)
comment:21 in reply to: ↑ 19 Changed 3 years ago by
Replying to vbraun:
Thats hardcoding the assumption that python is a symlink to python3 when built with SAGE_PYTHON3=yes and would conflict with #22582
Another way of looking at this: if we're not really thinking about how to run Sage using python3, that it's premature to do so, then: if you build with SAGE_PYTHON3=yes
, the build will not complete because Sage isn't ready for Python 3 yet, so no matter what the symlink points to, you will get this message. If you build without SAGE_PYTHON3=yes
, then the symlink should point to python2
and this check will behave as it always has.
If we just check whether local/bin/sage
exists, then you will get a less clear error message if Sage hasn't finished building yet, for whatever reason.
Once we figure out whether SAGE_PYTHON3
has runtime effects or not, we can modify this check.
comment:22 Changed 3 years ago by
 Commit changed from ab49413b6ee1f7eebcc9044caf2c0abc45aad746 to fb90d5e3667237f2d9fc7ba982b466bc52802b1d
comment:23 Changed 3 years ago by
But I don't really care that much.
comment:24 Changed 3 years ago by
Should we bump the version numbers for python2 and python3, to make sure their spkginstall files get run, so the link actually gets removed when people upgrade?
comment:25 Changed 3 years ago by
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
Its not actually harmful, so I'm fine with leaving the old link hang around...
comment:26 Changed 3 years ago by
The link can break the python3 build on OS X, but #22756 bumps the version number on python3 to try to address this. It still may break the first time, but in such a way that the build will get far enough to delete the link. After that it should work the next time.
comment:27 Changed 3 years ago by
 Branch changed from u/jhpalmieri/nolocallibpython to fb90d5e3667237f2d9fc7ba982b466bc52802b1d
 Resolution set to fixed
 Status changed from positive_review to closed
New commits:
Trac 22764: remove the link SAGE_LOCAL/lib/python > python2.7