make SAGE_CHECK work with SAGE_ATLAS_LIB

[jhpalmieri]

If SAGE_CHECK is set, then the spkg-check file in the ATLAS spkg tries to run the test suite. But if SAGE_ATLAS_LIB is also set, then there is nothing to test, so SAGE_CHECK fails. The new spkg (based on the one from #9780) fixes this by skipping the test suite if SAGE_CHECK is set.

Note that if SAGE_ATLAS_LIB is set badly, then spkg-install fails before spkg-check is ever run, so in spkg-check we just need to see whether SAGE_ATLAS_LIB is not empty.

Get the new spkg here:

​http://sage.math.washington.edu/home/palmieri/SPKG/atlas-3.8.3.p16.spkg

[leif]

I think the $ in the message should be escaped... ;-)

[jhpalmieri]

Oops, you're right. I've fixed it now.

By the way, I didn't know whether to use

if [ "x$SAGE_ATLAS_LIB" != "x" ]; then

or

if [ -n "$SAGE_ATLAS_LIB" ]; then

or whether it mattered. Right now I'm using the first of these.

[leif]

Hmmm, the latter is much nicer (and works with all shells / test programs).

Some dead old are only broken in comparing empty strings with = or !=; -z and -n always work (otherwise wouldn't make sense at all).

[leif]

(... as long as you put the variable to test into quotes.)

[jhpalmieri]

Okay, I switched it to the second one.

[drkirkby]

I agree with Leif, the $ in the message should be skipped. We have refereed to SAGE_ATLAS_LIB before, so I think it's best to refer to it as that and not $SAGE_ATLAS_LIB. But it works fine.

real	0m0.147s
user	0m0.060s
sys	0m0.085s
Successfully installed atlas-3.8.3.p16
Running the test suite.
$SAGE_ATLAS_LIB is set; skipping test suite.
Now cleaning up tmp files.
Making Sage/Python scripts relocatable...
Making script relocatable
Finished installing atlas-3.8.3.p16.spkg
drkirkby@hawk:~/sage-4.6.alpha1$ 

Arguably a nice touch would be to add

echo "SAGE_ATLAS_LIB is set to $SAGE_ATLAS_LIB; skipping test suite."

BTW, one more stupid thing, which is nothing to do with you, but a result of a bad bit of code being copied around everywhere, is there's no need for the semi-colon on the line

echo "SAGE_LOCAL undefined ... exiting";

You might as well remove that at the same time.

Dave

[drkirkby]

Replying to leif:

Hmmm, the latter is much nicer (and works with all shells / test programs).

Some dead old are only broken in comparing empty strings with = or !=; -z and -n always work (otherwise wouldn't make sense at all).

Actually, I'd have to disagree with that. I've been using -z and -n, as I agree it looks cleaner, but this is a quote from the autoconf mailing list:

​http://lists.gnu.org/archive/html/autoconf/2010-09/msg00030.html

says

this is yet another case of @var{string} that looks like an operator, and yet another reason that you should ALWAYS use test x"$val" = x rather than test -z "$val" if you don't know what $val contains.

The autoconf developers have a huge experience in writing code as portable as possible, so I'm going to switch to the the more portable "x$var" = x. IMHO, for questions of portability, the autoconf mailing list is the best place to ask.

You can argue rightly argue it does not matter with bash, which this shell is, but it does matter with some shells. So I would personally choose to use the most portable way, so things I write do not rely on bash, but would work with just about any shell. But it's a matter of style. Let John choose what he wants. I believe in order of decreasing portability, they are:

• if [ "x$var" = x ] ; then
• if [ -z "$var" ] ; then
• if [ "$var" = "" ] ; then

But all work with modern bash shells. But my personal preference is for the first of these, since it would appear to be the most portable.

Dave

[drkirkby]

Replying to leif:

I think the $ in the message should be escaped... ;-)

I realise I was not agreeing with Leif - how unusual!

IMHO, the $ should not be there. Leif was saying it should be escaped, I think it should not be there at all, since throughout we have called the variable SAGE_ATLAS_LIB, now to switch it to $SAGE_ATLAS_LIB seems wrong to me.

Dave

[leif]

I'd say there's a slight difference between test -z ... and [ -z ... ], but I can't confirm that for the example given because my ksh isn't broken like the mentioned Solaris one.

I won't consider this a real portability issue (at least in our case), but a matter of robustness. Furthermore, at that point we can rely on SAGE_ATLAS_LIB being unset or empty, or containing a valid filename IIRC.

W.r.t $, of course $SAGE_ATLAS_LIB refers to the value of the environment variable SAGE_ATLAS_LIB, but the former might be clearer to some users in case one omits "The environment variable ...".

I think Dave's

    echo "SAGE_ATLAS_LIB is set to $SAGE_ATLAS_LIB; skipping test suite."

is also more explicit, though I'd add (escaped) double quotes around the variable's value.

[jhpalmieri]

for reference only: the diff between the old spkg and the new one

[jhpalmieri]

This is an amusingly large amount of discussion for a ticket which will deals with an extremely unusual case. Anyway, I've changed it now. It seems to work for me on sage.math and on t2. Note that setting SAGE_ATLAS_LIB to an empty string causes the build to fail earlier, so we really only need to check whether SAGE_ATLAS_LIB is set. Nonetheless, I'm using

if [ "x$SAGE_ATLAS_LIB" != "x" ]; then

[leif]

Replying to jhpalmieri:

This is an amusingly large amount of discussion [...]

Yes.

Nonetheless, I'm using

if [ "x$SAGE_ATLAS_LIB" != "x" ]; then

Though quite weird in the presence of

if [ "$SAGE_LOCAL" = "" ]; then
   echo "SAGE_LOCAL undefined ... exiting"
   echo "Maybe run 'sage -sh'?"
   exit 1
fi

The "style" of autotools is IMHO targeted at (or demanded by) other purposes. We wouldn't discuss the C "coding style" of e.g. Cython either... ;-)

Readability and maintainability also count.

[leif]

P.S.: If the (unescaped) $ hadn't been there in the first place, the first "comment" (by me) would have been "positive review". :)

[jhpalmieri]

Oh, so it's my fault, is it? ;)

[leif]

Yeah, never raise such issues when both Dave and me are involved! (We have our separate tickets for endless discussions.)

[drkirkby]

Replying to jhpalmieri:

This is an amusingly large amount of discussion for a ticket which will deals with an extremely unusual case.

Yes, it is rather a case of the bike shed problem.

​http://en.wikipedia.org/wiki/Parkinson%27s_Law_of_Triviality

This is the reason I was not willing to write anything for the Sage Developer's Guide about writing shell scripts - note there was a discussion about this matter on sage-devel. Lot's of people have their own ideas, and coming to any sort of agreement is difficult.

Dave