SageNB broken when SAGE_PATH contains flavours of '.' during installation

[hivert]

When SAGE_PATH (and hence also PYTHONPATH) contains "." (the current working directory) during the installation of sagenb, Python is able to find sagenb in its path and therefore refuses to add it to easy-install.pth. Later, Sage won't find it, leading to the following error message:

ImportError: No module named sagenb.misc.sphinxify
Error importing ipy_profile_sage - perhaps you should run %upgrade?
WARNING: Loading of ipy_profile_sage failed.

This is a follow-up to #10176, and there has been some discussion on ​sage-release.

---

Apply only

10192-remove_dot_from_PYTHONPATH_in_sage-spkg.2.patch​ to the Sage root repository.

[leif]

From sage-env:

if [ -d "$SAGE_ROOT/local/lib/python" ]; then
    PYTHONPATH="$SAGE_PATH:$SAGE_ROOT/local/lib/python"   && export PYTHONPATH
    PYTHONHOME="$SAGE_ROOT/local" && export PYTHONHOME
fi

We shouldn't add $SAGE_PATH there if it is empty btw., but the removal of "." from PYTHONPATH should be performed in sage-spkg I think.

($SAGE_PATH is also appended to an overwritten(?!) PYTHONPATH in sage-doctest.)

[leif]

Ok, after changing some instances of PYTHON_PATH to PYTHONPATH (argh!), the attached patch works for me.

You can try setting SAGE_PATH to something containing "." (e.g. ":/foo/bar:.:/baz, too), of course also doing export SAGE_PATH, then [re]installing the SageNB 0.8.7 spkg (or the newer ones, p1 or p2, but these aren't necessary, i.e., the patch here fixes the issue in a different way, not needing an additional work-around).

[leif]

As far as I know, Cygwin (as opposed to MS DOS/Windows) also uses ":" to separate directories in search paths, so this should work for Cygwin as well.

(Other shell and even Python scripts in Sage also hard-code that character.)

[hivert]

Replying to leif:

Applying the patch give me

patching file sage-spkg
Hunk #1 succeeded at 292 (offset -8 lines).

Are you sure that you didn't change anything else from a vanilla 4.6.rc0 ?

Anyway, for what concerns openSUSE 11.1 and 11.3 the patch perfectly works. I also checked it on sage.sagemath.org. So I'm ok to give it a positive review, except for the Cygwin question where I'm not competent. Please do it on my behalf as soon as you have Mike answer.

[leif]

Replying to hivert:

Applying the patch give me

patching file sage-spkg
Hunk #1 succeeded at 292 (offset -8 lines).

Are you sure that you didn't change anything else from a vanilla 4.6.rc0 ?

Ok, not based on vanilla rc0; I've also applied a completely unrelated patch to sage-spkg (removing "x" from tar by default, #10040).

[leif]

Apply to scripts repo. Based on vanilla Sage 4.6.rc0. (Updated version, handles more weird cases, too.)

[leif]

I've uploaded a slightly improved patch, this time really based on vanilla 4.6.rc0.

Try again...

(also with SAGE_PATH=".::./././//./://bingo//:baz:::://///:./.:." and alike) ;-)

Although it might still fail with e.g. SAGE_PATH="./foo/.." (if ./foo/ happens to exist in the directory setup is run in), but this would at least for SageNB be catched by the 0.8.7.p{1,2} spkgs...

[leif]

ping...

(Still needs review, only the Cygwin question seems still open, though I believe it'll also work on Cygwin.)

The patch will almost certainly have to get rebased, haven't tried yet.

[leif]

SCRIPTS repo. Same patch, rebased to Sage 4.7.1.rc2.

[leif]

Replying to leif:

The patch will almost certainly have to get rebased, haven't tried yet.

Wow, the patch still applied (with an offset of only a few lines); I've rebased it on Sage 4.7.1.rc2 despite of that to now apply cleanly.

There are currently some concurrent tickets also patching sage-spkg though.

[ppurka]

New attempt to clean up even nastier paths (including .., ./., etc)

[ppurka]

New attempt to clean up even nastier paths (including .., ./., etc) (Ignore the other patch)

[ppurka]

Please check trac_10192-remove_dot_and_more_from_PYTHONPATH_in_sage.spkg.patch​ against other platforms. I have checked it on Linux x86_64 and it cleans up nasty examples like the one mentioned earlier ".::./././//./://bingo//:baz:::://///:./.:.". (Indented the paste below :))

 ~ $ AAAA=".::./././//./://bingo//:baz:::://///:./.:."
 ~ $ new_pp=""
 ~ $ IFS=":"
 ~ $ for x in $AAAA; do
  if [ -n "$x" -a -n "$(echo "$x" | sed -e 's/[\.\/]\+//g')" ]; then
    if [ -z "$new_pp" ]; then
      new_pp="$x"
    else
      new_pp="${new_pp}:$x"
    fi
  fi
done 
 ~ $ echo "$new_pp"
//bingo//:baz

This is against 4.7.0. I will later try to upload it rebased against 4.7.2.

[leif]

And what's the advantage of running sed (and twice test, and echo) multiple times in a loop? B)

I'm not sure right now whether my patch is up-to-date btw., as the currently attached one doesn't remove redundant slashs and (e.g.) ./... 8/

[ppurka]

Replying to leif:

And what's the advantage of running sed (and twice test, and echo) multiple times in a loop? B)

I'm not sure right now whether my patch is up-to-date btw., as the currently attached one doesn't remove redundant slashs and (e.g.) ./... 8/

The main point in the patch is the separator that is redefined to ':'.

Now when you run a loop through the variable, it loops through all the parts separated by ':'. Once each part is separated, you can check whether it is empty (hence the first test -n "$x"), or whether it consists of only . and / (hence the echo + sed). This is important because this catches all the bad cases like .., ./., ., etc. ( In relation to #11914, I tried to cd to devel/sage/sage and ran sage as SAGE_PATH=.. sage -n and it failed to run. So we should definitely weed out at least .. by itself. ) I do feel that it is over-engineered, but I haven't been able to come up with a better solution that catches all the cases.

Since all the scripts in Sage are trying to conform to POSIX you need the echo + sed. In pure bash, you can replace the "$( echo "$x" | sed ... )" with simply "${x//[\.\/]}"

[leif]

Replying to ppurka:

Replying to leif:

And what's the advantage of running sed (and twice test, and echo) multiple times in a loop? B)

I'm not sure right now whether my patch is up-to-date btw., as the currently attached one doesn't remove redundant slashs and (e.g.) ./... 8/

The main point in the patch is the separator that is redefined to ':'.

Well, I know IFS, but I don't want to run it in a loop; that was my point.

My improved code snippet (simplified and catching additional instances; using a single invocation of sed) looks like this:

$ echo $EXAMPLE; echo; echo ":$EXAMPLE:" | sed \
-e 's|/\+|/|g' \
-e 's|:\(\./\)\+|:|g' \
-e 's|\(/\.\)\+:|:|g' \
-e 's|\(:\.\)\+:|:|g' \
-e 's|:\+|:|g' -e 's|^:\+||' -e 's|:\+$||' 
.::./././//./://bingo//:baz:::://///:./.:.

/bingo/:baz:/

This is important because this catches all the bad cases like .., ./., ., etc.

We only have to catch "." (i.e., the current working directory) here.

(In relation to #11914, I tried to cd to devel/sage/sage and ran sage as SAGE_PATH=.. sage -n and it failed to run. So we should definitely weed out at least .. by itself.)

IMHO not. If you put things into SAGE_PATH, you have to live with the results (or side-effects) in that case. Or we have to do that in sage-sage, before we (try to) start the notebook only, independently of the fix to sage-env at #11914 (and of course sage-spkg, which is patched here).

A better approach in the case of the notebook would perhaps be to directly test whether any component of PYTHONPATH yields devel/sage-*/ (or has a subdirectory sage with an __init__.py file in it), or, probably easiest, prepend the correct directory to PYTHONPATH before running sage-notebook.

(We should better discuss the latter on #11914.)

[leif]

SCRIPTS repo. Improved version of my previous patch. Based on Sage 4.7.2.alpha4.

[leif]

New patch is up.

[ppurka]

New patch looks good to me. I will leave it to people who actually had the problem(s) to report back and change this to "positive review."

Personally, I had no problems in getting Sage compiled and up and running on two different Linux distributions, and with sage-4.7.0 and sage-4.7.2_alpha3.

[ppurka]

Updated the v2 of the file so that it applies to sage-4.8

[ppurka]

Update patch to sage-4.8.

This has positive review from my side. If there is nothing else holding it up, then it should be merged, in my opinion.

[jdemeyer]

Rebased to #11073.

[jdemeyer]

This needs work because "\+" in sed scripts isn't portable. You can replace "X\+" by "XX*", which is equivalent.

[jdemeyer]

Another minor comment: use spaces instead of TABs for indenting.

[ppurka]

Replying to jdemeyer:

This needs work because "\+" in sed scripts isn't portable. You can replace "X\+" by "XX*", which is equivalent.

Is \{1,\} a portable version of \+?

[jdemeyer]

Replying to ppurka:

Is \{1,\} a portable version of \+?

As far as I can tell, yes. But I consider "XX*" to be more readable than "X\{1,\}", especially for short strings X, which is the case here.

[ppurka]

Apply only this to SAGE_ROOT

[ppurka]

Updated the patch with the stated modifications. Please review.

Here is the output in busybox (probably the closest system to POSIX I have around here, but the earlier version also works fine):

~> busybox sh


BusyBox v1.18.4 (Ubuntu 1:1.18.4-2ubuntu2) built-in shell (ash)
Enter 'help' for a list of built-in commands.

~ $ EXAMPLE=".::./././//./://bingo//:baz:::://///:./.:."
~ $ echo :$EXAMPLE: | busybox sed -e 's|//*|/|g' -e 's|:\(\./\)\{1,\}|:|g' -e 's
|\(/\.\)\{1,\}:|:|g' -e 's|\(:\.\)\{1,\}:|:|g' -e 's|::*|:|g' -e 's|^::*||' -e '
s|::*$||'
/bingo/:baz:/