Opened 11 years ago

Closed 11 years ago

#9952 closed defect (fixed)

make SAGE_CHECK work with SAGE_ATLAS_LIB

Reported by: jhpalmieri Owned by: tbd
Priority: minor Milestone: sage-4.6
Component: packages: standard Keywords:
Cc: drkirkby Merged in: sage-4.6.alpha2
Authors: John Palmieri Reviewers: David Kirkby, Leif Leonhardy
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description

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

Attachments (1)

atlas-p16.patch (1.3 KB) - added by jhpalmieri 11 years ago.
for reference only: the diff between the old spkg and the new one

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 11 years ago by leif

  • Status changed from new to needs_work

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

comment:2 Changed 11 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

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.

comment:3 follow-up: Changed 11 years ago by 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).

comment:4 Changed 11 years ago by leif

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

comment:5 Changed 11 years ago by jhpalmieri

Okay, I switched it to the second one.

comment:6 Changed 11 years ago by 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

comment:7 in reply to: ↑ 3 Changed 11 years ago by 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

comment:8 in reply to: ↑ 1 Changed 11 years ago by 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

comment:9 Changed 11 years ago by 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.

Changed 11 years ago by jhpalmieri

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

comment:10 follow-ups: Changed 11 years ago by 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

comment:11 in reply to: ↑ 10 Changed 11 years ago by leif

  • Reviewers set to David Kirkby, Leif Leonhardy
  • Status changed from needs_review to positive_review

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.

comment:12 Changed 11 years ago by leif

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

comment:13 Changed 11 years ago by jhpalmieri

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

comment:14 Changed 11 years ago by leif

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

comment:15 in reply to: ↑ 10 Changed 11 years ago by 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

comment:16 Changed 11 years ago by mpatel

  • Merged in set to sage-4.6.alpha2
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.