Unify how SAGE_ROOT and SAGE_LOCAL are normalized regarding symbolic links

[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.

CC: @mkoeppe @orlitzky

Component: build

Author: Markus Wageringel

Branch/Commit: 20f564e

Reviewer: Matthias Koeppe

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

[mwageringel]

comment:1

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.

[jhpalmieri]

comment:2

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.

[mwageringel]

comment:3

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.

[mwageringel]

comment:5

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

[mkoeppe]

comment:6

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.

[mwageringel]

comment:7

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

[mkoeppe]

comment:8

That's right.

[jhpalmieri]

comment:10

resolvelinks seems to have been introduced in #5852.

[mwageringel]

comment:12

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?

[mkoeppe]

comment:13

Replying to @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.

[mkoeppe]

comment:14

Replying to @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.

[mkoeppe]

comment:15

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.

[mkoeppe]

comment:16

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

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

[mkoeppe]

comment:17

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@.

[mwageringel]

comment:18

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.

[mwageringel]

Author: Markus Wageringel

[mwageringel]

Branch: u/gh-mwageringel/29446

[mwageringel]

comment:19

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:

20f564e

29446: improve handling of SAGE_ROOT to avoid symbolic links

[mwageringel]

Commit: 20f564e

[mkoeppe]

Reviewer: Matthias Koeppe

[mwageringel]

comment:21

Thank you.

[vbraun]

Changed branch from u/gh-mwageringel/29446 to 20f564e