Opened 9 years ago

Closed 9 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:

Status badges

Description (last modified by jdemeyer)

Remove all unneeded "cd" statements from spkg/bin/sage.

Apply:

  1. 13459_no_cd.patch to the SAGE_ROOT repository.
  2. 13459_no_cd_scripts.patch to the SCRIPTS repository.

Attachments (2)

13459_no_cd.patch (9.5 KB) - added by jdemeyer 9 years ago.
13459_no_cd_scripts.patch (5.4 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 9 years ago by jdemeyer

  • 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 9 years ago by jdemeyer

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

Changed 9 years ago by jdemeyer

comment:3 Changed 9 years ago by jdemeyer

  • Status changed from needs_review to needs_work

There is a problem with

import sage

when the current directory is $SAGE_ROOT/devel/sage (or more generally, when a directory sage exists).

comment:4 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

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

comment:6 in reply to: ↑ 5 Changed 9 years ago by jdemeyer

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: Changed 9 years ago by 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 run sage from the command line.

comment:8 in reply to: ↑ 7 Changed 9 years ago by jdemeyer

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 run sage 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 9 years ago by jdemeyer

comment:9 Changed 9 years ago by jdemeyer

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 9 years ago by jhpalmieri

  • 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 9 years ago by jdemeyer

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