Sage: Ticket #10469: Don't (effectively) source sage-env more than once
https://trac.sagemath.org/ticket/10469
<p>
This can currently happen because
</p>
<ul><li>some scripts source it themselves (like e.g. at least <code>sage-spkg</code>, which is necessary since <code>sage-spkg</code> isn't always called through <code>sage-sage</code>, which also sources it),
</li></ul><ul><li>some receipts in the top-level <code>Makefile</code> source it before some other script is called through <code>sage-sage</code>,
</li></ul><ul><li><code>sage-sage</code> itself or other (e.g. Python) code may run <code>sage ...</code> commands such that <code>sage-sage</code> is then called recursively, again sourcing <code>sage-env</code>.
</li></ul><p>
To achieve this, we could simply add
</p>
<div class="wiki-code"><div class="code"><pre><span class="c1"># Don't execute the commands below more than once:
</span><span class="k">if</span> <span class="o">[</span> -z <span class="s2">"</span><span class="nv">$SAGE_ENV_SOURCED</span><span class="s2">"</span> <span class="o">]</span><span class="p">;</span> <span class="k">then</span>
<span class="nb">export</span> <span class="nv">SAGE_ENV_SOURCED</span><span class="o">=</span><span class="m">1</span> <span class="c1"># or "yes", "true" or alike, but see below (versioning)
</span><span class="k">else</span>
<span class="c1"># Already sourced, nothing to do.
</span> <span class="k">return</span> <span class="m">0</span>
<span class="k">fi</span>
</pre></div></div><p>
near its top.
</p>
<p>
We <em>may</em> even put a version number into that variable and execute (perhaps only some of) the commands in <code>sage-env</code> "again" in case it was modified during running scripts, which could be helpful when upgrading Sage.
</p>
<hr />
<p>
Such a variable also allows still generic, but safer tests than the usual <code>[ -z "$SAGE_LOCAL" ]</code>, since <code>SAGE_LOCAL</code> being defined doesn't really imply <code>sage-env</code> was sourced, but we usually intend to ensure that some environment variables (like e.g. <code>CC</code> or <code>UNAME</code>) are properly set up, without testing each of these individually.
</p>
<p>
Effectively sourcing (at least parts of) <code>sage-env</code> only once also
</p>
<ul><li>avoids redundant entries in <code>PATH</code> etc.,
</li><li>avoids special tests if some variable was already defined / modified (like e.g. <code>SAGE_ORIG_LD_LIBRARY_PATH</code>), also simplifying other scripts (cf. <a class="closed ticket" href="https://trac.sagemath.org/ticket/10286" title="#10286: defect: sage-native-execute does not unset path etc. (closed: duplicate)">#10286</a>),
</li><li>IMHO reduces the risk of unintentional side-effects, and
</li><li>speeds up execution.
</li></ul><hr />
<p>
Also, we should replace the useless
</p>
<div class="wiki-code"><div class="code"><pre><span class="ch">#!/usr/bin/env bash
</span></pre></div></div><p>
at its top by (e.g.)
</p>
<div class="wiki-code"><div class="code"><pre><span class="ch">#!/this/script/must/be/sourced
</span></pre></div></div><p>
which causes an error if <code>sage-env</code> is called rather than sourced (which makes no sense and therefore <em>is</em> indeed an error), with an "appropriate" error message since such an interpreter certainly does not exist.
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/10469
Trac 1.2Leif LeonhardyMon, 13 Dec 2010 18:26:12 GMT
https://trac.sagemath.org/ticket/10469#comment:1
https://trac.sagemath.org/ticket/10469#comment:1
<p>
Thoughts?
</p>
TicketLeif LeonhardyMon, 13 Dec 2010 18:30:22 GMTdescription changed
https://trac.sagemath.org/ticket/10469#comment:2
https://trac.sagemath.org/ticket/10469#comment:2
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=2">diff</a>)
</li>
</ul>
TicketIvan AndrusThu, 24 Mar 2011 00:06:05 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469.patch</em>
</li>
</ul>
TicketIvan AndrusThu, 24 Mar 2011 00:07:50 GMTstatus changed; author set
https://trac.sagemath.org/ticket/10469#comment:3
https://trac.sagemath.org/ticket/10469#comment:3
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
<li><strong>author</strong>
set to <em>Ivan Andrus</em>
</li>
</ul>
<p>
It seems reasonable to me, so I've included a patch which does everything except versioning. This will obsolete <a class="closed ticket" href="https://trac.sagemath.org/ticket/4029" title="#4029: defect: sage-env kills the shell when called from "wrong" directory (closed: duplicate)">#4029</a>.
</p>
TicketJohn PalmieriThu, 24 Mar 2011 20:58:45 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469-scripts.patch</em>
</li>
</ul>
<p>
scripts repo; replaces other patch
</p>
TicketKeshav KiniFri, 25 Mar 2011 00:30:11 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469.v3.patch</em>
</li>
</ul>
TicketKeshav KiniFri, 25 Mar 2011 00:31:35 GMTdescription changed
https://trac.sagemath.org/ticket/10469#comment:4
https://trac.sagemath.org/ticket/10469#comment:4
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=4">diff</a>)
</li>
</ul>
<p>
While we're at it, why don't we just make sage-env non-executable? Patch attached.
</p>
TicketIvan AndrusFri, 25 Mar 2011 00:46:25 GMT
https://trac.sagemath.org/ticket/10469#comment:5
https://trac.sagemath.org/ticket/10469#comment:5
<p>
What is the purpose of checking <code>SAGE_ROOT</code> but not doing anything with it? Is it just so that it prints a warning? It seems that if <code>sage-env</code> has been sourced <code>SAGE_ROOT</code> should be set correctly.
</p>
TicketJohn PalmieriFri, 25 Mar 2011 01:46:56 GMT
https://trac.sagemath.org/ticket/10469#comment:6
https://trac.sagemath.org/ticket/10469#comment:6
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/10469#comment:5" title="Comment 5">iandrus</a>:
</p>
<blockquote class="citation">
<p>
What is the purpose of checking <code>SAGE_ROOT</code> but not doing anything with it? Is it just so that it prints a warning? It seems that if <code>sage-env</code> has been sourced <code>SAGE_ROOT</code> should be set correctly.
</p>
</blockquote>
<p>
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.
</p>
TicketJohn PalmieriFri, 25 Mar 2011 17:09:34 GMTauthor changed
https://trac.sagemath.org/ticket/10469#comment:7
https://trac.sagemath.org/ticket/10469#comment:7
<ul>
<li><strong>author</strong>
changed from <em>Ivan Andrus</em> to <em>Ivan Andrus, John Palmieri, Keshav Kini</em>
</li>
</ul>
<p>
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?
</p>
TicketIvan AndrusFri, 25 Mar 2011 17:13:00 GMTstatus changed; reviewer set
https://trac.sagemath.org/ticket/10469#comment:8
https://trac.sagemath.org/ticket/10469#comment:8
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Ivan Andrus</em>
</li>
</ul>
<p>
I'm happy to give it a positive review.
</p>
TicketJeroen DemeyerSat, 26 Mar 2011 20:03:32 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469.v4.patch</em>
</li>
</ul>
TicketJeroen DemeyerSat, 26 Mar 2011 20:07:35 GMTstatus changed
https://trac.sagemath.org/ticket/10469#comment:9
https://trac.sagemath.org/ticket/10469#comment:9
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
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 <code>sage-env</code> not executable.
</p>
TicketJeroen DemeyerSat, 26 Mar 2011 20:08:06 GMTstatus changed
https://trac.sagemath.org/ticket/10469#comment:10
https://trac.sagemath.org/ticket/10469#comment:10
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketJeroen DemeyerSat, 26 Mar 2011 20:09:48 GMTdescription changed
https://trac.sagemath.org/ticket/10469#comment:11
https://trac.sagemath.org/ticket/10469#comment:11
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=11">diff</a>)
</li>
</ul>
TicketKeshav KiniSat, 26 Mar 2011 22:09:26 GMT
https://trac.sagemath.org/ticket/10469#comment:12
https://trac.sagemath.org/ticket/10469#comment:12
<p>
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.
</p>
<p>
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.
</p>
TicketKeshav KiniSat, 26 Mar 2011 22:10:08 GMT
https://trac.sagemath.org/ticket/10469#comment:13
https://trac.sagemath.org/ticket/10469#comment:13
<p>
Oh, I'm sorry Jeroen. Somehow I thought you were John Palmieri...
</p>
TicketKeshav KiniSat, 26 Mar 2011 22:27:02 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469.v5[1].patch</em>
</li>
</ul>
<p>
git format of Jeroen's v4 patch, which tracks the filemode change
</p>
TicketKeshav KiniSat, 26 Mar 2011 22:28:56 GMTdescription changed
https://trac.sagemath.org/ticket/10469#comment:14
https://trac.sagemath.org/ticket/10469#comment:14
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=14">diff</a>)
</li>
</ul>
<p>
Added a patch as described and simplified the application instructions so the patchbot can read them more easily (?)
</p>
TicketJeroen DemeyerSun, 27 Mar 2011 20:59:31 GMTstatus changed
https://trac.sagemath.org/ticket/10469#comment:15
https://trac.sagemath.org/ticket/10469#comment:15
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
Doctest error:
</p>
<pre class="wiki">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.
**********************************************************************
</pre>
TicketJeroen DemeyerSun, 27 Mar 2011 21:05:29 GMTdescription changed
https://trac.sagemath.org/ticket/10469#comment:16
https://trac.sagemath.org/ticket/10469#comment:16
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=16">diff</a>)
</li>
</ul>
TicketJohn PalmieriSun, 27 Mar 2011 23:37:08 GMTstatus, description changed
https://trac.sagemath.org/ticket/10469#comment:17
https://trac.sagemath.org/ticket/10469#comment:17
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/10469?action=diff&version=17">diff</a>)
</li>
</ul>
<p>
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 <code>/scratch</code> is actually a link, pointing to <code>/mnt/usb1/scratch/</code>. 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.
</p>
<p>
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.
</p>
TicketJohn PalmieriSun, 27 Mar 2011 23:37:35 GMTattachment set
https://trac.sagemath.org/ticket/10469
https://trac.sagemath.org/ticket/10469
<ul>
<li><strong>attachment</strong>
set to <em>trac_10469-sage-repo.patch</em>
</li>
</ul>
<p>
main Sage repo
</p>
TicketJeroen DemeyerWed, 30 Mar 2011 07:54:41 GMTreviewer changed
https://trac.sagemath.org/ticket/10469#comment:18
https://trac.sagemath.org/ticket/10469#comment:18
<ul>
<li><strong>reviewer</strong>
changed from <em>Ivan Andrus</em> to <em>Ivan Andrus, Jeroen Demeyer</em>
</li>
</ul>
<p>
The patch makes sense to me, I will test that it works.
</p>
TicketIvan AndrusWed, 13 Apr 2011 16:19:40 GMTcc changed
https://trac.sagemath.org/ticket/10469#comment:19
https://trac.sagemath.org/ticket/10469#comment:19
<ul>
<li><strong>cc</strong>
<em>Ivan Andrus</em> added
</li>
</ul>
TicketJohn PalmieriWed, 13 Apr 2011 18:38:56 GMT
https://trac.sagemath.org/ticket/10469#comment:20
https://trac.sagemath.org/ticket/10469#comment:20
<p>
For reviewing this, my opinion is that only <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/10469/trac_10469-sage-repo.patch" title="Attachment 'trac_10469-sage-repo.patch' in Ticket #10469">trac_10469-sage-repo.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/10469/trac_10469-sage-repo.patch" title="Download"></a> 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.
</p>
TicketJeroen DemeyerWed, 13 Apr 2011 18:41:48 GMTmilestone changed
https://trac.sagemath.org/ticket/10469#comment:21
https://trac.sagemath.org/ticket/10469#comment:21
<ul>
<li><strong>milestone</strong>
changed from <em>sage-4.7</em> to <em>sage-4.7.1</em>
</li>
</ul>
<p>
I promised to test it and I'm actually going to do it for real this time :-)
</p>
TicketJeroen DemeyerThu, 14 Apr 2011 07:19:56 GMTstatus changed; resolution, merged set
https://trac.sagemath.org/ticket/10469#comment:22
https://trac.sagemath.org/ticket/10469#comment:22
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.1.alpha0</em>
</li>
</ul>
<p>
Works for me, on <code>sage.math.washington.edu</code> using <code>/scratch</code>.
</p>
TicketJeroen DemeyerWed, 08 Jun 2011 15:27:05 GMT
https://trac.sagemath.org/ticket/10469#comment:23
https://trac.sagemath.org/ticket/10469#comment:23
<p>
See <a class="closed ticket" href="https://trac.sagemath.org/ticket/11449" title="#11449: defect: Do not make sage-env executable (closed: fixed)">#11449</a> for a follow-up (do not make <code>sage-env</code> executable in <code>sage-make_devel_packages</code>.
</p>
TicketLeif LeonhardyThu, 07 Jul 2011 04:01:09 GMT
https://trac.sagemath.org/ticket/10469#comment:24
https://trac.sagemath.org/ticket/10469#comment:24
<p>
Some funny side effect, i.e. a bug that only gets really visible with <a class="closed ticket" href="https://trac.sagemath.org/ticket/11021" title="#11021: defect: Fix install_package() library function (closed: fixed)">#11021</a> (substituting almost all instances of <code>build</code> in <code>sage-spkg</code> with <code>$BUILD</code>, which by itself is pretty ok, but also replacing the odd <code>mymkdir()</code> with <code>mkdir -p</code>, the latter then finally showing the bug):
</p>
<pre class="wiki">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
</pre><p>
The error originates from <code>mkdir -p "$BUILD"</code>, previously <code>mymkdir "$BUILD"</code>, which incidentally didn't try to create the "empty" directory, because its test in advance <code>[ ! -d $1 ]</code> evaluates to <code>false</code> (although the directory of course does not exist).
</p>
<p>
The following <code>cd "$BUILD"</code> becomes a no-op, and most other instances of <code>$BUILD</code> happen to be <code>.../$BUILD</code>, so don't raise an error either. Only <code>rm</code> also fails when trying to remove the temporary files in the wrong directory (still <code>.../build/...</code>).
</p>
<p>
Note that previously, after merging this ticket (<a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="#10469: defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a>), the error just didn't show up because during the build <code>sage-spkg</code> gets called directly, hence fully sourcing <code>sage-env</code> (such that <code>$BUILD</code> gets <code>build</code>), creating the <code>build</code> directory, which can later be used when otherwise its creation and therefore also references to it would fail.
</p>
<p>
Long story, for short:
</p>
<p>
If <code>sage-env</code> is (effectively) sourced only once, all variables must be exported, since the script first sourcing it can call other scripts that source <code>sage-env</code> again, but if <code>sage-env</code> then returns early, the second script doesn't get non-exported variables set.
</p>
<p>
(At least) <code>BUILD</code> is one such variable that currently doesn't get exported.
</p>
<p>
I'll provide a patch to <code>sage-env</code> at <a class="closed ticket" href="https://trac.sagemath.org/ticket/11021" title="#11021: defect: Fix install_package() library function (closed: fixed)">#11021</a>.
</p>
TicketJeroen DemeyerWed, 29 Aug 2012 20:01:42 GMT
https://trac.sagemath.org/ticket/10469#comment:25
https://trac.sagemath.org/ticket/10469#comment:25
<p>
The idea of versioning <code>sage-env</code> and <code>SAGE_ENV_SOURCED</code> is a good idea: I'm going to implement this in <a class="closed ticket" href="https://trac.sagemath.org/ticket/13395" title="#13395: defect: Fix upgrading with GCC (closed: fixed)">#13395</a>.
</p>
Ticket