Opened 10 years ago
Closed 10 years ago
#13459 closed enhancement (fixed)
spkg/bin/sage: do not change directory
Reported by: | jdemeyer | Owned by: | leif |
---|---|---|---|
Priority: | major | Milestone: | sage-5.4 |
Component: | scripts | Keywords: | |
Cc: | Merged in: | sage-5.4.rc0 | |
Authors: | Jeroen Demeyer | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
Remove all unneeded "cd" statements from spkg/bin/sage
.
Apply:
- 13459_no_cd.patch to the SAGE_ROOT repository.
- 13459_no_cd_scripts.patch to the SCRIPTS repository.
Attachments (2)
Change History (13)
comment:1 Changed 10 years ago by
- Description modified (diff)
- Summary changed from sage_setup() in spkg/bin/sage: do not change directory to spkg/bin/sage: do not change directory
comment:2 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
Changed 10 years ago by
comment:3 Changed 10 years ago by
- Status changed from needs_review to needs_work
comment:4 Changed 10 years ago by
- Status changed from needs_work to needs_review
comment:5 follow-up: ↓ 6 Changed 10 years ago by
Why are there still some "cd"s in spkg/bin/sage? At least some of them (sync-build, bdist, upgrade) don't look like they're needed. I don't know about the patchbot.
comment:6 in reply to: ↑ 5 Changed 10 years ago by
Replying to jhpalmieri:
Why are there still some "cd"s in spkg/bin/sage? At least some of them (sync-build, bdist, upgrade) don't look like they're needed. I don't know about the patchbot.
I wanted to err on the safe side. When unsure, I put the cd
statement (or left it). bdist
seems to require the cd
, for the rest I don't know.
comment:7 follow-up: ↓ 8 Changed 10 years ago by
Can you explain the change in sage-ipython? I don't see any lines corresponding to import sage
in the old version. I guess I don't know the mechanics of how sage gets imported when you run sage
from the command line.
comment:8 in reply to: ↑ 7 Changed 10 years ago by
Replying to jhpalmieri:
Can you explain the change in sage-ipython? I don't see any lines corresponding to
import sage
in the old version. I guess I don't know the mechanics of how sage gets imported when you runsage
from the command line.
When running Sage through IPython (but not through plain Python when running a script for example), the current working directory is always added to sys.path
(the empty string is added as zeroth entry, which means the current working directory). So when doing
import sage # or sage.foo.bar....
the current working directory will be checked first for the existence of a "sage" module. If the current working directory happens to be $SAGE_ROOT/devel/sage
, that contains a "sage" subdirectory which Python will happily import. But this is the wrong directory, it should be imported from site-packages
instead. This creates problems for Cython modules, as these exist only in site-packages
.
So we avoid this problem by first fixing sys.path
, then importing Sage so any future imports of Sage submodules will work fine.
Changed 10 years ago by
comment:9 Changed 10 years ago by
I just realized that there is no need to change sys.path
in sage-ipython
, because we simply import sage
before IPython is started. I also added some more comments, needs_review.
comment:10 Changed 10 years ago by
- Reviewers set to John Palmieri
- Status changed from needs_review to positive_review
Okay, this looks pretty good to me. I might prefer some more comments in spkg/bin/sage, for example pointing out that sage-bdist needs to be run from SAGE_ROOT. (I'm not actually sure that it does, actually: if run from somewhere else, it just creates the temporary directories in the current working directory, but maybe it will still work. I guess for the principle of least surprise, we shouldn't change the behavior of building in SAGE_ROOT/tmp/.)
comment:11 Changed 10 years ago by
- Merged in set to sage-5.4.rc0
- Resolution set to fixed
- Status changed from positive_review to closed
There is a problem with
when the current directory is
$SAGE_ROOT/devel/sage
(or more generally, when a directorysage
exists).