Opened 9 years ago

Closed 8 years ago

Last modified 7 years ago

#10469 closed defect (fixed)

Don't (effectively) source sage-env more than once

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: 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 jhpalmieri)

This can currently happen because

  • some scripts source it themselves (like e.g. at least sage-spkg, which is necessary since sage-spkg isn't always called through sage-sage, which also sources it),
  • some receipts in the top-level Makefile source it before some other script is called through sage-sage,
  • sage-sage itself or other (e.g. Python) code may run sage ... commands such that sage-sage is then called recursively, again sourcing sage-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).

Attachments (6)

trac_10469.patch (1.5 KB) - added by iandrus 8 years ago.
trac_10469-scripts.patch (2.1 KB) - added by jhpalmieri 8 years ago.
scripts repo; replaces other patch
trac_10469.v3.patch (2.1 KB) - added by kini 8 years ago.
trac_10469.v4.patch (2.1 KB) - added by jdemeyer 8 years ago.
trac_10469.v5[1].patch (2.1 KB) - added by kini 8 years ago.
git format of Jeroen's v4 patch, which tracks the filemode change
trac_10469-sage-repo.patch (1.2 KB) - added by jhpalmieri 8 years ago.
main Sage repo

Download all attachments as: .zip

Change History (31)

comment:1 Changed 9 years ago by leif

Thoughts?

comment:2 Changed 9 years ago by leif

  • Description modified (diff)

Changed 8 years ago by iandrus

comment:3 Changed 8 years ago by iandrus

  • Authors set to Ivan Andrus
  • 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 8 years ago by jhpalmieri

scripts repo; replaces other patch

Changed 8 years ago by kini

comment:4 Changed 8 years ago by kini

  • Description modified (diff)

While we're at it, why don't we just make sage-env non-executable? Patch attached.

comment:5 follow-up: Changed 8 years ago by 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 if sage-env has been sourced SAGE_ROOT should be set correctly.

comment:6 in reply to: ↑ 5 Changed 8 years ago by jhpalmieri

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 if sage-env has been sourced SAGE_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 8 years ago by jhpalmieri

  • Authors changed from Ivan Andrus to 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 8 years ago by iandrus

  • Reviewers set to Ivan Andrus
  • Status changed from needs_review to positive_review

I'm happy to give it a positive review.

Changed 8 years ago by jdemeyer

comment:9 Changed 8 years ago by jdemeyer

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

  • Status changed from needs_work to needs_review

comment:11 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 8 years ago by kini

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 8 years ago by kini

Oh, I'm sorry Jeroen. Somehow I thought you were John Palmieri...

Changed 8 years ago by kini

git format of Jeroen's v4 patch, which tracks the filemode change

comment:14 Changed 8 years ago by kini

  • Description modified (diff)

Added a patch as described and simplified the application instructions so the patchbot can read them more easily (?)

comment:15 Changed 8 years ago by jdemeyer

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

  • Description modified (diff)

comment:17 Changed 8 years ago by jhpalmieri

  • 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.

Changed 8 years ago by jhpalmieri

main Sage repo

comment:18 Changed 8 years ago by jdemeyer

  • Reviewers changed from Ivan Andrus to Ivan Andrus, Jeroen Demeyer

The patch makes sense to me, I will test that it works.

comment:19 Changed 8 years ago by iandrus

  • Cc iandrus added

comment:20 Changed 8 years ago by jhpalmieri

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

  • Milestone changed from sage-4.7 to sage-4.7.1

I promised to test it and I'm actually going to do it for real this time :-)

comment:22 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.1.alpha0
  • Resolution set to fixed
  • Status changed from needs_review to closed

Works for me, on sage.math.washington.edu using /scratch.

comment:23 Changed 8 years ago by jdemeyer

See #11449 for a follow-up (do not make sage-env executable in sage-make_devel_packages.

comment:24 Changed 8 years ago by leif

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

The idea of versioning sage-env and SAGE_ENV_SOURCED is a good idea: I'm going to implement this in #13395.

Note: See TracTickets for help on using tickets.