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 )
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)
Change History (36)
comment:1 Changed 8 years ago by
- Cc leif added
comment:2 Changed 8 years ago by
- Keywords dot PYTHONPATH easy_install easy-install pth sage-spkg setuptools added
comment:3 Changed 8 years ago by
- 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
- 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: ↓ 6 Changed 8 years ago by
- Component changed from notebook to build
comment:6 in reply to: ↑ 5 ; follow-up: ↓ 7 Changed 8 years ago by
- 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
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
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
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: ↓ 10 Changed 7 years ago by
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.
comment:10 in reply to: ↑ 9 Changed 7 years ago by
- 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
- Cc jdemeyer jhpalmieri added
Changed 7 years ago by
New attempt to clean up even nastier paths (including .., ./., etc) (Ignore the other patch)
comment:12 Changed 7 years ago by
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: ↓ 14 Changed 7 years ago by
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: ↓ 15 Changed 7 years ago by
Replying to leif:
And what's the advantage of running
sed
(and twicetest
, andecho
) 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
Replying to ppurka:
Replying to leif:
And what's the advantage of running
sed
(and twicetest
, andecho
) 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
SCRIPTS repo. Improved version of my previous patch. Based on Sage 4.7.2.alpha4.
comment:17 Changed 7 years ago by
- 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
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.
comment:19 Changed 7 years ago by
- 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.
Changed 7 years ago by
comment:20 Changed 7 years ago by
- Dependencies set to #11073
- Description modified (diff)
Rebased to #11073.
comment:21 Changed 7 years ago by
- Merged in set to sage-5.0.beta3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:22 follow-up: ↓ 24 Changed 7 years ago by
- 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
Another minor comment: use spaces instead of TABs for indenting.
comment:24 in reply to: ↑ 22 ; follow-up: ↓ 25 Changed 7 years ago by
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
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.
comment:26 Changed 7 years ago by
- 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
- 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
- Merged in set to sage-5.0.beta6
- Resolution set to fixed
- Status changed from positive_review to closed
From
sage-env
:We shouldn't add
$SAGE_PATH
there if it is empty btw., but the removal of ".
" fromPYTHONPATH
should be performed insage-spkg
I think.(
$SAGE_PATH
is also appended to an overwritten(?!)PYTHONPATH
insage-doctest
.)