Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

spkg/bin/sage: do not change directory #13459

Closed
jdemeyer opened this issue Sep 13, 2012 · 13 comments
Closed

spkg/bin/sage: do not change directory #13459

jdemeyer opened this issue Sep 13, 2012 · 13 comments

Comments

@jdemeyer
Copy link
Contributor

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

Apply:

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

Component: scripts

Author: Jeroen Demeyer

Reviewer: John Palmieri

Merged: sage-5.4.rc0

Issue created by migration from https://trac.sagemath.org/ticket/13459

@jdemeyer

This comment has been minimized.

@jdemeyer jdemeyer changed the title sage_setup() in spkg/bin/sage: do not change directory spkg/bin/sage: do not change directory Sep 13, 2012
@jdemeyer

This comment has been minimized.

@jdemeyer
Copy link
Contributor Author

Attachment: 13459_no_cd.patch.gz

@jdemeyer
Copy link
Contributor Author

comment:3

There is a problem with

import sage

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

@jhpalmieri
Copy link
Member

comment:5

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.

@jdemeyer
Copy link
Contributor Author

comment:6

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.

@jhpalmieri
Copy link
Member

comment:7

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.

@jdemeyer
Copy link
Contributor Author

comment:8

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.

@jdemeyer
Copy link
Contributor Author

Attachment: 13459_no_cd_scripts.patch.gz

@jdemeyer
Copy link
Contributor Author

comment:9

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.

@jhpalmieri
Copy link
Member

Reviewer: John Palmieri

@jhpalmieri
Copy link
Member

comment:10

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

@jdemeyer
Copy link
Contributor Author

Merged: sage-5.4.rc0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants