#10469 closed defect (fixed)
Don't (effectively) source sage-env more than once
Reported by: | Leif Leonhardy | Owned by: | Georg S. Weber |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | build | Keywords: | environment variables sage-sage scripts |
Cc: | John Palmieri, Mitesh Patel, Ivan Andrus | Merged in: | sage-4.7.1.alpha0 |
Authors: | Ivan Andrus, John Palmieri, Keshav Kini | Reviewers: | Ivan Andrus, Jeroen Demeyer |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
This can currently happen because
- some scripts source it themselves (like e.g. at least
sage-spkg
, which is necessary sincesage-spkg
isn't always called throughsage-sage
, which also sources it),
- some receipts in the top-level
Makefile
source it before some other script is called throughsage-sage
,
sage-sage
itself or other (e.g. Python) code may runsage ...
commands such thatsage-sage
is then called recursively, again sourcingsage-env
.
To achieve this, we could simply add
# Don't execute the commands below more than once: if [ -z "$SAGE_ENV_SOURCED" ]; then export SAGE_ENV_SOURCED=1 # or "yes", "true" or alike, but see below (versioning) else # Already sourced, nothing to do. return 0 fi
near its top.
We may even put a version number into that variable and execute (perhaps only some of) the commands in sage-env
"again" in case it was modified during running scripts, which could be helpful when upgrading Sage.
Such a variable also allows still generic, but safer tests than the usual [ -z "$SAGE_LOCAL" ]
, since SAGE_LOCAL
being defined doesn't really imply sage-env
was sourced, but we usually intend to ensure that some environment variables (like e.g. CC
or UNAME
) are properly set up, without testing each of these individually.
Effectively sourcing (at least parts of) sage-env
only once also
- avoids redundant entries in
PATH
etc., - avoids special tests if some variable was already defined / modified (like e.g.
SAGE_ORIG_LD_LIBRARY_PATH
), also simplifying other scripts (cf. #10286), - IMHO reduces the risk of unintentional side-effects, and
- speeds up execution.
Also, we should change the permissions of sage-env
to non-executable (it must be sourced, not executed).
- Apply trac_10469.v5[1.patch] to the scripts repo.
- Apply trac_10469-sage-repo.patch to the Sage library repo.
Attachments (6)
Change History (31)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
Description: | modified (diff) |
---|
Changed 12 years ago by
Attachment: | trac_10469.patch added |
---|
comment:3 Changed 12 years ago by
Authors: | → Ivan Andrus |
---|---|
Status: | new → needs_review |
It seems reasonable to me, so I've included a patch which does everything except versioning. This will obsolete #4029.
Changed 12 years ago by
Attachment: | trac_10469-scripts.patch added |
---|
scripts repo; replaces other patch
Changed 12 years ago by
Attachment: | trac_10469.v3.patch added |
---|
comment:4 Changed 12 years ago by
Description: | modified (diff) |
---|
While we're at it, why don't we just make sage-env non-executable? Patch attached.
comment:5 follow-up: 6 Changed 12 years ago by
What is the purpose of checking SAGE_ROOT
but not doing anything with it? Is it just so that it prints a warning? It seems that if sage-env
has been sourced SAGE_ROOT
should be set correctly.
comment:6 Changed 12 years ago by
Replying to iandrus:
What is the purpose of checking
SAGE_ROOT
but not doing anything with it? Is it just so that it prints a warning? It seems that ifsage-env
has been sourcedSAGE_ROOT
should be set correctly.
You're probably right, and if this is always called via a call to sage, this could probably be deleted. As it stands, the changes here look pretty safe to me, but removing that check seems a bit riskier, and something which would require more serious testing.
comment:7 Changed 12 years ago by
Authors: | Ivan Andrus → Ivan Andrus, John Palmieri, Keshav Kini |
---|
I'm happy with all of the changes here, but since I helped to write them, I don't think I should give this a positive review. Anyone else? Ivan or Keshav?
comment:8 Changed 12 years ago by
Reviewers: | → Ivan Andrus |
---|---|
Status: | needs_review → positive_review |
I'm happy to give it a positive review.
Changed 12 years ago by
Attachment: | trac_10469.v4.patch added |
---|
comment:9 Changed 12 years ago by
Status: | positive_review → needs_work |
---|
I don't like the #!/this/script/must/be/sourced
In my v4 patch, I removed that line.
Instead, I think a much better solution is simply to make sage-env
not executable.
comment:10 Changed 12 years ago by
Status: | needs_work → needs_review |
---|
comment:11 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:12 Changed 12 years ago by
John, Mercurial tracks file modes. The diffs it exports (and thus the patches mq exports) do not mention them unless you put "[diff]\ngit = true" in your .hgrc. My patch does this - you can just remove the hashbang line from it.
Generally putting "[diff]\ngit = true" in your .hgrc is a good idea, since it frees Mercurial from having to remain compatible with ancient pre-dvcs diff formats.
comment:13 Changed 12 years ago by
Oh, I'm sorry Jeroen. Somehow I thought you were John Palmieri...
Changed 12 years ago by
Attachment: | trac_10469.v5[1].patch added |
---|
git format of Jeroen's v4 patch, which tracks the filemode change
comment:14 Changed 12 years ago by
Description: | modified (diff) |
---|
Added a patch as described and simplified the application instructions so the patchbot can read them more easily (?)
comment:15 Changed 12 years ago by
Status: | needs_review → needs_work |
---|
Doctest error:
sage -t -force_lib devel/sage/sage/misc/dist.py ********************************************************************** File "/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha2-10469/devel/sage-main/sage/misc/dist.py", line 46: sage: install_scripts(SAGE_TMP) Expected: Checking that Sage has the command 'gap' installed Created script ... Got: Checking that Sage has the command 'gap' installed The command 'gap' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'gp' installed The command 'gp' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'singular' installed The command 'singular' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'maxima' installed The command 'maxima' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'M2' installed The command 'M2' is not available; not adding shortcut <BLANKLINE> Checking that Sage has the command 'kash' installed The command 'kash' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'mwrank' installed The command 'mwrank' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'ipython' installed The command 'ipython' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'hg' installed The command 'hg' is installed outside of Sage; not adding shortcut <BLANKLINE> Checking that Sage has the command 'R' installed The command 'R' is installed outside of Sage; not adding shortcut <BLANKLINE> Finished creating scripts. You need not do this again even if you upgrade or move Sage. The only requirement is that the command 'sage' is in the PATH. **********************************************************************
comment:16 Changed 12 years ago by
Description: | modified (diff) |
---|
comment:17 Changed 12 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_work → needs_review |
Okay, I know what's going on. This is a bug in a different aspect of Sage, but perhaps it can be fixed on this ticket. On sage.math.washington.edu, the directory /scratch
is actually a link, pointing to /mnt/usb1/scratch/
. If you have a copy of Sage in a subdirectory of /scratch, then for reasons I don't understand, sometimes Sage thinks SAGE_ROOT is /scratch/... and sometimes it thinks it is /mnt/usb1/scratch/.... This doctest is failing because when the doctest runs, it thinks SAGE_ROOT is /mnt/usb1/scratch/..., while the output from "which gap" starts with /scratch/..., so the first entry in $PATH is the right directory, but with the wrong name. Maybe when sage-env gets run twice, it manages to use /mnt/... both times.
Here's a patch (to the Sage library) which applies os.path.realpath to both the value of SAGE_ROOT and the output from "which gap" (etc.). This fixes the problem for me.
comment:18 Changed 12 years ago by
Reviewers: | Ivan Andrus → Ivan Andrus, Jeroen Demeyer |
---|
The patch makes sense to me, I will test that it works.
comment:19 Changed 12 years ago by
Cc: | Ivan Andrus added |
---|
comment:20 Changed 12 years ago by
For reviewing this, my opinion is that only trac_10469-sage-repo.patch needs to be reviewed: the other patch has a positive review already. For this Sage repo patch, make sure to test it on sage.math, and in particular, build and test Sage in a subdirectory of the /scratch directory (since /scratch is symbolically linked to another directory). It might be a good idea to build it on some other platforms in directories which are symbolic links, to make sure os.path.realpath(...) is doing the right thing.
comment:21 Changed 12 years ago by
Milestone: | sage-4.7 → sage-4.7.1 |
---|
I promised to test it and I'm actually going to do it for real this time :-)
comment:22 Changed 12 years ago by
Merged in: | → sage-4.7.1.alpha0 |
---|---|
Resolution: | → fixed |
Status: | needs_review → closed |
Works for me, on sage.math.washington.edu
using /scratch
.
comment:23 Changed 12 years ago by
See #11449 for a follow-up (do not make sage-env
executable in sage-make_devel_packages
.
comment:24 Changed 11 years ago by
Some funny side effect, i.e. a bug that only gets really visible with #11021 (substituting almost all instances of build
in sage-spkg
with $BUILD
, which by itself is pretty ok, but also replacing the odd mymkdir()
with mkdir -p
, the latter then finally showing the bug):
leif@portland:~/Sage/sage-4.7.1.alpha4$ ./sage -i flint-1.5.0.p5.spkg Installing flint-1.5.0.p5.spkg Calling sage-spkg on flint-1.5.0.p5.spkg Warning: Attempted to overwrite SAGE_ROOT environment variable mkdir: cannot create directory `': No such file or directory flint-1.5.0.p5 Machine: Linux portland 2.6.28-19-generic #66-Ubuntu SMP Sat Oct 16 18:00:58 UTC 2010 x86_64 GNU/Linux sage: /home/leif/Sage/sage-4.7.1.alpha4/flint-1.5.0.p5.spkg is already installed
The error originates from mkdir -p "$BUILD"
, previously mymkdir "$BUILD"
, which incidentally didn't try to create the "empty" directory, because its test in advance [ ! -d $1 ]
evaluates to false
(although the directory of course does not exist).
The following cd "$BUILD"
becomes a no-op, and most other instances of $BUILD
happen to be .../$BUILD
, so don't raise an error either. Only rm
also fails when trying to remove the temporary files in the wrong directory (still .../build/...
).
Note that previously, after merging this ticket (#10469), the error just didn't show up because during the build sage-spkg
gets called directly, hence fully sourcing sage-env
(such that $BUILD
gets build
), creating the build
directory, which can later be used when otherwise its creation and therefore also references to it would fail.
Long story, for short:
If sage-env
is (effectively) sourced only once, all variables must be exported, since the script first sourcing it can call other scripts that source sage-env
again, but if sage-env
then returns early, the second script doesn't get non-exported variables set.
(At least) BUILD
is one such variable that currently doesn't get exported.
I'll provide a patch to sage-env
at #11021.
comment:25 Changed 10 years ago by
The idea of versioning sage-env
and SAGE_ENV_SOURCED
is a good idea: I'm going to implement this in #13395.
Thoughts?