Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#11707 closed defect (duplicate)

Remove `readlink -n` and `realpath` from $SAGE_ROOT/sage

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-duplicate/invalid/wontfix
Component: scripts Keywords: SAGE_ROOT
Cc: Merged in:
Authors: Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

The file `$SAGE_ROOT/sage' contains the lines

if [ "$SAGE_ROOT" = "....." ];  then
    SAGE_ROOT=`readlink -n "$0" 2> /dev/null` || \
    SAGE_ROOT=`realpath    "$0" 2> /dev/null` || \
    SAGE_ROOT="$0"

However, readlink -n certainly does not do what is intended:

  • It only works when $0 (the sage executable itself) is a symbolic link
  • If the sage executable is a symbolic link, then readlink -n returns the link itself, not the canonicalized name. Example: if /usr/local/sage-4.7.1/sage is a symbolic link to sagefoo, then SAGE_ROOT would become sagefoo when '/usr/local/sage-4.7.1/sagefoo is intended.

Maybe, the whole readlink/realpath logic should be removed.

Change History (15)

comment:1 follow-up: Changed 10 years ago by leif

Haha, had the same idea just yesterday.

IMHO we should use (the more portable and perhaps simpler)

(cd "$WHEREVER"; NORMALIZED_PATH=`pwd -P`)

to get a canonical form in shell scripts.

I'm not sure if this also fixes the apparently sometimes prepended automount prefix (cf. sage-ptest etc.).

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

Replying to leif:

Haha, had the same idea just yesterday.

IMHO we should use (the more portable and perhaps simpler)

(cd "$WHEREVER"; NORMALIZED_PATH=`pwd -P`)

to get a canonical form in shell scripts.

I'm not sure if this also fixes the apparently sometimes prepended automount prefix (cf. sage-ptest etc.).

I guess you mean

NORMALIZED_PATH=`cd "$WHEREVER"; pwd -P`

because otherwise the "NORMALIZED_PATH" from the subshell will not be seen by the main shell.

comment:3 in reply to: ↑ 2 Changed 10 years ago by leif

Replying to jdemeyer:

I guess you mean

NORMALIZED_PATH=`cd "$WHEREVER"; pwd -P`

because otherwise the "NORMALIZED_PATH" from the subshell will not be seen by the main shell.

Yes, indeed I meant something like:

MY_PATH=${0%/*}

CANON_PATH=`cd "$MY_PATH" && pwd -P`

comment:4 Changed 10 years ago by leif

P.S.: sage-env should also get changed when guessing SAGE_ROOT (cf. also #11687).

comment:5 Changed 10 years ago by leif

I mean we should also simplify / use pwd -P in sage-env's

# GUESS SAGE_ROOT from pwd
SAVEDIR="`pwd`"
if [ -f sage -a -d spkg ]; then
    GUESSED_SAGE_ROOT="`pwd`"
else
    if [ -f ../../sage -a -d ../../spkg ]; then
        cd ../../
        GUESSED_SAGE_ROOT="`pwd`"
    else
        GUESSED_SAGE_ROOT=""
    fi
fi
cd "$SAVEDIR"

Sorry, currently doing four things at the same time. 8/

comment:6 in reply to: ↑ description ; follow-up: Changed 10 years ago by tornaria

Replying to jdemeyer:

The file `$SAGE_ROOT/sage' contains the lines

...

However, readlink -n certainly does not do what is intended:

...

Using readlink -n is definitely not "the right thing". However:

  1. Using it makes a symlink work, provided it is a full path symlink (and there are no recursive symlinks, etc)
  2. One way to properly expand the symlink is using readlink -f. That was proposed as a fix for #5852, it was applied, and later unapplied because using fully canonicalized paths causes other issues with doctesting. It just happens that using readlink -n is a middle-ground that works in some cases but using readlink -f exposes the doctesting issue in some common installs.

Thus, removing the use of readlink -n will break some common usage while not fixing anything. Replacing it by readlink -f fixes the canonicalization issue but will cause a regression by exposing a bug the way doctesting detects library code (see #5852 comment 12).


Additional remark: note that using readlink -f is not completely portable; neither is using realpath. Using both covers a majority of systems but not all of them. A more portable solution was proposed in #6146 but it doesn't make sense to push it until #5852 is resolved.

comment:7 in reply to: ↑ 6 Changed 10 years ago by tornaria

Replying to tornaria:

Thus, removing the use of readlink -n will break some common usage while not fixing

The common usage I refer to is the following:

  1. I install sage x.y in a directory ~/sage/sage-x.y
  2. I make a symlink ~/bin/sage -> ~/sage/sage-x.y/sage so that the sage command is available in my path [note the full path symlink].

This works fine right now, and would break if you remove "readlink -n".


NOTE: When "~/bin/sage" is a symlink with a relative path symlink, this doesn't work, and "readlink -f" would work. However, this leads to other breakup (this is the reason why #5852 was reverted and #6146 was not developed further). If you want to fix this you should start with #5852 (comment 12).

comment:8 follow-up: Changed 10 years ago by leif

It is not immediately clear why using pwd -P, which fully expands symbolic links, and therefore would canonicalize $SAGE_ROOT as well, should break doctesting.

Even if it did somehow, this should be addressed in the corresponding scripts, sage-{test,ptest,doctest}, which are currently under work anyway, and have certainly changed since #5852 was opened (or last modified).

Taking the canonical path of both the files to test and $SAGE_ROOT (or, more precisely, of the current branch of its Sage library), there shouldn't be any ambiguity in distinguishing Sage library code from other code. We also meanwhile have an option -force_lib to sage -t and its friends.

Slashs, dashs etc. from paths of files to test should never end up in an import statement, and I don't think they currently could, as we copy source files anyway.

comment:9 follow-up: Changed 10 years ago by jdemeyer

I attach a shell script which should be portable (however, I have not yet tested this) and behaves like "readlink -f" if combined with ( cd $dir && pwd ). I guess this would solve all problems mentioned here. I still have to think about the best way of using this from $SAGE_ROOT/sage.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 10 years ago by tornaria

Replying to jdemeyer:

I attach a shell script which should be portable (however, I have not yet tested this) and behaves like "readlink -f" if combined with ( cd $dir && pwd ). I guess this would solve all problems mentioned here. I still have to think about the best way of using this from $SAGE_ROOT/sage.

I'll suggest for a last time that you check out #6146. There is a script there that is simpler, shorter, and has some testing. Portability is not so easy... your script hangs on my first try on SunOS 5.10 (fulvia). Note also that because we are assuming bash, we can portably use "pwd -P" as a bash builtin.

If you can figure out a way to make "readlink -f" work (when available), i.e. finish fixing #5852, then #6146 already gives a solution for systems where "readlink -f" is not available.

comment:11 in reply to: ↑ 8 Changed 10 years ago by tornaria

Replying to leif:

It is not immediately clear why using pwd -P, which fully expands symbolic links, and therefore would canonicalize $SAGE_ROOT as well, should break doctesting.

The issue is described in comment:ticket:5852:12. Summary:

  • there's a symlink /home/wstein/sage-build -> /tmp/wstein
  • sage is untarred and compiled inside /home/wstein/sage-build/sage-x.y/
  • doctesting as sage -t /home/wstein/sage-x.y/...

The last step fails if SAGE_ROOT is fully canonicalized to /tmp/wstein/sage-x.y/... because in that case the doctesting code didn't realize /home/wstein/sage-x.y/... is library code. IOW, that workflow worked for William because canonicalization was broken!

I presume the fix is that the comparision done for detecting library code be done with using canonical paths, etc.

Even if it did somehow, this should be addressed in the corresponding scripts, sage-{test,ptest,doctest}, which are currently under work anyway, and have certainly changed since #5852 was opened (or last modified).

I agree. I haven't followed the development of these scripts, so I wouldn't know if the issue was addressed or not. If it is fixed, then #5852 can be reapplied and then #6164 can easily be turned into a patch that extends canonicalization to other systems where readlink -f is not available.

comment:12 Changed 10 years ago by jhpalmieri

Yesterday after reading some of this, I posted a question on #5852: has the main obstacle there been resolved? The relevant code in sage-doctest seems to resolve all symbolic links in the various paths before comparing them, so whatever we do elsewhere shouldn't cause problems with doctesting.

comment:13 in reply to: ↑ 10 Changed 10 years ago by jdemeyer

Replying to tornaria:

your script hangs on my first try on SunOS 5.10 (fulvia).

Fixed that.

If you can figure out a way to make "readlink -f" work (when available)

I don't think we should make "readlink -f" work when available. I think either your or my bash script should do the job without making any assumptions on availability of realpath or readlink -f.

comment:14 Changed 10 years ago by jdemeyer

  • Resolution set to duplicate
  • Reviewers set to Jeroen Demeyer
  • Status changed from new to closed

Let's move the whole discussion to #5852.

comment:15 Changed 10 years ago by mvngu

  • Milestone changed from sage-4.7.2 to sage-duplicate/invalid/wontfix
Note: See TracTickets for help on using tickets.