Opened 2 years ago

Closed 2 years ago

#29446 closed defect (fixed)

Unify how SAGE_ROOT and SAGE_LOCAL are normalized regarding symbolic links

Reported by: gh-mwageringel Owned by:
Priority: major Milestone: sage-9.2
Component: build Keywords:
Cc: mkoeppe, mjo Merged in:
Authors: Markus Wageringel Reviewers: Matthias Koeppe
Report Upstream: N/A Work issues:
Branch: 20f564e (Commits, GitHub, GitLab) Commit: 20f564ee650a8ce4983b6b67e822cf9e4452f44b
Dependencies: Stopgaps:

Status badges

Description (last modified by gh-mwageringel)

This ticket changes SAGE_ROOT, so that it is always the absolute physical path, i.e. symbolic links are resolved.


As reported on sage-release, when building Sage 9.1.beta9 from a path that is a symbolic link, the following test in src/sage/env.py fails:

    sage: env = {k:v for (k,v) in os.environ.items() if not k.startswith("SAGE_")}
    sage: from subprocess import check_output
    sage: cmd = "from sage.all import SAGE_ROOT, SAGE_LOCAL; print((SAGE_ROOT, SAGE_LOCAL))"
    sage: out = check_output([sys.executable, "-c", cmd], env=env).decode().strip()   # long time
    sage: out == repr((SAGE_ROOT, SAGE_LOCAL))                                        # long time

The problem is that the left path in out is the symbolic link to Sage's root directory, whereas SAGE_ROOT is the absolute path without symbolic link.

Change History (22)

comment:1 Changed 2 years ago by gh-mwageringel

Another way to see this problem directly:

./sage -sh
(sage-sh)$ unset SAGE_ROOT
(sage-sh)$ python3
>>> from sage.all import SAGE_ROOT
>>> SAGE_ROOT     # outputs the symbolic link, not the physical path

Rather than changing the doctest to use abspath, it seems that in both cases SAGE_ROOT should already be a path without symbolic links in it.

I am not sure where SAGE_ROOT is initialized for the Python executable. I tried to replace pwd by pwd -P in build/bin/sage-get-system-packages, but it did not help, unless the value is cached somewhere.

comment:2 follow-up: Changed 2 years ago by jhpalmieri

Maybe it's just not a good doctest. Why do we care what the Python executable thinks SAGE_ROOT is? The top-level sage script sets SAGE_ROOT and attempts to resolve all symbolic links. Does that work properly?

Note that this doctest is new as of 9.1.beta9. Maybe we should revert back to something closer to what was in beta8.

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

comment:3 in reply to: ↑ 2 Changed 2 years ago by gh-mwageringel

Replying to jhpalmieri:

Why do we care what the Python executable thinks SAGE_ROOT is?

I assume this is a step towards making sagelib more usable from Python. Previously, SAGE_ROOT was just None.

The top-level sage script sets SAGE_ROOT and attempts to resolve all symbolic links. Does that work properly?

Yes, that works as usual.

comment:4 Changed 2 years ago by mkoeppe

  • Milestone changed from sage-9.1 to sage-9.2

comment:5 Changed 2 years ago by gh-mwageringel

As of 9.2.beta3, I get a lot more test failures when building Sage in a location that is a symbolic link. The problem is that SAGE_ROOT changes between the symbolic link and the real path. For example:

sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/doctest/test.py
**********************************************************************
File "/home/math/mwagerin/git/sage_compute/src/sage/doctest/test.py", line 26, in sage.doctest.test
Failed example:
    subprocess.call(["sage", "-t", "--warn-long", "0", "longtime.rst"], **kwds)  # long time
Expected:
    Running doctests...
    Doctesting 1 file.
    sage -t --warn-long 0.0 longtime.rst
    [0 tests, ...s]
    ----------------------------------------------------------------------
    All tests passed!
    ----------------------------------------------------------------------
    ...
    0
Got:
    Warning: overwriting SAGE_ROOT environment variable:
    Old SAGE_ROOT=/home/math/mwagerin/git/sage_compute
    New SAGE_ROOT=/amd/compute/mwagerin/git/sage_compute
    Warning: overwriting SAGE_ROOT environment variable:
    Old SAGE_ROOT=/home/math/mwagerin/git/sage_compute
    New SAGE_ROOT=/amd/compute/mwagerin/git/sage_compute
    Running doctests with ID 2020-07-05-17-16-51-1a9a4d7d.
    Git branch: develop
    Using --optional=build,dochtml,memlimit,sage
    Doctesting 1 file.
    sage -t --warn-long 0.0 longtime.rst
        [0 tests, 0.00 s]
    ----------------------------------------------------------------------
    All tests passed!
    ----------------------------------------------------------------------
    Total time for all tests: 0.0 seconds
        cpu time: 0.0 seconds
        cumulative wall time: 0.0 seconds
    0
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/tests/cmdline.py
**********************************************************************
File "/home/math/mwagerin/git/sage_compute/src/sage/tests/cmdline.py", line 119, in sage.tests.cmdline.test_executable
Failed example:
    err
Expected:
    ''
Got:
    'Warning: overwriting SAGE_ROOT environment variable:\nOld SAGE_ROOT=/home/math/mwagerin/git/sage_compute\nNew SAGE_ROOT=/amd/compute/mwagerin/git/sage_co
mpute\nWarning: overwriting SAGE_ROOT environment variable:\nOld SAGE_ROOT=/home/math/mwagerin/git/sage_compute\nNew SAGE_ROOT=/amd/compute/mwagerin/git/sage_
compute\n'
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/doctest/test.py  # 23 doctests failed
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/plot/plot.py  # 1 doctest failed
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/tests/cmdline.py  # 28 doctests failed
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/libs/ppl.pyx  # 9 doctests failed
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/repl/interpreter.py  # 1 doctest failed
sage -t --long --warn-long 47.7 /home/math/mwagerin/git/sage_compute/src/sage/misc/temporary_file.py  # 1 doctest failed

comment:6 follow-up: Changed 2 years ago by mkoeppe

I think it needs to be investigated whether the resolvelinks logic in src/bin/sage-env serves any useful purpose. My guess is that it probably was motivated by the filesystem relocation mechanism for SAGE_ROOT, which has been defunct for years.

comment:7 Changed 2 years ago by gh-mwageringel

Just for the record, the new behaviour is a result of #25486.

comment:8 Changed 2 years ago by mkoeppe

That's right.

comment:9 Changed 2 years ago by mkoeppe

  • Summary changed from SAGE_ROOT should avoid symbolic links to Unify how SAGE_ROOT and SAGE_LOCAL are normalized regarding symbolic links

comment:10 Changed 2 years ago by jhpalmieri

resolvelinks seems to have been introduced in #5852.

comment:11 Changed 2 years ago by mkoeppe

  • Cc mjo added

comment:12 in reply to: ↑ 6 ; follow-ups: Changed 2 years ago by gh-mwageringel

Replying to mkoeppe:

I think it needs to be investigated whether the resolvelinks logic in src/bin/sage-env serves any useful purpose.

It seems that resolvelinks is not related to this, as it messes with relative links, not absolute ones.

I think the problem is that src/bin/sage-env-config sets SAGE_ROOT to the symbolic link after running configure from the symbolic location. Rerunning configure from the physical location sets SAGE_ROOT back to the physical path.

Should this problem be handled in src/bin/sage-env-config.in or in src/bin/sage-env?

comment:13 in reply to: ↑ 12 Changed 2 years ago by mkoeppe

Replying to gh-mwageringel:

Replying to mkoeppe: Should this problem be handled in src/bin/sage-env-config.in or in src/bin/sage-env?

From the comments at the top of src/bin/sage-env-config.in:

#  - This file is only for setting immediate values.  Any kind of conditionals
#    or computed values are to be set by src/bin/sage-env after sourcing this
#    file.

comment:14 in reply to: ↑ 12 Changed 2 years ago by mkoeppe

Replying to gh-mwageringel:

I think the problem is that src/bin/sage-env-config sets SAGE_ROOT to the symbolic link after running configure from the symbolic location. Rerunning configure from the physical location sets SAGE_ROOT back to the physical path.

Are you saying that the only setting in which you observe the reported warnings is when you manually call configure with different SAGE_ROOTs?

Then I'm not sure if anything needs to be fixed.

comment:15 Changed 2 years ago by mkoeppe

In $SAGE_ROOT/src/bin/sage-env:

# Make NEW_SAGE_ROOT absolute
NEW_SAGE_ROOT=`cd "$NEW_SAGE_ROOT" && pwd -P`

In $SAGE_ROOT/sage:

# Make SAGE_ROOT absolute
SAGE_ROOT=`cd "$SAGE_ROOT" && pwd -P`

Of course, these comments are not precise. The -P option makes these directories not just "absolute" but actually "physical" (all symlinks resolved).

In contrast, in $SAGE_ROOT/src/bin/sage-env-config.in:

export SAGE_ROOT="@abs_top_srcdir@"

autoconf's @abs_top_srcdir@ is only absolute, but not physical.

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

comment:16 Changed 2 years ago by mkoeppe

Also, in configure.ac, the $SAGE_ROOT is made physical:

# Assume current directory is SAGE_ROOT.
SAGE_ROOT=`pwd -P`

comment:17 follow-up: Changed 2 years ago by mkoeppe

I see two possible fixes:

a) Get rid of all the above -P. It's up the user to express the SAGE_ROOT using whatever symlinks they like to use. Use pwd -P only for the following: Issue the warning regarding changing SAGE_ROOT in sage-env only if they do not resolve to the same physical path.

b) AC_SUBST(SAGE_ROOT) and change export SAGE_ROOT="@abs_top_srcdir@" to ...@SAGE_ROOT@.

comment:18 in reply to: ↑ 17 Changed 2 years ago by gh-mwageringel

Replying to mkoeppe:

Are you saying that the only setting in which you observe the reported warnings is when you manually call configure with different SAGE_ROOTs?

./configure determines SAGE_ROOT automatically, without explicitly setting it, but the result depends on whether the current directory is symbolic or not.

Replying to mkoeppe:

I see two possible fixes:

a) Get rid of all the above -P. It's up the user to express the SAGE_ROOT using whatever symlinks they like to use. Use pwd -P only for the following: Issue the warning regarding changing SAGE_ROOT in sage-env only if they do not resolve to the same physical path.

b) AC_SUBST(SAGE_ROOT) and change export SAGE_ROOT="@abs_top_srcdir@" to ...@SAGE_ROOT@.

I prefer option b), since with a) it is confusing if the behavior depends on the (symbolic) path that was used to cd into SAGE_ROOT. If, for example, I open a new tab from SAGE_ROOT, the current directory is always the physical path, so it is easy to confuse the two.

Option b) seems to work for me, but to test this on the affected machine I first need to install dependencies for bootstrap.

comment:19 Changed 2 years ago by gh-mwageringel

  • Authors set to Markus Wageringel
  • Branch set to u/gh-mwageringel/29446
  • Commit set to 20f564ee650a8ce4983b6b67e822cf9e4452f44b
  • Description modified (diff)
  • Status changed from new to needs_review

Ok, this works for me now. With this branch, SAGE_ROOT will always be the absolute physical path, like it used to be until recently. In particular, the doctest from the description works now.

SAGE_LOCAL was already the physical path, so nothing needs to be changed for that.


New commits:

20f564e29446: improve handling of SAGE_ROOT to avoid symbolic links

comment:20 Changed 2 years ago by mkoeppe

  • Reviewers set to Matthias Koeppe
  • Status changed from needs_review to positive_review

comment:21 Changed 2 years ago by gh-mwageringel

Thank you.

comment:22 Changed 2 years ago by vbraun

  • Branch changed from u/gh-mwageringel/29446 to 20f564ee650a8ce4983b6b67e822cf9e4452f44b
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.