Opened 10 years ago
Closed 9 years ago
#10303 closed defect (duplicate)
clean up sage-check-64 and use of SAGE64
Reported by: | jhpalmieri | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-duplicate/invalid/wontfix |
Component: | build | Keywords: | 64 |
Cc: | drkirkby, leif, mpatel | Merged in: | |
Authors: | Reviewers: | John Palmieri, David Kirkby, Leif Leonhardy | |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
There are some problems with SAGE64 and sage-check-64. First, sage-check-64 gets run whenever sage-env gets run, which is way too often. As a consequence, if SAGE64 is set, I get extra messages printed whenever I run anything like "sage -hg":
$ sage -hg status Detected SAGE64 flag Building Sage on OS X in 64-bit mode
This is annoying on its own, but it also produces incorrect information when running "sage -pkg": the extra messages make hg think that there are unchecked in changes. Also, in several files in the scripts repo, comments say that SAGE64 is only for use on OS X or Solaris, but there are no such distinctions in various spkg-install files that I've looked at: if SAGE64 is set, then it tries to build in 64-bit mode, regardless of the platform. In contrast, the file sage-check-64 only prints messages about building 64-bit if on OS X or Solaris. Finally, the script sage-check-64 creates a file SAGE_LOCAL/lib/sage-64.txt if SAGE64 is set to "yes", to record the fact that this is a 64-bit build. This makes sense if SAGE64 is unset, but if the user sets SAGE64 to "no", then this file should be ignored, and in fact deleted.
The attached patch gets rid of the distinction between OS X, Solaris, and everything else. It deletes sage-64.txt if SAGE64 is "no". It also only runs sage-check-64 from sage-spkg (which is what calls spkg-install) and sage-build (which may not be strictly necessary, but it was there already). It no longer runs it from sage-env. Since it gets run by sage-spkg, it is now included in spkg/base.
I've asked some questions about this on sage-devel. If the answers are not what I expect, then this may need to be rethought.
Follow-up ticket: #11077.
Instructions:
- apply trac_10303-scripts-SAGE64.v5.patch to the scripts repo.
- apply trac_10303-root-repo.patch to the root repo.
- apply 10303-add-sage-check-64-dot-hgignore.patch to the spkg/base repo.
This depends on the patch at #9960.
Attachments (9)
Change History (85)
Changed 10 years ago by
comment:1 Changed 10 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 10 years ago by
Here is another choice, the "v2" patch. This one writes either "yes" or "no" to the file sage-64.txt. If SAGE64 is unset, then it gets its value from this file. If it is set and the setting is not compatible with the file, then a warning is printed and the user is asked if they want to continue. This requires modifying the file spkg/install, so that it can run this check before attempting to install any spkgs. (Otherwise, it behaves badly with parallel building.) So to use v2, apply that patch and also patch the file spkg/install with the attached patch.
comment:3 follow-up: ↓ 4 Changed 10 years ago by
Well, not my style... ;-) (which is a bit more compact)
I wonder if the sanity check on SAGE64
's setting should be in sage-env
. (I assume it's still there, not visible in the patch.)
Nobody should call sage-check-64
directly, but therefore I would
- test if
SAGE64
is eitheryes
,no
or empty there (otherwise one could call / source it with e.g.SAGE64=foo
), - make sure that
SAGE_LOCAL
is defined, - prevent running it directly by substituting
by
#!/usr/bin/env bash
(and/or perhaps#!/this/file/should/be/sourced
chmod -x
).
I think we could drop "uname-specific" messages (and it's unlikely that SAGE64
will work on 32-bit hosts).
Note that SAGE64=yes
is (currently) not supported (or doesn't make sense) on anything but MacOS X and [Open]Solaris; this isn't clear from the echo
in sage-env
(which is "commented out" IIRC, but people could read it; also, "Creating SAGE_LOCAL/lib/sage-64.txt since it does not exist." might be equally confusing to people building on e.g. 32-bit Linux).
The name of the variable (and the "flag" file) is odd anyway (it should IMHO be SAGE_FORCE_64BIT_BUILD
), and there's [currently] no (analogous) way to force a 32-bit build on systems that default to 64-bit...
comment:4 in reply to: ↑ 3 ; follow-up: ↓ 5 Changed 10 years ago by
Replying to leif:
Well, not my style... ;-) (which is a bit more compact)
I wonder if the sanity check on
SAGE64
's setting should be insage-env
. (I assume it's still there, not visible in the patch.)
It's still there, but it makes sense to move it to sage-check-64.
Nobody should call
sage-check-64
directly, but therefore I would
- test if
SAGE64
is eitheryes
,no
or empty there (otherwise one could call / source it with e.g.SAGE64=foo
),
- make sure that
SAGE_LOCAL
is defined,
- prevent running it directly by substituting
#!/usr/bin/env bash
by
#!/this/file/should/be/sourced
(and/or perhaps
chmod -x
).
Okay, we can do these.
I think we could drop "uname-specific" messages (and it's unlikely that
SAGE64
will work on 32-bit hosts).Note that
SAGE64=yes
is (currently) not supported (or doesn't make sense) on anything but MacOS X and [Open]Solaris;
If SAGE64 is set to "yes", then the compiler flag "-m64" is added by certain spkgs with no check on the platform. Is it the case this flag has no effect except on OS X and Solaris?
The name of the variable (and the "flag" file) is odd anyway (it should IMHO be
SAGE_FORCE_64BIT_BUILD
), and there's [currently] no (analogous) way to force a 32-bit build on systems that default to 64-bit...
I don't think we can change the name of the variable, but the flag file could be renamed, since it's only used in sage-check-64. Any suggestions?
comment:5 in reply to: ↑ 4 Changed 10 years ago by
Replying to jhpalmieri:
Replying to leif:
I think we could drop "uname-specific" messages (and it's unlikely that
SAGE64
will work on 32-bit hosts).Note that
SAGE64=yes
is (currently) not supported (or doesn't make sense) on anything but MacOS X and [Open]Solaris;If SAGE64 is set to "yes", then the compiler flag "-m64" is added by certain spkgs with no check on the platform. Is it the case this flag has no effect except on OS X and Solaris?
No. I think we should check the platform once, in sage-check-64
. While -m64
is superfluous on systems defaulting to 64-bit builds (with gcc
), it's illegal on typical 32-bit systems.
SAGE64=yes
should simply only be used on 64-bit MacOS X and [Open]Solaris; we could give an error message on 32-bit systems, I think no need to give a warning on 64-bit Linuces.
The name of the variable (and the "flag" file) is odd anyway (it should IMHO be
SAGE_FORCE_64BIT_BUILD
), and there's [currently] no (analogous) way to force a 32-bit build on systems that default to 64-bit...I don't think we can change the name of the variable, but the flag file could be renamed, since it's only used in sage-check-64. Any suggestions?
Well, we could use a new variable name but keep "backwards compatibility".
sage-build-arch.txt
(or -abi
) is perhaps inappropriate for a file containing "yes" or "no"... ;-)
Just omit the actual filename from the message? ("Recording build ABI...") At least if SAGE64=no
?
Changing the contents of the file (e.g. "32"/"64" instead of "no"/"yes") is of course another option, but would currently only complicate the tests. (Would make sense if we introduce SAGE_BUILD_ABI
and set SAGE64
accordingly, or the other way around if SAGE64
is set instead of that. Then we could rename the script to e.g. sage-check-abi
as well...)
comment:6 follow-up: ↓ 7 Changed 10 years ago by
Here's a new version of the "v2" patch. I haven't changed the variable or the file name, but this does check whether SAGE64 is set to anything except "no" on a non-OS X or Solaris machine, and gives an error in that case. I'm checking the return code for sage-check-64 where it's called.
SAGE64=yes should simply only be used on 64-bit MacOS X and [Open]Solaris; we could give an error message on 32-bit systems, I think no need to give a warning on 64-bit Linuces.
I don't know how to check for 32-bit systems in a shell script, so I'm not doing this, just checking for "Darwin" or "SunOS".
comment:7 in reply to: ↑ 6 Changed 10 years ago by
Replying to jhpalmieri:
Here's a new version of the "v2" patch.
Hmmm, now we get "32-bit build, so writing 'no' to SAGE_LOCAL/lib/sage-64.txt." on every non-OS X / non-[Open]Solaris system...
(And if SAGE64
is not set, we usually read the flag file twice.)
SAGE64=yes should simply only be used on 64-bit MacOS X and [Open]Solaris; we could give an error message on 32-bit systems, I think no need to give a warning on 64-bit Linuces.
I don't know how to check for 32-bit systems in a shell script, so I'm not doing this, just checking for "Darwin" or "SunOS".
You could check if ... `uname -m` in i[3456]86|ppc) ...
, Dave will know what in addition to look for on [Open]Solaris (e.g. "x86") and especially SPARC.
(On Darwin PPCs, uname -m
might report more weird "processor names" like "Power Macintosh", but that's IMHO a minor issue since the build would fail later on 32-bit OSs anyway if we pass -m64
. We do not support 64-bit builds on PowerPCs at all, so looking for i[3456]86|ppc*|[Pp]ower*
would be ok, too.)
If you edit sage-check-64
again, you could remove the persistent superfluous semicolon from echo "SAGE_LOCAL undefined ... exiting";
;-) and replace `uname`
by "$UNAME"
.
comment:8 Changed 10 years ago by
P.S.:
What about
... case "$UNAME" in Darwin|SunOS) # Do the right thing... ;; *) if [ -n "$SAGE64" ]; then echo "Warning: Ignoring setting of SAGE64 (=$SAGE64) [since it is only used on MacOS X and Solaris]..." fi # We don't care if there's a sage-64.txt file return 0 esac
?
comment:9 Changed 10 years ago by
Okay, this version of the patch still doesn't check for 32-bit. Doing that right may involve more testing than I have time to do right now. (Is there any authoritative listing of the possible outputs for "uname -m" anywhere, with information about each platform, like whether it's 64-bit or not?) This version does check for the platform; if not OS X or Solaris (or OpenSolaris), if SAGE64 is not "no" or unset, there is an error. If SAGE64 is okay, it just exits immediately and silently.
comment:10 follow-up: ↓ 12 Changed 10 years ago by
Seems you really like negation... ;-) (perhaps because proofs by contradiction are usually easier?)
Looks ok to me, though still a bit complicated.
Giving an (early) error if someone tries to build a 64-bit version on a 32-bit platform is IMHO less important, and there are still ticket numbers available.
Regarding uname
, unfortunately every OS implementor is free to let the uname()
system call return whatever he/she likes (i.e. almost prose, and no official standards); I now recall Solaris returns e.g. "i86pc
", not "i386
" or alike, and the uname
command just returns what it gets from that call.
For the purpose of Sage, the list of possible values shouldn't be that long though, but I'm not aware of a compilation, and just testing uname -m
might not even be sufficient. A good resource are autotools sources (i.e. config.guess
and configure
scripts); our Skynet machines wiki page also contains entries for uname -a
IIRC.
Another, safer way to detect if 64-bit builds are supported is to use getconf
, e.g.
if [ `getconf _POSIX_V6_LP64_OFF64` -eq 1 ]; then # 64-bit builds supported echo "Max. unsigned long: `getconf -v POSIX_V6_LP64_OFF64 ULONG_MAX`" # should give 2^64-1 fi
comment:11 follow-up: ↓ 13 Changed 10 years ago by
Like Peter on sage-devel, I think it's bad to restrict this SAGE64 to OS X and Solaris, as these two systems are certainly not the only ones which default to 32-bit, but can build 64-bit.
comment:12 in reply to: ↑ 10 ; follow-up: ↓ 17 Changed 10 years ago by
Replying to leif:
Another, safer way to detect if 64-bit builds are supported is to use
getconf
, e.g.if [ `getconf _POSIX_V6_LP64_OFF64` -eq 1 ]; then # 64-bit builds supported echo "Max. unsigned long: `getconf -v POSIX_V6_LP64_OFF64 ULONG_MAX`" # should give 2^64-1 fi
That's not true. Read the section on "Exit Status" on the OpenGroup? web site.
Dave
comment:13 in reply to: ↑ 11 ; follow-up: ↓ 14 Changed 10 years ago by
Replying to drkirkby:
Like Peter on sage-devel, I think it's bad to restrict this SAGE64 to OS X and Solaris, as these two systems are certainly not the only ones which default to 32-bit, but can build 64-bit.
I don't have a strong opinion about this, but I lean slightly toward agreeing with you. At least allowing SAGE64=yes on any system maintains the status quo, more or less.
Do you think we should print a warning saying that it might not be fully supported, or just accept SAGE64=yes silently on platforms other than OS X and Solaris?
comment:14 in reply to: ↑ 13 ; follow-up: ↓ 18 Changed 10 years ago by
Replying to jhpalmieri:
Replying to drkirkby:
Like Peter on sage-devel, I think it's bad to restrict this SAGE64 to OS X and Solaris, as these two systems are certainly not the only ones which default to 32-bit, but can build 64-bit.
I don't have a strong opinion about this, but I lean slightly toward agreeing with you. At least allowing SAGE64=yes on any system maintains the status quo, more or less.
It's basically what in the installation guide says:
http://www.sagemath.org/doc/installation/source.html#environment-variables says:
Do you think we should print a warning saying that it might not be fully supported, or just accept SAGE64=yes silently on platforms other than OS X and Solaris?
I don't feel the need to print any message it's unsupported.
Anyone using using something other than Linux, Solaris or OS X gets a message that the system in unsupported. They have to set SAGE_PORT to something non-empty. I don't feel the need to hold their hand any more.
comment:15 follow-up: ↓ 20 Changed 10 years ago by
Okay, here's a "v3" patch which allows the use of SAGE64 on any platform. It also requires replacing spkg/install with the attached version (that's the same for v2 and v3). I'm leaving v2 up here in case there is more debate about which approach is better: to allow SAGE64 on any system or not.
comment:16 Changed 10 years ago by
- Description modified (diff)
comment:17 in reply to: ↑ 12 Changed 10 years ago by
Replying to drkirkby:
Replying to leif:
Another, safer way to detect if 64-bit builds are supported is to use
getconf
, e.g.
if [ `getconf _POSIX_V6_LP64_OFF64` -eq 1 ]; then # 64-bit builds supported echo "Max. unsigned long: `getconf -v POSIX_V6_LP64_OFF64 ULONG_MAX`" # should give 2^64-1 fi
That's not true. Read the section on "Exit Status" on the OpenGroup? web site.
That was just an example. I know one shouldn't test for equality (-eq 1
), but check that the result (output) is something else than "-1"
and "undefined"
, but that's unrelated to its exit status.
But using getconf
is safe and IMHO the right direction.
comment:18 in reply to: ↑ 14 ; follow-up: ↓ 19 Changed 10 years ago by
Replying to drkirkby:
Replying to jhpalmieri:
Replying to drkirkby:
Like Peter on sage-devel, I think it's bad to restrict this SAGE64 to OS X and Solaris, as these two systems are certainly not the only ones which default to 32-bit, but can build 64-bit.
I don't have a strong opinion about this, but I lean slightly toward agreeing with you. At least allowing SAGE64=yes on any system maintains the status quo, more or less.
It's basically what in the installation guide says:
... where operating systems other than MacOS X, Solaris and OpenSolaris means Linux, since we currently don't support any other. And it doesn't make any sense to mess with SAGE64
and flag files on systems not defaulting to 64-bit builds, unless or until we also support the opposite, 32-bit builds on systems that default to 64-bit.
As we support more OSs, it's trivial to add them to sage-check-64
.
Do you think we should print a warning saying that it might not be fully supported, or just accept SAGE64=yes silently on platforms other than OS X and Solaris?
I don't feel the need to print any message it's unsupported.
Because of its bad name, people erroneously set SAGE64
in the past.
Anyone using using something other than Linux, Solaris or OS X gets a message that the system in unsupported.
Which is pretty correct.
They have to set SAGE_PORT to something non-empty. I don't feel the need to hold their hand any more.
Btw, one can configure GCC to default to 32-bit builds on (almost) any system. (And one could configure GCC to default to 64-bit builds on e.g. Solaris as well.)
comment:19 in reply to: ↑ 18 Changed 10 years ago by
Replying to leif:
And it doesn't make any sense to mess with
SAGE64
and flag files on systems not defaulting to 64-bit builds, unless or until we also support the opposite, 32-bit builds on systems that default to 64-bit.
Should of course read "on systems defaulting to 64-bit builds" (or "not defaulting to 32-bit builds").
Do you think we should print a warning saying that it might not be fully supported, or just accept SAGE64=yes silently on platforms other than OS X and Solaris?
I don't feel the need to print any message it's unsupported.
Because of its bad name, people erroneously set
SAGE64
in the past.
Note that there are (still) spkgs around that do more than just adding e.g. -m64
if SAGE64=yes
, i.e. setting it at least currently has other side effects.
So giving an error message on unexpected (and therefore unsupported) situations makes sense to me.
We could suppress such messages if SAGE_PORT
is non-empty.
comment:20 in reply to: ↑ 15 ; follow-up: ↓ 21 Changed 10 years ago by
Replying to jhpalmieri:
Okay, here's a "v3" patch which allows the use of SAGE64 on any platform. It also requires replacing spkg/install with the attached version (that's the same for v2 and v3). I'm leaving v2 up here in case there is more debate about which approach is better: to allow SAGE64 on any system or not.
I don't know if that's intentional, but to me the "v3" is simply a regression.
- The information on
SAGE64
insage-env
is again misleading.
- If
SAGE64
is not set, the message when creatingsage-64.txt
is simply wrong on systems defaulting to 64-bit builds.
If SAGE64
is not set, we still read the flag file twice if it exists (which is the usual case).
comment:21 in reply to: ↑ 20 Changed 10 years ago by
Here's a new version of the v3 patch.
comment:22 follow-up: ↓ 23 Changed 10 years ago by
I'm a bit puzzled. Is the file trac_10303-install, which is said to be for version 2, supposed to work with version 3 too?
I thought I'd create a new standard package
sage-4.6.1.alpha2/spkg/standard/sage_scripts-4.6.1.alpha2.spkg
with the patches applied before running make for the first time. But that does not work, as then I get a failure as
sage-4.6.1.alpha2/local/bin/sage-check-64
does not exist.
If I'm not mistaken, some of the files in the sage-scripts package also need to be copied to $SAGE_ROOT/spkg/base
too, which is rather confusing.They then get copied to $SAGE_ROOT/local/bin
which add further to the confusion.
Am I mistaken?
I was hoping to test out on my laptop, which I just stuck Solaris 11 Express on it. I need to install the developers tools first on this, though a sage binary created on 'hawk' works OK.
Perhaps you can clarify matters. By which time I should have ehough tools on here to build Sage on the laptop and test the patch more thoroughly
BTW, I would avoid using source
and simply replace it with a .
- i.e. just a dot. It is more portable, and will work on shells which don't support source
.
Dave
comment:23 in reply to: ↑ 22 Changed 10 years ago by
Replying to drkirkby:
I'm a bit puzzled. Is the file trac_10303-install, which is said to be for version 2, supposed to work with version 3 too?
Yes. See the instructions in the ticket description.
I thought I'd create a new standard package...
I think it's best to take a working version of Sage, apply the patch to the scripts repo (by cd'ing to SAGE_ROOT/local/bin and applying it), and then run "sage -sdist ...". (I guess it doesn't have to be a "working version", but enough things have to be installed for "sage -sdist ..." to work.) This will copy the appropriate files, which now include sage-check-64, to spkg/base.
BTW, I would avoid using
source
and simply replace it with a.
- i.e. just a dot. It is more portable, and will work on shells which don't supportsource
.
Are there any differences between . and source? That is, if I make this change, does it require other changes (do I still use "return" rather than "exit" in sage-check-64, for instance)?
comment:24 Changed 10 years ago by
By the way:
$ csh (...local/bin) [4:51pm]>> . sage-check-64 .: Permission denied. (...local/bin) [4:51pm]>> source sage-check-64 SAGE_LOCAL: Undefined variable.
So .
doesn't work, and source
doesn't behave the way I want. In bash:
$ source sage-check-64 SAGE_LOCAL undefined ... exiting Do not call sage-check-64 directly!
comment:25 Changed 10 years ago by
To my knowledge, source
and dot are the same for Bourne type shells, which includes /bin/sh
and /bin/bash
as well as other shells like the Korn shell.
However csh and tcsh are different animals. It not really a good idea to use them for scripts - see
http://www.faqs.org/faqs/unix-faq/shell/csh-whynot/
For interactive use, csh/tcsh have their own syntax which is quite different from sh/bash/ksh etc.
Assuming you have forced your scripts to use bash, then using source
is acceptable. But I think it's better to get into a habit of using more portable constructs, avoiding bashisms.
There were (perhaps still are) some error messages generated in Sage when building on Solaris due to the use of source
. They might have all gone now though, if a shell now uses bash rather than /bin/sh.
Note using bash for scripts is in general not a great idea, as its size makes it slow. That's why some Linux distributions like Ubunta & Debian link /bin/sh to /bin/dash and use the lighter-weight dash shell. But if you use #!/bin/sh at the top, you need to be more careful about what you write if you want it to be portable.
If you source a file, no matter how you do it, then if it exits you will still be back in script you sourced it from. So what you see is to be expected.
If the script is only called from one place, you might consider just writing it inline, and not sourcing the file at all.
Dave
comment:26 Changed 10 years ago by
John, do you want to make any changes? If not, I'm happy to give this a positive review now.
It is a definite improvement over the current system. It works as expected, from a fresh build on my laptop, which runs Solaris 11 Express.
Dave
comment:27 follow-ups: ↓ 30 ↓ 32 Changed 10 years ago by
- Reviewers set to David Kirkby, Leif Leonhardy
- Status changed from needs_review to positive_review
echo "WARNING: At least some of the following information is outdated." echo "See the Sage installation guide for more up-to-date information" echo "about environment variables related to Sage."
So still needs work? ;-) Funny I never noticed the enviroment[sic]s.
Looks better, though I still don't like messing around with SAGE64
and the flag file at all on systems that don't use / require it. I personally would make a distinction on the OS and, to not annoy some developers, SAGE_PORT
(i.e., only take care of SAGE64
on MacOS X 10.4 and 10.5, Solaris and OpenSolaris, or if SAGE_PORT
is non-empty. Allowing SAGE64=ignore
would be another option, perhaps automatically setting this on other platforms.)
Does the Sage Installation Guide mention how to "cure" an installation which erroneously defaults to SAGE64=no
because the user forgot to set it in the first place?
[Want to get rid of this ticket, so positive review. I assume Dave agrees.]
comment:28 Changed 10 years ago by
P.S.:
#!/this/file/must/be/sourced flagfile=... # whatever its name / location is # Set SAGE64 to "ignore" on everything but MacOS X / [Open]Solaris unless SAGE_PORT is set: test -z "$SAGE_PORT" && case "$UNAME" in Darwin|SunOS) # Add other operating systems later if appropriate. # Do nothing here. ;; *) export SAGE64=ignore esac test -z "$SAGE64" && export SAGE64=no # default to no if not set case "$SAGE64" in yes|no) # Also make sure SAGE_LOCAL is defined if it is used; here or above. if [ -f "$flagfile" ]; then saved64=`cat "$flagfile"` if [ "$saved64" != "$SAGE64" ]; then # inconsistent -> error fi else echo $SAGE64 > "$flagfile" fi ;; ignore) # do nothing ;; *) # invalid value -> error esac # And no messages, only errors to stderr
A bit shorter, isn't it?
comment:29 Changed 10 years ago by
Perhaps
test -z "$SAGE64" && export SAGE64=no # default to no if not set
should be
test -z "$SAGE64" && export SAGE64=`cat "$flagfile" || echo no`
comment:30 in reply to: ↑ 27 Changed 10 years ago by
comment:31 Changed 10 years ago by
- Description modified (diff)
comment:32 in reply to: ↑ 27 ; follow-up: ↓ 33 Changed 10 years ago by
Replying to leif:
echo "WARNING: At least some of the following information is outdated." echo "See the Sage installation guide for more up-to-date information" echo "about environment variables related to Sage."
So still needs work? ;-)
Probably so, but not on this ticket :p Does it work to set $AR or $RANLIB or many of these other variables? We should probably delete all of this and just update the information in the installation guide. (There are also other typos in this passage of the code, but it doesn't seem worth spending more time on.)
Funny I never noticed the enviroment[sic]s.
Me neither, until the last patch.
Looks better, though I still don't like messing around with
SAGE64
and the flag file at all on systems that don't use / require it. I personally would make a distinction on the OS and, to not annoy some developers,SAGE_PORT
(i.e., only take care ofSAGE64
on MacOS X 10.4 and 10.5, Solaris and OpenSolaris, or ifSAGE_PORT
is non-empty. AllowingSAGE64=ignore
would be another option, perhaps automatically setting this on other platforms.)
I didn't see any support on sage-devel to change the status quo which allows the use of SAGE64 on any system, so I think for now, doing more documenting of what's going on is the right choice.
Does the Sage Installation Guide mention how to "cure" an installation which erroneously defaults to
SAGE64=no
because the user forgot to set it in the first place?
I doubt it.
[Want to get rid of this ticket, so positive review. I assume Dave agrees.]
...
A bit shorter, isn't it?
Yes, but you left out many of the comments and messages; for example, your "# inconsistent -> error" replaced 15 lines of my version. Shortening it that way doesn't count. ;)
comment:33 in reply to: ↑ 32 ; follow-up: ↓ 34 Changed 10 years ago by
Replying to jhpalmieri:
Replying to leif:
Looks better, though I still don't like messing around with
SAGE64
and the flag file at all on systems that don't use / require it. I personally would make a distinction on the OS and, to not annoy some developers,SAGE_PORT
(i.e., only take care ofSAGE64
on MacOS X 10.4 and 10.5, Solaris and OpenSolaris, or ifSAGE_PORT
is non-empty. AllowingSAGE64=ignore
would be another option, perhaps automatically setting this on other platforms.)I didn't see any support on sage-devel to change the status quo which allows the use of SAGE64 on any system, so I think for now, doing more documenting of what's going on is the right choice.
Well, the status quo is (or was) to do nothing on any operating system other than MacOS X or SunOS/Solaris. (I.e., SAGE64
was effectively ignored w.r.t. sage-env
/ sage-check-64
. The assumption in spkg-install
s is that a non-empty SAGE64
implies MacOS X or SunOS; we previously had tests like if [ "$SAGE64" = yes -a "$UNAME" = Darwin ]
, until Dave began to port to Solaris, first adding a test for SunOS, then dropping the distinction on $UNAME
, which as a side-effect then slightly changed the meaning or possible interpretations of the spkg-install
files regarded in isolation. Of course removing the OS dependency there makes sense, as long as the overall meaning is kept.)
So this patch introduces quite different behavoir on the other platforms rather than just removing stupid messages (and fixing the previous ill and incomplete logic).
Yes, but you left out many of the comments and messages; for example, your "# inconsistent -> error" replaced 15 lines of my version. Shortening it that way doesn't count. ;)
I moved more detailed documentation to the Installation Guide... ;-)
My version also contains lots of (brief) comments and empty lines, nevertheless fits on half a page, so is easier to read and maintain. (Dave also likes saving a few bytes.) :D
The messages are a matter of taste; perhaps we should add more --verbose
and --quiet
options (and/or corresponding environment variables) to Sage.
comment:34 in reply to: ↑ 33 Changed 10 years ago by
Replying to leif:
Well, the status quo is (or was) to do nothing on any operating system other than MacOS X or SunOS/Solaris. (I.e.,
SAGE64
was effectively ignored w.r.t.sage-env
/sage-check-64
. The assumption inspkg-install
s is that a non-emptySAGE64
implies MacOS X or SunOS; we previously had tests likeif [ "$SAGE64" = yes -a "$UNAME" = Darwin ]
, until Dave began to port to Solaris, first adding a test for SunOS, then dropping the distinction on$UNAME
, which as a side-effect then slightly changed the meaning or possible interpretations of thespkg-install
files regarded in isolation. Of course removing the OS dependency there makes sense, as long as the overall meaning is kept.)So this patch introduces quite different behavoir on the other platforms rather than just removing stupid messages (and fixing the previous ill and incomplete logic).
"Quite different" is not accurate. The only functional difference was that previously on non OS X or Solaris systems, if SAGE64=yes, then the file sage-64.txt would not be created or read. Now it is. Some extra messages are also printed. (Yes, previously sage-check-64 ignored SAGE64 on Linux boxes, but many spkg-install files do not.)
comment:35 follow-up: ↓ 37 Changed 10 years ago by
I'm happy with this The variable name SAGE64
implies absolutely nothing about the operating system, so I don't see any reason it should behave any differently on different operating systems. As John said, nobody on sage-devel objected to this either.
No two people are likely to want to code a script in exactly the same way, or use the exact same messages. One could spend ages worrying about such small things, and not have any significant impact on Sage.
For me personally, I try to make my scripts as portable as possible, so they don't rely on bash. Then I can bit of the scripts on other software projects, were people don't rely on bash.
Dave
comment:36 Changed 10 years ago by
Replying to jhpalmieri:
Replying to leif:
Well, the status quo is (or was) to do nothing on any operating system other than MacOS X or SunOS/Solaris. (I.e.,
SAGE64
was effectively ignored w.r.t.sage-env
/sage-check-64
. The assumption inspkg-install
s is that a non-emptySAGE64
implies MacOS X or SunOS; we previously had tests likeif [ "$SAGE64" = yes -a "$UNAME" = Darwin ]
, until Dave began to port to Solaris, first adding a test for SunOS, then dropping the distinction on$UNAME
, which as a side-effect then slightly changed the meaning or possible interpretations of thespkg-install
files regarded in isolation. Of course removing the OS dependency there makes sense, as long as the overall meaning is kept.)So this patch introduces quite different behavoir on the other platforms rather than just removing stupid messages (and fixing the previous ill and incomplete logic).
"Quite different" is not accurate. The only functional difference was that previously on non OS X or Solaris systems, if SAGE64=yes, then the file sage-64.txt would not be created or read. Now it is.
And exactly that is the "problem", besides interpreting SAGE64
not set as "use previous setting", which wasn't recorded on e.g. Linux.
If a user now erroneously sets SAGE64=yes
once, potential problems persist, where previously doing unset SAGE64
(or simply leaving / changing the shell) was sufficient. This has to be documented.
(Though it's [at least meanwhile] simply wrong what some spkgs still do when SAGE64
is set [to "yes"], namely e.g. also modifying flags unrelated to building 32- or 64-bit. We've already removed lots of wrong Building a 32-bit version of ... messages.)
IMHO setting SAGE64
(to "yes" or anything else then "no", and perhaps in the future "ignore") on e.g. Linux should raise an error, nothing else, since the behavior is - at least currently - more or less "undefined" in that case.
Some extra messages are also printed.
I won't mind that.
(Yes, previously sage-check-64 ignored SAGE64 on Linux boxes, but many spkg-install files do not.)
Because SAGE64
is not expected to be set on platforms not defaulting to 32-bit builds; this should be catched in a central place, where also new OSs would and easily could be added in the first place.
comment:37 in reply to: ↑ 35 Changed 10 years ago by
Replying to drkirkby:
The variable name
SAGE64
implies absolutely nothing about the operating system,
Hence it should be named SAGE_FORCE_64_BIT_BUILD
.
Note that it used to, and you're right in the sense that its documentation has now (actually recently) changed, but that doesn't really reflect the current situation in Sage. (It will hopefully in the future, but currently it's a "should" rather than "is", for developers rather than users.)
so I don't see any reason it should behave any differently on different operating systems.
Its only (intended) use is to force 64-bit builds (on operating systems that default to 32-bit, and therefore doesn't make sense on others, while it should in principle do no harm there either, or more precisely it shouldn't have any effect there).
As John said, nobody on sage-devel objected to this either.
I haven't followed that thread, but in general that doesn't mean much to me. How many people did read the thread? Two people besides you two posted; William just noted that SAGE64
was introduced solely for MacOS X, with no impact on anything else. (Of course that wasn't a very good decision, since lots of code had to be changed for Solaris in essentially the same manner again.)
May I cite Peter's statement?
Generically adding '-m64' when SAGE64 is set is definitely wrong because not all compilers will support that flag. In addition, gcc treats x86 and x86_64 as different variants of the one architecture - so gcc on a 32-bit platform can compile x86_64 code. On 32-bit x86 Linux and *BSD, using '-m64' will cause gcc to build x86_64 code - which the kernel won't be able to execute - so this is highly undesirable.
Ideally, all skpg files should inherit {C,CPP,CXX,FC,LD}FLAGS from the environment (adding spkg-specific options if required). This would allow SAGE64 to be processed in one spot fairly early on in the build -- adding '-m64' or equivalent to {C,FCC,FC,LD}FLAGS, which is then inherited by all succeeding spkg builds. This would remove a lot of special-casing from the build.
If all spkgs really contained just what John posted,
f [ -z $CFLAG64 ] ; then CFLAG64=-m64 fi if [ "x$SAGE64" = xyes ]; then echo "64 bit build of cddlib" CFLAGS="$CFLAGS $CFLAG64"; export CFLAGS fi
(which definitely isn't the case, so also the thread didn't really address the issue), the situation would be different, and as Peter noted, we could easily move that code snippet into e.g. sage-env
, as well as others by the way.
Other aspects we discussed here weren't discussed there at all.
comment:38 follow-ups: ↓ 39 ↓ 42 Changed 10 years ago by
- Status changed from positive_review to needs_work
With a build from scratch, I get
spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a ../install.log" ./install: line 77: /scratch/jdemeyer/merger/sage-4.6.1.alpha3/local/bin/sage-check-64: No such file or directory make: *** [build] Error 1
comment:39 in reply to: ↑ 38 Changed 10 years ago by
Replying to jdemeyer:
With a build from scratch, I get
spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a ../install.log" ./install: line 77: /scratch/jdemeyer/merger/sage-4.6.1.alpha3/local/bin/sage-check-64: No such file or directory make: *** [build] Error 1
It needs at least some of the files must be copied to $SAGE_ROOT/spkg/base
too. Then they get to $SAGE_LOCAL/bin
.
I can't understand why some files are in two repositories, but that seems to be the case. Some things in $SAGE_ROOT/spkg/base
are also in the sage-scripts package.
comment:40 Changed 10 years ago by
At least the following are both in sage_scripts-4.6.1.alpha2.spkg
and in $SAGE_ROOT/spkg/base
sage-env sage-make_relative sage-spkg
I think sage-check-64
needs to be put into $SAGE_ROOT/spkg/base
and any of the above files which get modified by Johns patch.
comment:41 Changed 10 years ago by
- Status changed from needs_work to needs_review
To the release manager
I wish I understood the logic of this, but it appears that
sage-env sage-make_relative sage-spkg
in $SAGE_ROOT/spkg/base
are in .hgignore
. I'm adding a patch which adds sage-check-64
to $SAGE_ROOT/spkg/base/.hgignore
. After that is added, the files
sage-env sage-make_relative sage-spkg sage-check-64
should all be copied to $SAGE_ROOT/spkg/base
At least that is my understanding of what's needed.
comment:42 in reply to: ↑ 38 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_review to positive_review
Positive review for Dave's patch.
Replying to jdemeyer:
With a build from scratch, I get
spkg/pipestatus "cd spkg && ./install all 2>&1" "tee -a ../install.log" ./install: line 77: /scratch/jdemeyer/merger/sage-4.6.1.alpha3/local/bin/sage-check-64: No such file or directory make: *** [build] Error 1
How did you apply the patch? After you apply it to the scripts repo, you have to run "sage -sdist" to make sure the appropriate files are copied to SAGE_ROOT/spkg/base.
comment:43 follow-up: ↓ 44 Changed 10 years ago by
Is there any good reason for the placing of these files in the scripts repository, copying them to $SAGE_ROOT/spkg/base
and then having them added to $SAGE_ROOT/spkg/base/.hgignore
? Perhaps I'm missing something, but it would seem more logical to me to:
- Remove the files from the scripts repository
- Put then in the
$SAGE_ROOT/spkg/base
and remove them from the$SAGE_ROOT/spkg/base/.hgignore
Perhaps on another ticket, we address this issue, if there is a more logical way of doing this, as to me at least, this is pretty illogical.
Dave
comment:44 in reply to: ↑ 43 Changed 10 years ago by
Replying to drkirkby:
Is there any good reason for the placing of these files in the scripts repository, copying them to
$SAGE_ROOT/spkg/base
and then having them added to$SAGE_ROOT/spkg/base/.hgignore
?
I don't know. These are scripts that get used both in the installation process and in day-to-day use of Sage (at least when doing things like installing optional skpgs). If you add them to base, then you need to remove them from scripts. One advantage to having them in the scripts repo is that it is an actively updated, well-documented, repo, whereas base is not. Is base even mentioned in the developer's guide?
comment:45 Changed 10 years ago by
Dave: I agree 100% that having files in spkg/base
and in the scripts
repo is a mess. This has caused problems for me countless times. I will now also handle sage-check-64
specially, so it should work.
comment:46 follow-up: ↓ 47 Changed 10 years ago by
- Status changed from positive_review to needs_work
The commit message of trac_10303-scripts-SAGE64.v3.patch should not be one long line. Provide a short description on the first line (this will appear in hg log
). Any further info should go on following lines.
comment:47 in reply to: ↑ 46 Changed 10 years ago by
Replying to jdemeyer:
The commit message of trac_10303-scripts-SAGE64.v3.patch should not be one long line. Provide a short description on the first line (this will appear in
hg log
). Any further info should go on following lines.
I was aware of that, but didn't want to appear to picky... ;-)
comment:48 follow-up: ↓ 49 Changed 10 years ago by
- Status changed from needs_work to positive_review
Seriously? You're holding up the patch because of this? The release manager can easily fix an issue like this in about 2 minutes. Instead, ten hours later, I'm going to post a new patch.
comment:49 in reply to: ↑ 48 ; follow-up: ↓ 50 Changed 10 years ago by
Replying to jhpalmieri:
You're holding up the patch because of this?
No, I'm not.
Of course, the release manager can do it, but I think it is better that I point out these minor issues to the aurhors and that they fix them (does it matter if it takes 10 hours?). Imagine you have 10 tickets with various of these minor issues.
comment:50 in reply to: ↑ 49 ; follow-ups: ↓ 51 ↓ 52 Changed 10 years ago by
Replying to jdemeyer:
Replying to jhpalmieri:
You're holding up the patch because of this?
No, I'm not.
Changing it "needs work" instead of merging it seems like holding it up...
Of course, the release manager can do it, but I think it is better that I point out these minor issues to the aurhors
Sure.
and that they fix them
Why? If it's trivial to fix, why does it matter who does it? If you insist that the authors do it every time, you're adding needless bureaucracy. If anyone notices a problem that's trivial to fix, they should just do it. Sage is not run by the French government: it is not supposed to be governed by rigid bureaucratic rules, but instead by common sense whenever possible. Leif, the same goes for you: if you see a problem like missing line breaks in the commit message, and if it seems clear that other work on the ticket has stopped, why not just fix it? That seems more productive than waiting until someone else (like the release manager) notices it and then commenting "I was aware of that."
(does it matter if it takes 10 hours?).
If it's the 10 hours during which a version is released, yes. But 10 hours is just an example. What if I were on vacation or at a conference with little or no internet access, or swamped with grading or research or other issues, and couldn't get to this for a week or two? This particular ticket is not very important, so if it had missed being included in the upcoming version of Sage, no big deal, but why incur a possible long delay and require the author to fix it?
Imagine you have 10 tickets with various of these minor issues.
I don't have to imagine it, I've been release manager and fixed a number of issues like this (for example, commit messages without the ticket number). I may have then posted to the ticket what the problem was, but I took care of fixing it myself.
comment:51 in reply to: ↑ 50 Changed 10 years ago by
Replying to jhpalmieri:
Leif, the same goes for you: if you see a problem like missing line breaks in the commit message, and if it seems clear that other work on the ticket has stopped, why not just fix it? That seems more productive than waiting until someone else (like the release manager) notices it and then commenting "I was aware of that."
As I said, I considered it a minor issue (and was pretty sure you were also aware of it), so did not even mention it.
Perhaps I should have, but it's IMHO relatively useless to upload another patch just fixing the commit message, since that usually causes more confusion. (I actually did so on other tickets, because I knew the author was busy / unavailable.)
If we had different access rights on trac (e.g. reviewers as "invited" co-authors with the ability to change attachments of others), that would be different.
comment:52 in reply to: ↑ 50 Changed 10 years ago by
Replying to jhpalmieri:
Changing it "needs work" instead of merging it seems like holding it up...
My interpretation is: changing it to "needs_work" is a way of signaling the author, that's all...
comment:53 Changed 10 years ago by
- Merged in set to sage-4.6.1.alpha3
- Resolution set to fixed
- Status changed from positive_review to closed
comment:54 follow-up: ↓ 55 Changed 10 years ago by
Apparently this doesn't work with upgrades, if the flag file already exists.
I think spkg/install
should check if one with the previous semantics (present, but usually empty => SAGE64=yes
) exists and then write the new one, containing "yes" or "no".
sage-env
and sage-check-64
have to be the new versions immediately after this as well.
comment:55 in reply to: ↑ 54 Changed 10 years ago by
Replying to leif:
Apparently this doesn't work with upgrades, if the flag file already exists.
I think
spkg/install
should check if one with the previous semantics (present, but usually empty =>SAGE64=yes
) exists and then write the new one, containing "yes" or "no".
sage-env
andsage-check-64
have to be the new versions immediately after this as well.
See for example http://build.sagemath.org/sage/builders/OSX%2010.6-64%20up%20%28bsd%29/builds/8/steps/shell_2/logs/stdio .
comment:56 Changed 10 years ago by
Trouble with sage -ba
, see #10436.
comment:57 follow-up: ↓ 59 Changed 10 years ago by
I don't think I have any time to work on this right now. If no one else has time either, then the best solution might be to back it out; it was not very high priority to begin with.
comment:58 Changed 10 years ago by
- Cc mpatel added
comment:59 in reply to: ↑ 57 Changed 10 years ago by
Replying to jhpalmieri:
I don't think I have any time to work on this right now. If no one else has time either, then the best solution might be to back it out;
Allright, current plan is to unmerge this in sage-4.6.1.rc0.
comment:60 Changed 10 years ago by
- Merged in sage-4.6.1.alpha3 deleted
- Resolution fixed deleted
- Status changed from closed to new
comment:61 Changed 10 years ago by
- Status changed from new to needs_work
comment:62 Changed 10 years ago by
comment:63 Changed 10 years ago by
- Priority changed from minor to blocker
comment:64 in reply to: ↑ description Changed 10 years ago by
- Description modified (diff)
Replying to jhpalmieri:
There are some problems with SAGE64 and sage-check-64. First, sage-check-64 gets run whenever sage-env gets run, which is way too often. As a consequence, if SAGE64 is set, I get extra messages printed whenever I run anything like "sage -hg":
$ sage -hg status Detected SAGE64 flag Building Sage on OS X in 64-bit modeThis is annoying on its own, but it also produces incorrect information when running "sage -pkg": the extra messages make hg think that there are unchecked in changes.
Also, in several files in the scripts repo, comments say that SAGE64 is only for use on OS X or Solaris, but there are no such distinctions in various spkg-install files that I've looked at: if SAGE64 is set, then it tries to build in 64-bit mode, regardless of the platform.
In contrast, the file sage-check-64 only prints messages about building 64-bit if on OS X or Solaris.
Finally, the script sage-check-64 creates a file SAGE_LOCAL/lib/sage-64.txt if SAGE64 is set to "yes", to record the fact that this is a 64-bit build. This makes sense if SAGE64 is unset, but if the user sets SAGE64 to "no", then this file should be ignored, and in fact deleted.
The attached patch gets rid of the distinction between OS X, Solaris, and everything else. It deletes sage-64.txt if SAGE64 is "no". It also only runs sage-check-64 from sage-spkg (which is what calls spkg-install) and sage-build (which may not be strictly necessary, but it was there already). It no longer runs it from sage-env. Since it gets run by sage-spkg, it is now included in spkg/base.
I've asked some questions about this on sage-devel. If the answers are not what I expect, then this may need to be rethought.
Instructions:
- apply trac_10303-scripts-SAGE64.v3.patch.
- copy trac_10303-install to SAGE_ROOT/spkg/install, or apply the corresponding patch for that file.
- apply 10303-add-sage-check-64-dot-hgignore.patch to the base repo (SAGE_ROOT/spkg/base).
To produce a version with which to test from scratch, you then have to run "sage -sdist ..." to make sure that the necessary files from the scripts repo are copied to spkg/base.
comment:65 Changed 10 years ago by
- Description modified (diff)
comment:66 Changed 10 years ago by
What's the status for this issue?
It breaks sage -combinat install
on the OS X machine of a
colleague, precisely due to the SAGE64 verbosity effect in
sage -hg
.
Thanks!
Nicolas
comment:67 Changed 10 years ago by
- Description modified (diff)
The status is, the patches here don't work with upgrades. There was also some debate about the approach in general. If someone else has the time and energy to work on it, please go ahead; it might be best to start over.
comment:68 Changed 10 years ago by
For future reference: if the proposed fix works like the old one -- copying sage-check-64 to spkg/base -- then you must add sage-check-64 to the .hgignore file for the base repo. See also #11073 for other issues dealing with the base repo.
comment:69 follow-up: ↓ 70 Changed 10 years ago by
I'm considering reviving this ticket. As it stands, this ticket deals with two separate issues:
- where sage-check-64 is called, where the variable $SAGE64 is set (in sage-env or sage-check-64), etc.
- what sage-check-64 actually does: which platforms are relevant, does it write anything to the file SAGE_ROOT/local/lib/sage-64.txt, etc.
The second of these is more controversial, and also more complicated, so I think it should go on a separate ticket. The first of these can be solved, I think, by using a slight modification of the "v4" patch. Basically, we should modify everything touched by that patch except for sage-check-64. I'll test this and see how things go; if it looks good, I'll post a patch and open up a second ticket for rewriting sage-check-64.
comment:70 in reply to: ↑ 69 Changed 10 years ago by
Replying to jhpalmieri:
I'm considering reviving this ticket.
Brave of you John!
I've just spent 15 minutes reading all the comments. This is another of the many tickets that gets out of hand, with a huge number of requested changes.
As it stands, this ticket deals with two separate issues:
They should be on separate tickets.
It might be simpler to ask the release manager to close this ticket as "duplicate", and open two new tickets which deal with two issues in isolation. That will save there being a lot of irrelevant comments on this ticket. This has become too large for comfort.
Dave
comment:71 Changed 10 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Here are some patches. I believe these should also fix the problem at #10436. (I think you can see that problem if you apply the scripts patch, but then revert just the file sage-build. Then build the polybori spkg without SAGE64 being set.)
comment:72 Changed 10 years ago by
comment:73 Changed 10 years ago by
I don't know enough about the build process of Sage, i.e. the role of all these scripts, to comment much more on this ticket. It is also dependent on #9960 which is listed as needing work.
I'm happy to apply the patches and try some builds with and without setting SAGE64. But I'm not going to be the one to give this a positive review, as I don't understand it enough.
I'll also take a look at #11077 and put some comments there.
Dave
comment:74 Changed 10 years ago by
- Milestone changed from sage-4.7 to sage-4.7.1
Changing milestone to 4.7.1 since it depends on #9960.
comment:75 Changed 10 years ago by
- Priority changed from blocker to major
comment:76 Changed 9 years ago by
- Milestone changed from sage-5.3 to sage-duplicate/invalid/wontfix
- Resolution set to duplicate
- Reviewers changed from David Kirkby, Leif Leonhardy to John Palmieri, David Kirkby, Leif Leonhardy
- Status changed from needs_review to closed
I think none of the issues mentioned are still relevant (some were fixed at #12789). I'm going to close, but feel to protest and disagree.
scripts repo