-
-
Notifications
You must be signed in to change notification settings - Fork 454
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make SAGE_CHECK work with SAGE_ATLAS_LIB #9952
Comments
comment:1
I think the |
comment:2
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
Hmmm, the latter is much nicer (and works with all shells / Some dead old are only broken in comparing empty strings with |
comment:4
(... as long as you put the variable to test into quotes.) |
comment:5
Okay, I switched it to the second one. |
comment:6
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.
Arguably a nice touch would be to add
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
You might as well remove that at the same time. Dave |
comment:7
Replying to @nexttime:
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 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 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:
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
Replying to @nexttime:
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 Dave |
comment:9
I'd say there's a slight difference between 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 W.r.t 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. |
Attachment: atlas-p16.patch.gz for reference only: the diff between the old spkg and the new one |
comment:10
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
Replying to @jhpalmieri:
Yes.
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. |
Reviewer: David Kirkby, Leif Leonhardy |
comment:12
P.S.: If the (unescaped) |
comment:13
Oh, so it's my fault, is it? ;) |
comment:14
Yeah, never raise such issues when both Dave and me are involved! (We have our separate tickets for endless discussions.) |
comment:15
Replying to @jhpalmieri:
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 |
Merged: sage-4.6.alpha2 |
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
CC: @sagetrac-drkirkby
Component: packages: standard
Author: John Palmieri
Reviewer: David Kirkby, Leif Leonhardy
Merged: sage-4.6.alpha2
Issue created by migration from https://trac.sagemath.org/ticket/9952
The text was updated successfully, but these errors were encountered: