Sage: Ticket #9960: require SAGE_CHECK to be "yes"
https://trac.sagemath.org/ticket/9960
<p>
Right now, setting SAGE_CHECK to any nonempty string (e.g., "no") runs the test suite. The documentation actually says that SAGE_CHECK should be "yes" for this to happen. Fix this.
</p>
<p>
While we're at it, fix something else: in the script SAGE_ROOT/local/bin/sage-env, SAGE64 is required to be "yes", "no", or unset:
</p>
<pre class="wiki">if [ "$SAGE64" != "yes" -a "$SAGE64" != "no" ]; then
echo "The environment variable SAGE64 (=$SAGE64) must be either unset, yes or no."
exit 1
fi
</pre><p>
The problem is, whenever sage-env is run, output is redirected to /dev/null, so this error message isn't printed. So for example:
</p>
<pre class="wiki">$ export SAGE64='maybe'
$ sage
$
</pre><p>
Sage fails to run and is completely silent as to why. Fix this, too.
</p>
<hr />
<p>
Also, due to a bug in <code>sage-spkg</code>, successful test suite runs never get logged in <code>spkg/installed/<package-name></code> as they should (or is intended); this is fixed by the reviewer patch.
</p>
<p>
(Note that test suite <strong>failures</strong> cannot be logged in these files as they get deleted on non-successful builds, which [currently] includes successful builds with failing self-tests.)
</p>
<hr />
<p>
Apply only <a class="attachment" href="https://trac.sagemath.org/attachment/ticket/9960/trac_9960-scripts-SAGE_CHECK.v2.patch" title="Attachment 'trac_9960-scripts-SAGE_CHECK.v2.patch' in Ticket #9960">trac_9960-scripts-SAGE_CHECK.v2.patch</a><a class="trac-rawlink" href="https://trac.sagemath.org/raw-attachment/ticket/9960/trac_9960-scripts-SAGE_CHECK.v2.patch" title="Download"></a> (to the scripts repo).
</p>
en-usSagehttps://trac.sagemath.org/chrome/site/logo_sagemath_trac.png
https://trac.sagemath.org/ticket/9960
Trac 1.1.6jhpalmieriTue, 21 Sep 2010 16:41:49 GMTcc set
https://trac.sagemath.org/ticket/9960#comment:1
https://trac.sagemath.org/ticket/9960#comment:1
<ul>
<li><strong>cc</strong>
<em>drkirkby</em> <em>kcrisman</em> added
</li>
</ul>
TicketjhpalmieriTue, 21 Sep 2010 16:41:59 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:2
https://trac.sagemath.org/ticket/9960#comment:2
<ul>
<li><strong>status</strong>
changed from <em>new</em> to <em>needs_review</em>
</li>
</ul>
TicketjhpalmieriTue, 21 Sep 2010 16:46:19 GMTcc changed
https://trac.sagemath.org/ticket/9960#comment:3
https://trac.sagemath.org/ticket/9960#comment:3
<ul>
<li><strong>cc</strong>
<em>leif</em> added
</li>
</ul>
<p>
By the way, if you look at sage-env, it rarely produces any output, so I don't see any reason to redirect it to /dev/null. The lines
</p>
<pre class="wiki">. $SAGE_ROOT/local/bin/sage-env 1>/dev/null 2>/dev/null
if [ $? -ne 0 ]; then
echo "Error setting environment variables by running $SAGE_ROOT/local/bin/sage-env; possibly contact sage-devel (see http://groups.google.com/group/sage-devel)."
fi
</pre><p>
from the sage-sage script don't actually seem to print the warning message; I'm not sure why. But it's clearer if it says
</p>
<pre class="wiki">$ export SAGE64=dlkjdf
$ sage
The environment variable SAGE64 (=dlkjdf) must be either unset, yes or no.
</pre>
TicketleifTue, 21 Sep 2010 17:01:08 GMT
https://trac.sagemath.org/ticket/9960#comment:4
https://trac.sagemath.org/ticket/9960#comment:4
<p>
I commented on this <em>somewhere</em> where I uploaded the "first aid" patch for spaces in <code>SAGE_ROOT</code>.
</p>
<p>
Redirecting the output of <code>sage-env</code> is really bad, and the test of <code>$?</code> is almost useless, since <code>sage-env</code> is (and must be) sourced, so it must not <code>exit</code> [some value].
</p>
<p>
The <code>SAGE_CHECK</code> issue (interpreting everything except "no" as "yes") is AFAIK a historical relict; some spkgs might still run the test suite from <code>spkg-install</code> if <code>SAGE_CHECK</code> is set and set to anything but "no".
</p>
TicketleifTue, 21 Sep 2010 17:16:28 GMT
https://trac.sagemath.org/ticket/9960#comment:5
https://trac.sagemath.org/ticket/9960#comment:5
<p>
Replacing every occurrence of <code>exit</code> in <code>sage-env</code> by <code>return</code> should work (then testing <code>$?</code> makes sense); I wonder why Sage is started even if <code>sage-env</code> failed though. (I think <em>there</em> an <code>exit 1</code> is missing.)
</p>
<p>
And we should quote all occurrences of <code>SAGE_ROOT</code>...
</p>
TicketleifTue, 21 Sep 2010 17:22:43 GMT
https://trac.sagemath.org/ticket/9960#comment:6
https://trac.sagemath.org/ticket/9960#comment:6
<p>
Any idea why <code>sage-build</code> doesn't <strong>source</strong> <code>sage-env</code>?!?
</p>
TicketleifTue, 21 Sep 2010 17:31:17 GMT
https://trac.sagemath.org/ticket/9960#comment:7
https://trac.sagemath.org/ticket/9960#comment:7
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:4" title="Comment 4">leif</a>:
</p>
<blockquote class="citation">
<p>
I commented on this <em>somewhere</em> where I uploaded the "first aid" patch for spaces in <code>SAGE_ROOT</code>.
</p>
</blockquote>
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a> (There' also a patch, including one to <code>sage-sage</code>.)
</p>
TicketjhpalmieriTue, 21 Sep 2010 19:04:43 GMT
https://trac.sagemath.org/ticket/9960#comment:8
https://trac.sagemath.org/ticket/9960#comment:8
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:4" title="Comment 4">leif</a>:
</p>
<blockquote class="citation">
<p>
Redirecting the output of <code>sage-env</code> is really bad,
</p>
</blockquote>
<p>
I agree.
</p>
<blockquote class="citation">
<p>
and the test of <code>$?</code> is almost useless, since <code>sage-env</code> is (and must be) sourced, so it must not <code>exit</code> [some value].
</p>
</blockquote>
<p>
Okay.
</p>
<blockquote class="citation">
<p>
The <code>SAGE_CHECK</code> issue (interpreting everything except "no" as "yes") is AFAIK a historical relict; some spkgs might still run the test suite from <code>spkg-install</code> if <code>SAGE_CHECK</code> is set and set to anything but "no".
</p>
</blockquote>
<p>
I ran "grep" after unpacking all of the spkg's, and found SAGE_CHECK used in cliquer and mpir. In both cases, it checks whether it's "yes". (In the case of cliquer, this is done in spkg-install, but there is no spkg-check. So it does the right thing -- running the test suite once if SAGE_CHECK is "yes", just not in the right way. In the case of mpir, if SAGE_CHECK is yes, then the test suite gets run twice, once because of the main sage-spkg script, and once more in mpir's spkg-install script. I thought there was a ticket for this, but I can't find it right now.)
</p>
<blockquote class="citation">
<p>
Replacing every occurrence of exit in sage-env by return should work (then testing $? makes sense); I wonder why Sage is started even if sage-env failed though. (I think there an exit 1 is missing.)
</p>
</blockquote>
<p>
When do you see Sage starting if sage-env fails?
</p>
<p>
I'll also take a look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>. If I can give that a positive review, I'll change the patch here to make it depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>. (I may do that anyway...) As far as changing sage-build to source sage-env, could that break things? The other changes here are pretty inocuous.
</p>
TicketjhpalmieriTue, 21 Sep 2010 19:05:55 GMT
https://trac.sagemath.org/ticket/9960#comment:9
https://trac.sagemath.org/ticket/9960#comment:9
<p>
Ah, the mpir issue is covered by <a class="closed ticket" href="https://trac.sagemath.org/ticket/9522" title="defect: MPIR: Don't check SAGE_CHECK in spkg-install (closed: fixed)">#9522</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/8664" title="enhancement: Upgrade Sage's MPIR spkg to version 2.1.3 (closed: fixed)">#8664</a>.
</p>
TicketleifTue, 21 Sep 2010 19:34:43 GMT
https://trac.sagemath.org/ticket/9960#comment:10
https://trac.sagemath.org/ticket/9960#comment:10
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:8" title="Comment 8">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:4" title="Comment 4">leif</a>:
</p>
<blockquote class="citation">
<p>
The <code>SAGE_CHECK</code> issue (interpreting everything except "no" as "yes") is AFAIK a historical relict; some spkgs might still run the test suite from <code>spkg-install</code> if <code>SAGE_CHECK</code> is set and set to anything but "no".
</p>
</blockquote>
<p>
I ran "grep" after unpacking all of the spkg's, and found SAGE_CHECK used in cliquer and mpir. In both cases, it checks whether it's "yes".
</p>
</blockquote>
<p>
Ok. Maybe we already corrected the others.
</p>
<blockquote class="citation">
<blockquote class="citation">
<p>
Replacing every occurrence of exit in sage-env by return should work (then testing $? makes sense); I wonder why Sage is started even if sage-env failed though. (I think there an exit 1 is missing.)
</p>
</blockquote>
<p>
When do you see Sage starting if sage-env fails?
</p>
</blockquote>
<p>
My bad. I think this happened <em>after</em> I had replaced <code>exit</code> by <code>return</code> in <code>sage-env</code> (screen buffer):
</p>
<pre class="wiki">Error setting environment variables by running /home/leif/Sage/sage-4.6.alpha1-final/local/bin/sage-env; possibly contact sage-devel (see http://groups.google.com/group/sage-devel).
----------------------------------------------------------------------
| Sage Version 4.6.alpha1, Release Date: 2010-09-18 |
| Type notebook() for the GUI, and license() for information. |
----------------------------------------------------------------------
**********************************************************************
* *
* Warning: this is a prerelease version, and it may be unstable. *
* *
**********************************************************************
sage:
</pre><p>
(But the same happens if <code>source</code> itself fails, e.g. because <code>sage-env</code> isn't found, likewise when <code>SAGE_ROOT</code> contains spaces.)
</p>
<p>
But if the error message ever gets printed, Sage shouldn't start. We currently have:
</p>
<div class="wiki-code"><div class="code"><pre>. <span class="nv">$SAGE_ROOT</span>/local/bin/sage-env 1>/dev/null 2>/dev/null
<span class="k">if</span> <span class="o">[</span> <span class="nv">$?</span> -ne 0 <span class="o">]</span>; <span class="k">then
</span><span class="nb">echo</span> <span class="s2">"Error setting environment variables by running $SAGE_ROOT/local/bin/sage-env; possibly cont
act sage-devel (see http://groups.google.com/group/sage-devel)."</span>
<span class="k">fi</span>
...
sage<span class="o">()</span> <span class="o">{</span>
sage_setup
sage-ipython <span class="s2">"$@"</span> -i
<span class="o">}</span>
<span class="k">if</span> <span class="o">[</span> <span class="nv">$# </span>-eq 0 <span class="o">]</span>; <span class="k">then
</span>sage
<span class="nb">exit</span> <span class="nv">$?</span>
<span class="k">fi</span>
</pre></div></div><blockquote class="citation">
<p>
As far as changing sage-build to source sage-env, could that break things? The other changes here are pretty inocuous.
</p>
</blockquote>
<p>
I guess that's just an old flaw; calling it is of little use (will only <em>check</em> some things, but doesn't change the environment at all).
</p>
<p>
So give it a try and see what happens... ;-)
</p>
<p>
(It <em>shouldn't</em><sup>TM</sup> break anything.)
</p>
TicketleifTue, 21 Sep 2010 19:49:22 GMT
https://trac.sagemath.org/ticket/9960#comment:11
https://trac.sagemath.org/ticket/9960#comment:11
<p>
Since <code>sage-build</code> is (or should be) called from <code>sage-sage</code>, we can either source it again or completely omit the (second) call.
</p>
TicketleifTue, 21 Sep 2010 19:57:39 GMT
https://trac.sagemath.org/ticket/9960#comment:12
https://trac.sagemath.org/ticket/9960#comment:12
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:11" title="Comment 11">leif</a>:
</p>
<blockquote class="citation">
<p>
Since <code>sage-build</code> is (or should be) called from <code>sage-sage</code>, we can either source it again or completely omit the (second) call.
</p>
</blockquote>
<p>
<em>Sourcing it</em> / <em>calling it</em> referred to <code>sage-env</code>.
</p>
TicketdrkirkbyTue, 21 Sep 2010 20:01:17 GMT
https://trac.sagemath.org/ticket/9960#comment:13
https://trac.sagemath.org/ticket/9960#comment:13
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:8" title="Comment 8">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
I'll also take a look at <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>. If I can give that a positive review, I'll change the patch here to make it depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>. (I may do that anyway...) As far as changing sage-build to source sage-env, could that break things? The other changes here are pretty inocuous.
</p>
</blockquote>
<p>
I agree.
</p>
<p>
John's changes are safe - the others may be desirable, but introduce extra risk. It would be a shame if this change got bounced out because larger changes were added to the ticket. Leif can always create another ticket to fix the other issues.
</p>
<p>
I'm keen we improve the code in Sage, but I don't think making lots of changes to a package every time someone wants to make a minor change, is a good idea. Leif knows I was pulling my hair out when all I wanted to do was get iconv to build on HP-UX on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a>.
</p>
<p>
One point John, if you do change this patch, there's no need to quote xyes. Since you are absolutely 100% sure that the string xyes has no spaces in it, there's no need to quote it. It's different for a variable like $FOOBAR, where you don't know if it might have a space or not.
</p>
<p>
To my knowledge there's nothing in Sage which checks if SAGE64 is "no". The only value actually tested for is "yes", so it's a bit pointless permitting "no".
</p>
<p>
Dave
</p>
TicketleifTue, 21 Sep 2010 20:13:36 GMT
https://trac.sagemath.org/ticket/9960#comment:14
https://trac.sagemath.org/ticket/9960#comment:14
<p>
There are already other tickets around touching at least some of the patched files, too.
</p>
<p>
I'd say adding the quotes around <code>SAGE_ROOT</code>, adding the <code>exit 1</code> if sourcing failed, and perhaps replacing <code>exit</code> by <code>return</code> in <code>sage-env</code> should be mandatory.
</p>
<p>
If we always postpone fixes, we'll never get far (and if one opens a new ticket for every "minor" thing at all, we'll most probably just increase the number of open tickets, with lots of rebasing being necessary if some of them finally get merged).
</p>
<p>
Just my 2 cents.
</p>
TicketdrkirkbyTue, 21 Sep 2010 20:38:56 GMT
https://trac.sagemath.org/ticket/9960#comment:15
https://trac.sagemath.org/ticket/9960#comment:15
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:14" title="Comment 14">leif</a>:
</p>
<blockquote class="citation">
<p>
There are already other tickets around touching at least some of the patched files, too.
</p>
<p>
I'd say adding the quotes around <code>SAGE_ROOT</code>, adding the <code>exit 1</code> if sourcing failed, and
</p>
</blockquote>
<p>
I won't argue with the <code>SAGE_ROOT</code>. I don't suppose John will. I've not looked at the exit, but that sounds sensible.
</p>
<blockquote class="citation">
<p>
perhaps replacing <code>exit</code> by <code>return</code> in <code>sage-env</code> should be mandatory.
</p>
</blockquote>
<p>
It adds extra risk.
</p>
<blockquote class="citation">
<p>
If we always postpone fixes, we'll never get far (and if one opens a new ticket for every "minor" thing at all, we'll most probably just increase the number of open tickets, with lots of rebasing being necessary if some of them finally get merged).
</p>
<p>
Just my 2 cents.
</p>
</blockquote>
<p>
You will just frustrate people if you expect them to make many changes to fix a minor issue. Tickets will get left. John has kindly picked up on something that's not hurting his work, but he knows is wrong. Make it too difficult, and people will in general just give up and not bother with such tickets any more.
</p>
<p>
There's nothing wrong with having a cleanup ticket, where you can make all the changes you want - for example the Cliquer one <a class="closed ticket" href="https://trac.sagemath.org/ticket/9870" title="defect: Clean up Cliquer's spkg-install (closed: fixed)">#9870</a>.
</p>
<p>
Dave
</p>
TicketleifTue, 21 Sep 2010 20:54:54 GMT
https://trac.sagemath.org/ticket/9960#comment:16
https://trac.sagemath.org/ticket/9960#comment:16
<p>
Replacing <code>exit</code> by <code>return</code> adds no risk if we <strong>do</strong> exit when <code>$? -ne 0</code>.
</p>
<p>
Note that John's initial patch also did more than necessary for the ticket (or announced by its title).
</p>
<p>
So IMHO one should either do just what's necessary to address a specific issue (and not touch other files), or do "the whole", i.e. fix most of the open issues with the files you touch, at least the "trivial" ones that do not require extraordinary testing (e.g. on all platforms).
</p>
<p>
There's e.g. still
</p>
<div class="wiki-code"><div class="code"><pre> <span class="nb">cd</span> <span class="s2">"$SAGE_ROOT/devel/sage/sage"</span>
<span class="nb">echo</span> <span class="s2">"*** TOUCHING ALL CYTHON (.pyx) FILES ***"</span>
touch */*.pyx */*/*.pyx */*/*/*.pyx */*/*/*/*.pyx */*/*/*/*/*.pyx */*/*/*/*/*.pyx */*/*/*/*/*/*.
pyx 2> /dev/null
</pre></div></div><p>
in <code>sage-build</code>, too. (Count the asterisks!)
</p>
TicketjhpalmieriTue, 21 Sep 2010 21:13:44 GMT
https://trac.sagemath.org/ticket/9960#comment:17
https://trac.sagemath.org/ticket/9960#comment:17
<p>
Leif: just to clarify, are you suggesting changing <em>every</em> instance of <code>exit</code> to <code>return</code> in sage-env, or just changing the one relevant to SAGE64?
</p>
<p>
As far as suppressing output from sage-env, it turns out that there is one bad instance: the output from sage-check-64. If this is not suppressed, then you can get behavior like this on a 64-bit OS X machine:
</p>
<pre class="wiki">$ sage -hg status
Building Sage on OS X in 64-bit mode
...
$ sage
Building Sage on OS X in 64-bit mode
...
$ sage -pkg ...
Building Sage on OS X in 64-bit mode
...
</pre><p>
This is confusing since nothing is actually being built. It is also not necessary to print this every time any script in local/bin runs, so right now I'm suppressing output from it in sage-env. Output from it is not suppressed in sage-build. Perhaps the right place to print this message would be somewhere in sage-spkg?
</p>
<p>
I'm attaching a new patch.
</p>
TicketjhpalmieriTue, 21 Sep 2010 21:14:35 GMTattachment set
https://trac.sagemath.org/ticket/9960
https://trac.sagemath.org/ticket/9960
<ul>
<li><strong>attachment</strong>
set to <em>trac_9960-scripts-SAGE_CHECK.patch</em>
</li>
</ul>
<p>
scripts repo: depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>
</p>
TicketjhpalmieriTue, 21 Sep 2010 21:15:59 GMT
https://trac.sagemath.org/ticket/9960#comment:18
https://trac.sagemath.org/ticket/9960#comment:18
<blockquote class="citation">
<p>
One point John, if you do change this patch, there's no need to quote xyes.
</p>
</blockquote>
<p>
Okay. I think it's a little more readable if it's quoted, especially the way my editor colorizes things, so I'm leaving it quoted.
</p>
TicketleifTue, 21 Sep 2010 21:42:27 GMT
https://trac.sagemath.org/ticket/9960#comment:19
https://trac.sagemath.org/ticket/9960#comment:19
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:17" title="Comment 17">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
Leif: just to clarify, are you suggesting changing <em>every</em> instance of <code>exit</code> to <code>return</code> in sage-env, or just changing the one relevant to SAGE64?
</p>
</blockquote>
<p>
Yes, every. If you exit in a sourced file, the sourcing shell exits.
</p>
<p>
(We should then test the return code when/whereever <code>sage-env</code> is sourced.)
</p>
<blockquote class="citation">
<p>
As far as suppressing output from sage-env, it turns out that there is one bad instance: the output from sage-check-64. If this is not suppressed, then you can get behavior like this on a 64-bit OS X machine:
</p>
</blockquote>
<pre class="wiki">$ sage -hg status
Building Sage on OS X in 64-bit mode
...
$ sage
Building Sage on OS X in 64-bit mode
...
$ sage -pkg ...
Building Sage on OS X in 64-bit mode
...
</pre><blockquote class="citation">
<p>
This is confusing since nothing is actually being built.
</p>
</blockquote>
<p>
The <code>sage-check-64</code> script is a bit funny...
</p>
<blockquote class="citation">
<p>
It is also not necessary to print this every time any script in local/bin runs, so right now I'm suppressing output from it in sage-env.
</p>
</blockquote>
<p>
We should echo <em>warning and error messages</em> in <code>sage-env</code> to <code>stderr</code>, then we can redirect <code>stdout</code> ("informative" messages) to <code>/dev/null</code> in the usual case.
</p>
<blockquote class="citation">
<p>
Output from it is not suppressed in sage-build. Perhaps the right place to print this message would be somewhere in sage-spkg?
</p>
</blockquote>
<p>
<code>sage -b</code> doesn't call <code>sage-spkg</code>. We could call <code>sage-check-64</code> there, though its use isn't very clear. I either set <code>SAGE64</code> or leave it; I don't think there needs to be some extra file which sets it if it ever has been set before. Perhaps Dave can explain the use of that.
</p>
TicketleifTue, 21 Sep 2010 22:29:44 GMT
https://trac.sagemath.org/ticket/9960#comment:20
https://trac.sagemath.org/ticket/9960#comment:20
<p>
What about removing <code>sage-check-64</code> from <code>sage-env</code> and directly sourcing it in the scripts that actually (attempt to) build something?
</p>
<p>
Btw, setting <code>SAGE64=no</code> should perhaps delete the "flag" file. But if one started with one setting and switched to another, a <code>make clean</code> or whatever would be required anyway (interpreting <code>SAGE64</code> <em>not set</em> as "use default / previous setting", which seems to be the [undocumented?] current logic).
</p>
TicketdrkirkbyTue, 21 Sep 2010 22:33:43 GMT
https://trac.sagemath.org/ticket/9960#comment:21
https://trac.sagemath.org/ticket/9960#comment:21
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:20" title="Comment 20">leif</a>:
</p>
<blockquote class="citation">
<p>
What about removing <code>sage-check-64</code> from <code>sage-env</code> and directly sourcing it in the scripts that actually (attempt to) build something?
</p>
<p>
Btw, setting <code>SAGE64=no</code> should perhaps delete the "flag" file. But if one started with one setting and switched to another, a <code>make clean</code> or whatever would be required anyway (interpreting <code>SAGE64</code> <em>not set</em> as "use default / previous setting", which seems to be the [undocumented?] current logic).
</p>
</blockquote>
<p>
What about creating another ticket for that?
</p>
TicketdrkirkbyTue, 21 Sep 2010 22:43:28 GMT
https://trac.sagemath.org/ticket/9960#comment:22
https://trac.sagemath.org/ticket/9960#comment:22
<p>
I've given positive review to <a class="closed ticket" href="https://trac.sagemath.org/ticket/9644" title="defect: Add error messages and update documentation for spaces in $SAGE_ROOT (closed: fixed)">#9644</a>, based on
</p>
<ul><li>John was happy with Leif's changes
</li><li>I'm happy with John's changes to the documentation.
</li></ul><p>
I'll look at this ticket when I have more time, as I'm not sure of the effect of changing the exits to returns.
</p>
<p>
I don't know how John feels, but I think we need to stop the ticket dragging on like <a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a> did.
</p>
TicketdrkirkbyTue, 21 Sep 2010 23:00:51 GMT
https://trac.sagemath.org/ticket/9960#comment:23
https://trac.sagemath.org/ticket/9960#comment:23
<p>
It seems to me that by replacing the exits by returns, you are now forcing another file to check the return code. Can anyone explain what we have gained by changing the exits to returns?
</p>
TicketjhpalmieriTue, 21 Sep 2010 23:09:21 GMT
https://trac.sagemath.org/ticket/9960#comment:24
https://trac.sagemath.org/ticket/9960#comment:24
<p>
As far as I understand it, the problem is the code in <a class="ext-link" href="http://trac.sagemath.org/sage_trac/ticket/9960#comment:3"><span class="icon"></span>an earlier comment</a>: if you use "exit", then do
</p>
<pre class="wiki">. sage-env
if [ $? -ne 0 ]; then
...
</pre><p>
the exit code for sage-env won't be available, so the code inside the "if" block is never executed.
</p>
TicketleifTue, 21 Sep 2010 23:23:17 GMT
https://trac.sagemath.org/ticket/9960#comment:25
https://trac.sagemath.org/ticket/9960#comment:25
<p>
Exactly. And you'll never see any error message in case you redirected the output to <code>/dev/null</code>.
</p>
<p>
If we only redirect <code>stdout</code> when sourcing <code>sage-env</code>, and its error messages are printed to <code>stderr</code>, we could leave the <code>exit</code>s in <code>sage-env</code>, but that's in general bad, since the sourcing script might want to do something else or in addition in case of errors.
</p>
TicketdrkirkbyTue, 21 Sep 2010 23:39:57 GMT
https://trac.sagemath.org/ticket/9960#comment:26
https://trac.sagemath.org/ticket/9960#comment:26
<p>
OK. I'll apply both patches later today (it's 0038 here). Then review it. I'm out most of tomorrow, so it will be late UK time before I can look at it.
</p>
<p>
Dave
</p>
TicketdrkirkbyTue, 21 Sep 2010 23:41:26 GMT
https://trac.sagemath.org/ticket/9960#comment:27
https://trac.sagemath.org/ticket/9960#comment:27
<p>
I mean I'm out most of today (given its just gone midnight here).
</p>
<p>
Dave
</p>
TicketleifWed, 22 Sep 2010 02:35:12 GMT
https://trac.sagemath.org/ticket/9960#comment:28
https://trac.sagemath.org/ticket/9960#comment:28
<p>
I've attached a patch to <code>sage-spkg</code> to apply on top of John's:
</p>
<ul><li>Quotes more environment variables.
</li><li>Checks return code of <code>sage-env</code> and exits with an error message if non-zero.
</li><li>Fixes successful test suite runs never getting logged.
</li><li>Corrects one comment, adds some more (including on what could further be fixed on another ticket).
</li></ul><p>
Sorry, just noticed the patch's name is wrong (patches <code>sage-spkg</code>, not <code>sage-sage</code>).
</p>
TicketleifWed, 22 Sep 2010 05:34:40 GMT
https://trac.sagemath.org/ticket/9960#comment:29
https://trac.sagemath.org/ticket/9960#comment:29
<p>
The only side-effect of <em>sourcing</em> <code>sage-env</code> twice is that some variables (e.g. <code>LD_LIBRARY_PATH</code>) get redundant entries.
</p>
<p>
We <em>could</em> in principle prevent such by an
</p>
<div class="wiki-code"><div class="code"><pre><span class="cp">#ifndef FOO
#define FOO
</span><span class="p">...</span>
<span class="cp">#endif
</span></pre></div></div><p>
analogon. But IMHO a minor issue...
</p>
TicketdrkirkbyWed, 22 Sep 2010 21:32:32 GMT
https://trac.sagemath.org/ticket/9960#comment:30
https://trac.sagemath.org/ticket/9960#comment:30
<p>
Leif, it would be less confusing if you just deleted the patch, renamed it, then put it back with the right name. Click on the patch, then in the bottom left you should see "Delte". You should be able to delete your own patches.
</p>
<p>
If you get a failure, then I can delete it for you, as I have admin rights on trac, but I believe you should be able to delete your own patches.
</p>
<p>
Let's not add any more changes to this ticket. Leif's changes look OK to me, though I feel they should have been on another ticket.
</p>
<p>
Dave
</p>
TicketleifWed, 22 Sep 2010 22:59:05 GMT
https://trac.sagemath.org/ticket/9960#comment:31
https://trac.sagemath.org/ticket/9960#comment:31
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:30" title="Comment 30">drkirkby</a>:
</p>
<blockquote class="citation">
<p>
Leif, it would be less confusing if you just deleted the patch, renamed it, then put it back
</p>
</blockquote>
<p>
I would have done so if only I could... :)
</p>
<blockquote class="citation">
<p>
[...] I can delete it for you, as I have admin rights on trac, but I believe you should be able to delete your own patches.
</p>
</blockquote>
<p>
No, there's not even a button to do so, so please delete it for me. (I've replaced the file with the wrong name by an almost empty file and re-uploaded the original with the correct name.)
</p>
TicketdrkirkbyWed, 22 Sep 2010 23:01:34 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:32
https://trac.sagemath.org/ticket/9960#comment:32
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I've deleted the two files for you. I'm changing to "needs work" until such time as you have attached them.
</p>
<p>
Dave
</p>
TicketleifWed, 22 Sep 2010 23:05:01 GMT
https://trac.sagemath.org/ticket/9960#comment:33
https://trac.sagemath.org/ticket/9960#comment:33
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:30" title="Comment 30">drkirkby</a>:
</p>
<blockquote class="citation">
<p>
Let's not add any more changes to this ticket.
</p>
</blockquote>
<p>
I didn't plan that; therefore I've also added comments on what has to get fixed on follow-up tickets.
</p>
<blockquote class="citation">
<p>
Leif's changes look OK to me, though I feel they should have been on another ticket.
</p>
</blockquote>
<p>
Testing the "exit" code of <code>sage-env</code> had become necessary because of the other patch.
</p>
<p>
I'm happy you're happy (or ok) with my changes though. :-)
</p>
TicketleifWed, 22 Sep 2010 23:07:25 GMT
https://trac.sagemath.org/ticket/9960#comment:34
https://trac.sagemath.org/ticket/9960#comment:34
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:32" title="Comment 32">drkirkby</a>:
</p>
<blockquote class="citation">
<p>
I've deleted the two files for you. I'm changing to "needs work" until such time as you have attached them.
</p>
</blockquote>
<p>
Argh, "it" != both...
</p>
<p>
Uploading the patch with the correct name <strong>again</strong>.
</p>
TicketleifWed, 22 Sep 2010 23:09:09 GMTattachment set
https://trac.sagemath.org/ticket/9960
https://trac.sagemath.org/ticket/9960
<ul>
<li><strong>attachment</strong>
set to <em>trac_9960-additional_fixes_to_sage-spkg--scripts-repo.patch</em>
</li>
</ul>
<p>
SCRIPTS REPO. Apply on top of John's patch.
</p>
TicketleifWed, 22 Sep 2010 23:09:43 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:35
https://trac.sagemath.org/ticket/9960#comment:35
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
</ul>
TicketjhpalmieriSat, 06 Nov 2010 22:40:49 GMT
https://trac.sagemath.org/ticket/9960#comment:36
https://trac.sagemath.org/ticket/9960#comment:36
<p>
Can we revive this one? Dave, do you have any time soon to take a look at it?
</p>
TicketdrkirkbySun, 07 Nov 2010 10:44:49 GMT
https://trac.sagemath.org/ticket/9960#comment:37
https://trac.sagemath.org/ticket/9960#comment:37
<p>
I'll try to look at this today - my wife wants me to do some painting, so that has to take priority!
</p>
<p>
The problem I have is that what was a reasonably easy ticket to understand, now has changes for which I don't know the full implication of them. Whilst I can see why exit was replaced with return, I would not be totally surprised if another part of Sage relies on the current behavior.
</p>
TicketleifThu, 30 Dec 2010 15:10:12 GMT
https://trac.sagemath.org/ticket/9960#comment:38
https://trac.sagemath.org/ticket/9960#comment:38
<p>
<strong>Ping.</strong>
</p>
<p>
Haven't tried yet, but I'm pretty sure the patches meanwhile need to be rebased...
</p>
TicketleifThu, 30 Dec 2010 16:02:16 GMTdescription changed
https://trac.sagemath.org/ticket/9960#comment:39
https://trac.sagemath.org/ticket/9960#comment:39
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9960?action=diff&version=39">diff</a>)
</li>
</ul>
TicketleifThu, 30 Dec 2010 16:28:52 GMTstatus changed; work_issues set
https://trac.sagemath.org/ticket/9960#comment:40
https://trac.sagemath.org/ticket/9960#comment:40
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>needs_work</em>
</li>
<li><strong>work_issues</strong>
set to <em>Rebase patches.</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:38" title="Comment 38">leif</a>:
</p>
<blockquote class="citation">
<p>
Haven't tried yet, but I'm pretty sure the patches meanwhile need to be rebased...
</p>
</blockquote>
<p>
Of course:
</p>
<pre class="wiki">applying /home/leif/Sage/patches/trac_9960-scripts-SAGE_CHECK.patch
patching file sage-build
Hunk #1 FAILED at 0
Hunk #2 FAILED at 33
Hunk #3 FAILED at 44
3 out of 3 hunks FAILED -- saving rejects to file sage-build.rej
patching file sage-env
Hunk #4 succeeded at 222 (offset 24 lines).
Hunk #5 succeeded at 281 (offset 24 lines).
patching file sage-sage
Hunk #1 FAILED at 141
1 out of 1 hunks FAILED -- saving rejects to file sage-sage.rej
patching file sage-spkg
Hunk #1 succeeded at 368 (offset 10 lines).
abort: patch failed to apply
</pre><p>
(Sage 4.6.1.rc0)
</p>
TicketleifThu, 30 Dec 2010 17:11:55 GMTattachment set
https://trac.sagemath.org/ticket/9960
https://trac.sagemath.org/ticket/9960
<ul>
<li><strong>attachment</strong>
set to <em>trac_9960-scripts-SAGE_CHECK_rebased_to_4.6.1.rc0.patch</em>
</li>
</ul>
<p>
SCRIPTS REPO. (Just) John's patch rebased to Sage 4.6.1.rc0.
</p>
TicketleifThu, 30 Dec 2010 17:13:38 GMT
https://trac.sagemath.org/ticket/9960#comment:41
https://trac.sagemath.org/ticket/9960#comment:41
<p>
I've attached a rebased version of John's patch; still have to rebase mine.
</p>
TicketleifThu, 30 Dec 2010 17:14:14 GMTwork_issues changed
https://trac.sagemath.org/ticket/9960#comment:42
https://trac.sagemath.org/ticket/9960#comment:42
<ul>
<li><strong>work_issues</strong>
changed from <em>Rebase patches.</em> to <em>Rebase reviewer patch.</em>
</li>
</ul>
TicketleifFri, 31 Dec 2010 03:53:52 GMTattachment set
https://trac.sagemath.org/ticket/9960
https://trac.sagemath.org/ticket/9960
<ul>
<li><strong>attachment</strong>
set to <em>trac_9960-additional_fixes_to_sage-spkg_rebased_to_4.6.1.rc0-scripts-repo.patch</em>
</li>
</ul>
<p>
SCRIPTS REPO. My previous patch rebased to Sage 4.6.1.rc0, with some more changes. Apply on top of John's (rebased one).
</p>
TicketleifFri, 31 Dec 2010 04:18:18 GMTstatus, author changed; reviewer set; work_issues deleted
https://trac.sagemath.org/ticket/9960#comment:43
https://trac.sagemath.org/ticket/9960#comment:43
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>needs_review</em>
</li>
<li><strong>reviewer</strong>
set to <em>Leif Leonhardy</em>
</li>
<li><strong>work_issues</strong>
<em>Rebase reviewer patch.</em> deleted
</li>
<li><strong>author</strong>
changed from <em>John Palmieri</em> to <em>John Palmieri, Leif Leonhardy</em>
</li>
</ul>
<p>
I've now also attached a rebased version of my "reviewer" patch, with some more changes:
</p>
<p>
<em>Besides cosmetic changes (formatting, tabs, some messages), I've only</em>
</p>
<ul><li><em>added lots of comments (including TODOs/notes on future changes),</em>
</li><li><em>quoted all necessary environment variables,</em>
</li><li><em>fixed a bug which caused a successful test suite run never getting
logged (to 'spkg/installed/<package-name>').</em>
</li></ul><p>
<em>So there are (still) lots of things to do, but on another ticket.</em>
</p>
<hr />
<p>
I can of course only review John's changes (which I already did, leading to my reviewer patch, so I'm in principle ok with his changes, although I haven't tested them recently); so someone else has to review mine. (I'm going to test them with 4.6.1.rc0 again though I don't expect new, undesired behavior.)
</p>
<p>
Additional changes (other than fixes of possible mistakes introduced here) should IMHO be made on follow-up tickets; as mentioned in the commit message (and comments in <code>sage-spkg</code>), there are quite a lot things to get fixed or improved.
</p>
TicketleifFri, 31 Dec 2010 05:00:57 GMTcc changed
https://trac.sagemath.org/ticket/9960#comment:44
https://trac.sagemath.org/ticket/9960#comment:44
<ul>
<li><strong>cc</strong>
<em>mpatel</em> <em>jdemeyer</em> added
</li>
</ul>
<p>
P.S.:
</p>
<ul><li>The odd messages from <code>sage-check-64</code> when sourcing <code>sage-env</code> are already addressed elsewhere (<a class="closed ticket" href="https://trac.sagemath.org/ticket/10303" title="defect: clean up sage-check-64 and use of SAGE64 (closed: duplicate)">#10303</a>, which unfortunately had to get unmerged from rc0), so John's patch <em>here</em> intentionally (still) <em>does</em> suppress <em>all</em> messages from the former in <code>sage-env</code>, as a <em>temporary</em> solution.
</li></ul><ul><li>(Not) sourcing <code>sage-env</code> more than once is addressed at <a class="closed ticket" href="https://trac.sagemath.org/ticket/10469" title="defect: Don't (effectively) source sage-env more than once (closed: fixed)">#10469</a>.
</li></ul><ul><li>The only scripts directly sourcing <code>sage-env</code> are <code>sage-sage</code> and <code>sage-spkg</code> (both patched here to check its "exit" code), so the changes turning <code>exit</code> into <code>return</code> there are safe.
</li></ul>
TicketleifFri, 31 Dec 2010 05:40:46 GMT
https://trac.sagemath.org/ticket/9960#comment:45
https://trac.sagemath.org/ticket/9960#comment:45
<p>
<strong>Positive review</strong> from me <strong>for John's patch(es).</strong>
</p>
<hr />
<p>
This ticket should also (temporarily) fix the doctests errors occurring when <code>SAGE64=yes</code>, caused by "<em>Building a 64-bit version of Sage</em>" messages.
</p>
<p>
(I've tested it on Linux only.)
</p>
TicketdrkirkbySat, 05 Feb 2011 13:29:31 GMT
https://trac.sagemath.org/ticket/9960#comment:46
https://trac.sagemath.org/ticket/9960#comment:46
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:22" title="Comment 22">drkirkby</a>:
</p>
<blockquote class="citation">
<p>
I don't know how John feels, but I think we need to stop the ticket dragging on like <a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a> did.
</p>
</blockquote>
<p>
Five months ago I wrote the above comment. I could see that if we were not careful, the ticket would drag on, as more and more changes were requested that had nothing to do solving the problem raised.
</p>
<p>
John's changes did the job and were not risky. Now the ticket has dragged on and on, with endless other changes, but still no positive review.
</p>
<p>
Dave
</p>
TicketjhpalmieriThu, 24 Mar 2011 23:38:35 GMTattachment set
https://trac.sagemath.org/ticket/9960
https://trac.sagemath.org/ticket/9960
<ul>
<li><strong>attachment</strong>
set to <em>trac_9960-scripts-SAGE_CHECK.v2.patch</em>
</li>
</ul>
<p>
replaces all previous patches
</p>
TicketjhpalmieriThu, 24 Mar 2011 23:41:00 GMT
https://trac.sagemath.org/ticket/9960#comment:47
https://trac.sagemath.org/ticket/9960#comment:47
<p>
Here is a new patch, replacing all previous ones. This takes the crucial elements from my patch and Leif's, and omits everything else. I've moved most of the rest of Leif's patch to <a class="closed ticket" href="https://trac.sagemath.org/ticket/11021" title="defect: Fix install_package() library function (closed: fixed)">#11021</a>.
</p>
<p>
Since Leif gave my patch a positive review, and since the new patch consists of mine plus a simple error check (which I'm happy with), I'd like to give this a positive review. Dave, any opinions?
</p>
TicketjhpalmieriThu, 24 Mar 2011 23:44:39 GMTdescription changed
https://trac.sagemath.org/ticket/9960#comment:48
https://trac.sagemath.org/ticket/9960#comment:48
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9960?action=diff&version=48">diff</a>)
</li>
</ul>
TicketiandrusFri, 25 Mar 2011 00:35:30 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:49
https://trac.sagemath.org/ticket/9960#comment:49
<ul>
<li><strong>status</strong>
changed from <em>needs_review</em> to <em>positive_review</em>
</li>
</ul>
<p>
I'm happy that this will obsolete <a class="closed ticket" href="https://trac.sagemath.org/ticket/4029" title="defect: sage-env kills the shell when called from "wrong" directory (closed: duplicate)">#4029</a>. Also the rest of the code looks good. Installing dot2tex with SAGE_CHECK=yes ran the test suite and with SAGE_CHECK=maybe didn't, so I give this a positive review.
</p>
TicketiandrusFri, 25 Mar 2011 01:10:39 GMTreviewer changed
https://trac.sagemath.org/ticket/9960#comment:50
https://trac.sagemath.org/ticket/9960#comment:50
<ul>
<li><strong>reviewer</strong>
changed from <em>Leif Leonhardy</em> to <em>Leif Leonhardy, Ivan Andrus</em>
</li>
</ul>
TicketdrkirkbyFri, 25 Mar 2011 05:45:13 GMTreviewer changed
https://trac.sagemath.org/ticket/9960#comment:51
https://trac.sagemath.org/ticket/9960#comment:51
<ul>
<li><strong>reviewer</strong>
changed from <em>Leif Leonhardy, Ivan Andrus</em> to <em>Leif Leonhardy, Ivan Andrus, David Kirkby</em>
</li>
</ul>
<p>
At last!!!
</p>
<p>
I still maintain some of these changes should have been on another ticket. John's patch 6-months ago fixed the problem. Had it been merged 6 months ago, Sage would have been better for it. I'm not denying the changes since are an improvement, but I think these improvements would have been better on another ticket.
</p>
<p>
Dave
</p>
TicketjhpalmieriFri, 25 Mar 2011 14:44:19 GMT
https://trac.sagemath.org/ticket/9960#comment:52
https://trac.sagemath.org/ticket/9960#comment:52
<p>
Dave, you're probably right, and <a class="closed ticket" href="https://trac.sagemath.org/ticket/4029" title="defect: sage-env kills the shell when called from "wrong" directory (closed: duplicate)">#4029</a> would probably have been the right place. But I think it's done now...
</p>
TicketjdemeyerSun, 27 Mar 2011 10:03:10 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:53
https://trac.sagemath.org/ticket/9960#comment:53
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>needs_work</em>
</li>
</ul>
<p>
I get an error when building sage from scratch (but I'm not entirely sure this patch is the cause):
</p>
<pre class="wiki">Machine:
Linux sage.math.washington.edu 2.6.24-28-server #1 SMP Wed Aug 25 14:46:03 UTC 2010 x86_64 GNU/Linux
Deleting directories from past builds of previous/current versions of sage_scripts-4.7.alpha3
Extracting package /scratch/jdemeyer/merger/sage-4.7.alpha3/spkg/standard/sage_scripts-4.7.alpha3.spkg ...
-rw-r--r-- 1 jdemeyer jdemeyer 990291 2011-03-26 21:12 /scratch/jdemeyer/merger/sage-4.7.alpha3/spkg/standard/sage_scripts-4.7.alpha3.spkg
Finished extraction
****************************************************
Host system
uname -a:
Linux sage.math.washington.edu 2.6.24-28-server #1 SMP Wed Aug 25 14:46:03 UTC 2010 x86_64 GNU/Linux
****************************************************
****************************************************
CC Version
gcc -v
Using built-in specs.
Target: x86_64-linux-gnu
Configured with: ../src/configure -v --enable-languages=c,c++,fortran,objc,obj-c++,treelang --prefix=/usr --enable-shared --with-system-zl
ib --libexecdir=/usr/lib --without-included-gettext --enable-threads=posix --enable-nls --with-gxx-include-dir=/usr/include/c++/4.2 --prog
ram-suffix=-4.2 --enable-clocale=gnu --enable-libstdcxx-debug --enable-objc-gc --enable-mpfr --enable-checking=release --build=x86_64-linu
x-gnu --host=x86_64-linux-gnu --target=x86_64-linux-gnu
Thread model: posix
gcc version 4.2.4 (Ubuntu 4.2.4-1ubuntu4)
****************************************************
real 0m0.044s
user 0m0.020s
sys 0m0.020s
/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/sage-spkg: line 436: syntax error near unexpected token `('
/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/sage-spkg: line 436: ` echo "(cd '`pwd`' && '$SAGE_ROOT/sage' -sh)"'
make[1]: *** [installed/sage_scripts-4.7.alpha3] Error 2
make[1]: Leaving directory `/mnt/usb1/scratch/jdemeyer/merger/sage-4.7.alpha3/spkg'
</pre>
TicketdrkirkbySun, 27 Mar 2011 14:03:38 GMT
https://trac.sagemath.org/ticket/9960#comment:54
https://trac.sagemath.org/ticket/9960#comment:54
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:53" title="Comment 53">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
I get an error when building sage from scratch (but I'm not entirely sure this patch is the cause):
</p>
</blockquote>
<p>
And there was me thinking a very small patch John submitted 6 months ago was finally going to be merged!
</p>
<p>
I think Leif should <strong>seriously</strong> consider the implications of the endless changes he wants on tickets like this one - changes that are not related to the problem the ticket is supposed to address.
</p>
<p>
In many ways I wish all sage developers were as fussy as Leif, as his approach does generally result in improved quality of code. Many of the bugs in Sage would not be there if more people were fussy about code that is written. I cringe at some of the stuff I see written. But Leif's endless requested changes are slowing Sage development to a crawl.
</p>
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a> took a couple of months to get a positive review on what was originally a 3 or 4 line patch that was only applied on AIX.
</p>
<p>
The cliquer package is a complete mess, so I opened a ticket to clean it up (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9870" title="defect: Clean up Cliquer's spkg-install (closed: fixed)">#9870</a>), and another (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9871" title="defect: Change Cliquer compiler flags on Solaris to build without text relocations. (closed: fixed)">#9871</a>) to address an important single issue for a 64-bit Solaris port. The latter took ages, due to unrelated changes requested by Leif. Then Leif decided to take ownership of the cleanup patch (<a class="closed ticket" href="https://trac.sagemath.org/ticket/9870" title="defect: Clean up Cliquer's spkg-install (closed: fixed)">#9870</a>), but has done nothing about it in 7 months, despite a couple of reminders from me.
</p>
<p>
Not only do these endless changes take longer to implement for the author, but it makes review so much more difficult. Whereas I would have been happy to give a positive review to John's original patch, this has become so complex it is a nightmare reviewing it.
</p>
<p>
Dave
</p>
TicketdrkirkbySun, 27 Mar 2011 14:23:17 GMT
https://trac.sagemath.org/ticket/9960#comment:55
https://trac.sagemath.org/ticket/9960#comment:55
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:54" title="Comment 54">drkirkby</a>:
</p>
<blockquote class="citation">
<p>
<a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a> took a couple of months to get a positive review on what was originally a 3 or 4 line patch that was only applied on AIX.
</p>
</blockquote>
<blockquote class="citation">
<p>
Dave
</p>
</blockquote>
<p>
Correction, <a class="closed ticket" href="https://trac.sagemath.org/ticket/9603" title="defect: Build iconv in parallel, install it on HP-UX and make it work properly ... (closed: fixed)">#9603</a> was only applied on HP-UX. The point being a <em>safe</em> change, that was only intended to effect Sage on an unsupported platform became overly complex and dragged on for a long time.
</p>
<p>
IMHO, this sort of thing needs to stop.
</p>
<p>
Dave
</p>
TicketjhpalmieriSun, 27 Mar 2011 15:29:03 GMT
https://trac.sagemath.org/ticket/9960#comment:56
https://trac.sagemath.org/ticket/9960#comment:56
<p>
Jeroen: The line cited by the error
</p>
<pre class="wiki">/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/sage-spkg: line 436: syntax error near unexpected token `('
/scratch/jdemeyer/merger/sage-4.7.alpha3/local/bin/sage-spkg: line 436: ` echo "(cd '`pwd`' && '$SAGE_ROOT/sage' -sh)"'
</pre><p>
is not touched by this patch. Did you run "sage -sdist ..." or just apply the patch to the scripts repo? If you just applied it to the scripts repo, then the version of sage-spkg in the repo will not match with the version in spkg/base/, and I think that could cause an error like the one you saw.
</p>
<p>
I ran "sage -sdist ..." and then started building from the new tar file, and it's going fine: it's built around 50 of the spkgs, including sage_scripts, with no problems.
</p>
TicketjhpalmieriMon, 28 Mar 2011 18:48:42 GMT
https://trac.sagemath.org/ticket/9960#comment:57
https://trac.sagemath.org/ticket/9960#comment:57
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:53" title="Comment 53">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
I get an error when building sage from scratch (but I'm not entirely sure this patch is the cause)
</p>
</blockquote>
<p>
My build completed just fine. I just tried again: I took a vanilla copy of 4.7.alpha2, applied this patch, ran "sage -sdist ...", and then built from the resulting tar file. It has started just fine, successfully installing sage_scripts and 40 more spkgs. The build is still ongoing, but I'm not seeing the error you saw.
</p>
<p>
Since you're not sure that this patch caused it, what can I do to help resolve this?
</p>
TicketjdemeyerMon, 28 Mar 2011 19:30:46 GMT
https://trac.sagemath.org/ticket/9960#comment:58
https://trac.sagemath.org/ticket/9960#comment:58
<p>
Probably your original comment about <code>spkg/base</code> was correct. But I will have a look at this later.
</p>
<p>
If you <em>really</em> want to move forward, make sure that no files appear in duplicate places (like <code>local/bin</code> and <code>spkg/base</code> in this case). This has caused trouble for me tons of times. <a class="closed ticket" href="https://trac.sagemath.org/ticket/9433" title="enhancement: Put more files under revision control. (closed: fixed)">#9433</a> fixed a lot of these issues, but apparently there is still some work to do. We should probably get rid of the <code>spkg/base</code> repository and use the <code>SAGE_ROOT</code> repository for this instead.
</p>
TicketjhpalmieriMon, 28 Mar 2011 19:53:44 GMT
https://trac.sagemath.org/ticket/9960#comment:59
https://trac.sagemath.org/ticket/9960#comment:59
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:58" title="Comment 58">jdemeyer</a>:
</p>
<blockquote class="citation">
<p>
If you <em>really</em> want to move forward, make sure that no files appear in duplicate places (like <code>local/bin</code> and <code>spkg/base</code> in this case). This has caused trouble for me tons of times. <a class="closed ticket" href="https://trac.sagemath.org/ticket/9433" title="enhancement: Put more files under revision control. (closed: fixed)">#9433</a> fixed a lot of these issues, but apparently there is still some work to do. We should probably get rid of the <code>spkg/base</code> repository and use the <code>SAGE_ROOT</code> repository for this instead.
</p>
</blockquote>
<p>
I don't know how to avoid the duplication, but see <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a> for a ticket.
</p>
TicketjdemeyerTue, 12 Apr 2011 13:15:14 GMTdescription, milestone changed
https://trac.sagemath.org/ticket/9960#comment:60
https://trac.sagemath.org/ticket/9960#comment:60
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9960?action=diff&version=60">diff</a>)
</li>
<li><strong>milestone</strong>
changed from <em>sage-4.7</em> to <em>sage-4.7.1</em>
</li>
</ul>
TicketjhpalmieriTue, 12 Apr 2011 16:54:26 GMT
https://trac.sagemath.org/ticket/9960#comment:61
https://trac.sagemath.org/ticket/9960#comment:61
<p>
I'm not sure why this depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a>; can you clarify? (I have the same question for <a class="closed ticket" href="https://trac.sagemath.org/ticket/11008" title="defect: spkg test suite successes are not correctly written to the appropriate ... (closed: fixed)">#11008</a>.)
</p>
TicketjdemeyerWed, 13 Apr 2011 07:32:28 GMTstatus changed
https://trac.sagemath.org/ticket/9960#comment:62
https://trac.sagemath.org/ticket/9960#comment:62
<ul>
<li><strong>status</strong>
changed from <em>needs_work</em> to <em>positive_review</em>
</li>
</ul>
<p>
Replying to <a class="ticket" href="https://trac.sagemath.org/ticket/9960#comment:61" title="Comment 61">jhpalmieri</a>:
</p>
<blockquote class="citation">
<p>
I'm not sure why this depends on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a>; can you clarify?
</p>
</blockquote>
<p>
Technically speaking, it doesn't depend on <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a>. But I would really like <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a> to be finished before merging any ticket which affects files in <code>spkg/base</code>. If you insist, I could try to merge <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11008" title="defect: spkg test suite successes are not correctly written to the appropriate ... (closed: fixed)">#11008</a> anyway (before <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a>).
</p>
TicketjhpalmieriWed, 13 Apr 2011 16:16:30 GMTdescription changed
https://trac.sagemath.org/ticket/9960#comment:63
https://trac.sagemath.org/ticket/9960#comment:63
<ul>
<li><strong>description</strong>
modified (<a href="/ticket/9960?action=diff&version=63">diff</a>)
</li>
</ul>
<p>
I'm concerned that <a class="closed ticket" href="https://trac.sagemath.org/ticket/11073" title="enhancement: remove the spkg/base repo! (closed: fixed)">#11073</a> is going to be complicated: it will touch files in the scripts repo, in the root repo, and (of course) in the base repo. I'm not even sure what the right approach is for that ticket. I'll post some questions there.
</p>
<p>
The fixes in <a class="closed ticket" href="https://trac.sagemath.org/ticket/9960" title="defect: require SAGE_CHECK to be "yes" (closed: fixed)">#9960</a> and <a class="closed ticket" href="https://trac.sagemath.org/ticket/11008" title="defect: spkg test suite successes are not correctly written to the appropriate ... (closed: fixed)">#11008</a> are pretty tame, comparatively. I think you can just apply the patches on those two tickets and then run "sage -sdist" to make a new source distribution; that should move the changed files to spkg/base. So I would <em>prefer</em> that you merge those two. But I'm not going to <em>insist</em>; you're the release manager, so if it makes your job much easier to wait, then wait.
</p>
TicketjdemeyerThu, 12 May 2011 20:27:20 GMTstatus, description changed; resolution, merged set
https://trac.sagemath.org/ticket/9960#comment:64
https://trac.sagemath.org/ticket/9960#comment:64
<ul>
<li><strong>status</strong>
changed from <em>positive_review</em> to <em>closed</em>
</li>
<li><strong>resolution</strong>
set to <em>fixed</em>
</li>
<li><strong>description</strong>
modified (<a href="/ticket/9960?action=diff&version=64">diff</a>)
</li>
<li><strong>merged</strong>
set to <em>sage-4.7.1.alpha1</em>
</li>
</ul>
Ticket