Opened 12 years ago
Last modified 10 years ago
#10469 closed defect
Don't (effectively) source sage-env more than once — at Version 17
Reported by: | leif | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-4.7.1 |
Component: | build | Keywords: | environment variables sage-sage scripts |
Cc: | jhpalmieri, mpatel, iandrus | Merged in: | |
Authors: | Ivan Andrus, John Palmieri, Keshav Kini | Reviewers: | Ivan Andrus |
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.
Change History (23)
comment:1 Changed 12 years ago by
comment:2 Changed 12 years ago by
- Description modified (diff)
Changed 11 years ago by
comment:3 Changed 11 years ago by
- Status changed from new to needs_review
It seems reasonable to me, so I've included a patch which does everything except versioning. This will obsolete #4029.
Changed 11 years ago by
comment:4 Changed 11 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 11 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 in reply to: ↑ 5 Changed 11 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 11 years ago by
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 11 years ago by
- Reviewers set to Ivan Andrus
- Status changed from needs_review to positive_review
I'm happy to give it a positive review.
Changed 11 years ago by
comment:9 Changed 11 years ago by
- Status changed from positive_review to 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 11 years ago by
- Status changed from needs_work to needs_review
comment:11 Changed 11 years ago by
- Description modified (diff)
comment:12 Changed 11 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 11 years ago by
Oh, I'm sorry Jeroen. Somehow I thought you were John Palmieri...
comment:14 Changed 11 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 11 years ago by
- Status changed from needs_review to 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 11 years ago by
- Description modified (diff)
comment:17 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to 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.
Thoughts?