Opened 8 years ago

Closed 7 years ago

#10192 closed defect (fixed)

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

Reported by: hivert Owned by: jason, was
Priority: major Milestone: sage-5.0
Component: build Keywords: notebook installation dot PYTHONPATH easy_install easy-install pth sage-spkg setuptools
Cc: leif, mpatel, mhansen, jdemeyer, jhpalmieri Merged in: sage-5.0.beta6
Authors: Leif Leonhardy, Punarbasu Purkayastha Reviewers: Florent Hivert, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: #11073 Stopgaps:

Description (last modified by ppurka)

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.

Attachments (8)

trac_10192-remove_dot_from_PYTHONPATH_in_sage-spkg-scripts_repo.patch (1.4 KB) - added by leif 8 years ago.
Apply to scripts repo. Based on vanilla Sage 4.6.rc0. (Updated version, handles more weird cases, too.)
trac_10192-remove_dot_from_PYTHONPATH_in_sage-spkg.rebased_to_4.7.1.rc2.scripts.patch (1.2 KB) - added by leif 7 years ago.
SCRIPTS repo. Same patch, rebased to Sage 4.7.1.rc2.
trac_10192-remove_dot_and_more_from_PYTHONPATH_in_sage.spkg.2.patch (1.1 KB) - added by ppurka 7 years ago.
New attempt to clean up even nastier paths (including .., ./., etc)
trac_10192-remove_dot_and_more_from_PYTHONPATH_in_sage.spkg.patch (1.2 KB) - added by ppurka 7 years ago.
New attempt to clean up even nastier paths (including .., ./., etc) (Ignore the other patch)
trac_10192-remove_dot_from_PYTHONPATH_in_sage-spkg.v2.scripts.patch (1.4 KB) - added by leif 7 years ago.
SCRIPTS repo. Improved version of my previous patch. Based on Sage 4.7.2.alpha4.
trac_10192-remove_dot_from_PYTHONPATH_in_sage-spkg.v3.scripts.patch (1.4 KB) - added by ppurka 7 years ago.
Updated the v2 of the file so that it applies to sage-4.8
10192-remove_dot_from_PYTHONPATH_in_sage-spkg.patch (1.4 KB) - added by jdemeyer 7 years ago.
10192-remove_dot_from_PYTHONPATH_in_sage-spkg.2.patch (1.4 KB) - added by ppurka 7 years ago.
Apply only this to SAGE_ROOT

Download all attachments as: .zip

Change History (36)

comment:1 Changed 8 years ago by leif

  • Cc leif added

comment:2 Changed 8 years ago by leif

  • Keywords dot PYTHONPATH easy_install easy-install pth sage-spkg setuptools added

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

comment:3 Changed 8 years ago by leif

  • Authors set to Leif Leonhardy
  • Cc mpatel added
  • Status changed from new to needs_review

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

comment:4 Changed 8 years ago by leif

  • Cc mhansen added

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

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

  • Component changed from notebook to build

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by hivert

  • Reviewers set to Florent 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.

comment:7 in reply to: ↑ 6 Changed 8 years ago by 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).

Changed 8 years ago by leif

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

comment:8 Changed 8 years ago by 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...

comment:9 follow-up: Changed 7 years ago by 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.

Changed 7 years ago by leif

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

comment:10 in reply to: ↑ 9 Changed 7 years ago by leif

  • Description modified (diff)

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.

comment:11 Changed 7 years ago by leif

  • Cc jdemeyer jhpalmieri added

Changed 7 years ago by ppurka

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

Changed 7 years ago by ppurka

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

comment:12 Changed 7 years ago by 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.

comment:13 follow-up: Changed 7 years ago by 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/

comment:14 in reply to: ↑ 13 ; follow-up: Changed 7 years ago by 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//[\.\/]}"

comment:15 in reply to: ↑ 14 Changed 7 years ago by 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.)

Changed 7 years ago by leif

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

comment:16 Changed 7 years ago by leif

  • Description modified (diff)

New patch is up.

comment:17 Changed 7 years ago by leif

  • Summary changed from SageNB fails to install when SAGE_PATH is set to '.' to SageNB broken when SAGE_PATH contains flavours of '.' during installation

comment:18 Changed 7 years ago by 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.

Changed 7 years ago by ppurka

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

comment:19 Changed 7 years ago by ppurka

  • Description modified (diff)
  • Status changed from needs_review to positive_review

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.

comment:20 Changed 7 years ago by jdemeyer

  • Dependencies set to #11073
  • Description modified (diff)

Rebased to #11073.

comment:21 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta3
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:22 follow-up: Changed 7 years ago by jdemeyer

  • Merged in sage-5.0.beta3 deleted
  • Resolution fixed deleted
  • Status changed from closed to new

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

comment:23 Changed 7 years ago by jdemeyer

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

comment:24 in reply to: ↑ 22 ; follow-up: Changed 7 years ago by 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 \+?

comment:25 in reply to: ↑ 24 Changed 7 years ago by 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.

Changed 7 years ago by ppurka

Apply only this to SAGE_ROOT

comment:26 Changed 7 years ago by ppurka

  • Description modified (diff)
  • Status changed from new to needs_review

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:/

comment:27 Changed 7 years ago by jdemeyer

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Punarbasu Purkayastha
  • Reviewers changed from Florent Hivert to Florent Hivert, Jeroen Demeyer
  • Status changed from needs_review to positive_review

comment:28 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.0.beta6
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.