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: sage-8.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 jhpalmieri

  • Branch set to u/jhpalmieri/no-local-lib-python

comment:2 Changed 3 years ago by jhpalmieri

  • Commit set to 0e90d5294f2dd1f95f2e96bf34830d1256a3f211
  • Status changed from new to needs_review

New commits:

0e90d52Trac 22764: remove the link SAGE_LOCAL/lib/python -> python2.7

comment:3 Changed 3 years ago by git

  • Commit changed from 0e90d5294f2dd1f95f2e96bf34830d1256a3f211 to f33e86f76463c02dee30513fd5553ebbe5a228d5

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

f33e86ftrac 22764: replace a few instances of

comment:4 Changed 3 years ago by git

  • Commit changed from f33e86f76463c02dee30513fd5553ebbe5a228d5 to b02ecf34018f48b24012623c5388a34d589bb3d2

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

b02ecf3trac 22764: in a few comments and print statements, fix a few

comment:5 Changed 3 years ago by chapoton

see also #22638

comment:6 Changed 3 years ago by chapoton

  • Status changed from needs_review to needs_work

does not apply

comment:7 Changed 3 years ago by git

  • Commit changed from b02ecf34018f48b24012623c5388a34d589bb3d2 to df6a763f1c6190cbcb33501fdf7b6f4e5ee46400

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

df6a763trac 22764: rebase to Sage 8.0.beta1

comment:8 Changed 3 years ago by jhpalmieri

  • 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 git

  • Commit changed from df6a763f1c6190cbcb33501fdf7b6f4e5ee46400 to 7cd0d62ad1fe72747813b665682fea591aaf2a80

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

7cd0d62trac 22764: merge with 8.0.beta1

comment:10 Changed 3 years ago by jhpalmieri

Edit: moved the comment to #22756.

Last edited 3 years ago by jhpalmieri (previous) (diff)

comment:11 Changed 3 years ago by jdemeyer

  • 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 jhpalmieri

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 
    383383# Prepare for running Sage, either interactively or non-interactively.
    384384sage_setup() {
    385385    # 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/site-packages/sage" ]; then
     391            built=no
     392        fi
     393    fi
     394    if [ $built = no ]; then
    387395        echo >&2 '************************************************************************'
    388396        echo >&2 'It seems that you are attempting to run Sage from an unpacked source'
    389397        echo >&2 'tarball, but you have not compiled it yet (or maybe the build has not'

How about that?

Last edited 3 years ago by jhpalmieri (previous) (diff)

comment:13 Changed 3 years ago by vbraun

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 jhpalmieri

I think that SAGE_LOCAL/bin/sage can get installed well before the Sage library.

comment:15 Changed 3 years ago by vbraun

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 jhpalmieri

Either way is fine with me: check for local/lib/python$PYTHON_VERSION/site-packages/sage/ or for local/bin/sage.

comment:17 Changed 3 years ago by git

  • Commit changed from 7cd0d62ad1fe72747813b665682fea591aaf2a80 to ab49413b6ee1f7eebcc9044caf2c0abc45aad746

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

ab49413trac 22764: change how check whether Sage is built.

comment:18 Changed 3 years ago by jhpalmieri

  • 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 follow-ups: Changed 3 years ago by vbraun

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 jhpalmieri

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 run-time aspects of Sage, not just build-time. 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 jhpalmieri

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 run-time effects or not, we can modify this check.

comment:22 Changed 3 years ago by git

  • Commit changed from ab49413b6ee1f7eebcc9044caf2c0abc45aad746 to fb90d5e3667237f2d9fc7ba982b466bc52802b1d

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

4715169merging with 8.0.beta2
fb90d5etrac 22764: check whether Sage is built by looking for local/bin/sage

comment:23 Changed 3 years ago by jhpalmieri

But I don't really care that much.

comment:24 Changed 3 years ago by jhpalmieri

Should we bump the version numbers for python2 and python3, to make sure their spkg-install files get run, so the link actually gets removed when people upgrade?

comment:25 Changed 3 years ago by vbraun

  • 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 jhpalmieri

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 vbraun

  • Branch changed from u/jhpalmieri/no-local-lib-python to fb90d5e3667237f2d9fc7ba982b466bc52802b1d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.