#12479 closed enhancement (fixed)
Clean up sage-spkg
Reported by: | jdemeyer | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-5.0 |
Component: | build | Keywords: | |
Cc: | Merged in: | sage-5.0.beta7 | |
Authors: | Jeroen Demeyer | Reviewers: | John Palmieri |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #4949, #10192 | Stopgaps: |
Description (last modified by )
This ticket aims to do a simply clean-up of sage-spkg
and nothing more. The goal is to provide a solid base for future enhancements to sage-spkg
, not fix all issues regarding sage-spkg
.
Mostly, this patch reorganizes things. Some diagnostics are added, for example a warning when spkg-install
or spkg-check
isn't executable. Also, the line
TEST SUITE: not available
being printed to spkg/installed/$PKG_NAME
when SAGE_CHECK=yes
but there is no spkg-check
file.
Follow-ups:
Attachments (1)
Change History (27)
comment:1 Changed 11 years ago by
comment:2 Changed 11 years ago by
Description: | modified (diff) |
---|
Thanks for the link. I think that ticket is now too bitrotted. We should consider the ideas from that ticket and apply them (better in separate tickets instead of one "fix everything" ticket).
comment:3 Changed 11 years ago by
Dependencies: | → #4949 |
---|
comment:4 Changed 11 years ago by
Dependencies: | #4949 → #4949, #10192 |
---|
comment:5 follow-ups: 6 7 Changed 11 years ago by
What is the file SAGE.txt
referred to in this script? Should it be SPKG.txt
instead? For example:
echo >&2 "No file SAGE.txt in $PKG_NAME"
By the way, #12579 also modifies sage-spkg; you might take a look at that ticket if you have time.
What do you think of replacing ./spkg-check
with time ./spkg-check
?
comment:6 Changed 11 years ago by
Replying to jhpalmieri:
What is the file
SAGE.txt
referred to in this script? Should it beSPKG.txt
instead?
Of course, but I don't want to mistake of #11021: fixing too many things on one ticket. On this ticket, I essentially just want to clean things up, with little or none functional difference.
What do you think of replacing
./spkg-check
withtime ./spkg-check
?
comment:7 Changed 11 years ago by
comment:8 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:10 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:11 follow-up: 15 Changed 11 years ago by
This is pretty minor, but if a package has been installed already, for example, then with the patch, sage -i pkg
still creates SAGE_BUILD_DIR
, then quits without doing anything else. It would be nicer if this directory were not created until needed.
I will look at this more later.
comment:12 follow-up: 14 Changed 11 years ago by
My previous comment about creating directories unnecessarily also applies when running sage -i
with no arguments.
For "Authors", perhaps you could add something like:
- William Stein and others (Sage 4.8 and earlier)
after your name.
The header might also include documentation about the options -f, -s, -info. Actually, I don't think -info is used anywhere, nor is it accessible from the main sage script; should it be deleted altogether (here or on another ticket)? If we want to keep it, then consider lines 209-213:
echo "" if [ $? -ne 0 ]; then echo >&2 "No file SPKG.txt in $PKG_NAME" exit 1 fi
Won't the test if [ $? -ne 0]; then
reflect the exit status of the echo
command? I think that the echo
line should be deleted and the test moved inside the previous if block, so it's connected to the tar command. Or something like that.
comment:13 Changed 11 years ago by
Description: | modified (diff) |
---|
comment:14 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
Replying to jhpalmieri:
My previous comment about creating directories unnecessarily also applies when running
sage -i
with no arguments.For "Authors", perhaps you could add something like:
- William Stein and others (Sage 4.8 and earlier)after your name.
I changed this to
# AUTHORS: # # - Jeroen Demeyer (2012-02-27): #12479: big reorganization. # # - Volker Braun, Jeroen Demeyer (2012-01-18): #11073: remove the # spkg/base repository, move this file from local/bin/sage-spkg to # spkg/bin/sage-spkg. # # - William Stein, John Palmieri and others (Sage 4.8 and earlier).
The reason I put your name also is because it appears several times in hg log
of the 4.8 sage-spkg
.
Actually, I don't think -info is used anywhere, nor is it accessible from the main sage script; should it be deleted altogether (here or on another ticket)?
See #12606.
comment:15 follow-up: 16 Changed 11 years ago by
Replying to jhpalmieri:
This is pretty minor, but if a package has been installed already, for example, then with the patch,
sage -i pkg
still createsSAGE_BUILD_DIR
, then quits without doing anything else. It would be nicer if this directory were not created until needed.
Okay, I will change this in the follow-up #12602.
comment:16 Changed 11 years ago by
Replying to jdemeyer:
Replying to jhpalmieri:
This is pretty minor, but if a package has been installed already, for example, then with the patch,
sage -i pkg
still createsSAGE_BUILD_DIR
, then quits without doing anything else. It would be nicer if this directory were not created until needed.Okay, I will change this in the follow-up #12602.
Never mind, I'll do it on this ticket anyway.
comment:17 Changed 11 years ago by
Status: | needs_work → needs_review |
---|
John, new patch needs review. I also made some changes to spkg/bin/sage
in the calls to sage-spkg
. I think I took care of all your comments, except for -info
which I moved to #12606.
This version allows multiple options, so you can do something silly like
./sage -i -f -s -f -f -m spkg/optional/mpc-0.9.spkg
comment:18 Changed 11 years ago by
Status: | needs_review → needs_work |
---|
The patch to spkg/bin/sage
needs work: the combination of
if [ "$1" = '-i' ]; then shift install "$@" # used to be install " " "$@" fi
and (in the install function)
if [ $# -le 1 ]; then exec sage-spkg fi
means that sage -i pkg
just lists the currently installed packages. Maybe this test should say if [ $# -eq 0 ]
? Maybe if after processing the flags, there are no arguments left, then you should run exec sage-spkg
? For example:
-
spkg/bin/sage
diff --git a/spkg/bin/sage b/spkg/bin/sage
a b fi 767 767 768 768 install() { 769 769 cd "$SAGE_ROOT/spkg" 770 if [ $# -le 1 ]; then771 exec sage-spkg772 fi773 770 SAGE_LOGS="$SAGE_ROOT/spkg/logs" 774 771 mkdir -p "$SAGE_LOGS" 772 no_pkg="yes" 775 773 for PKG in "$@" 776 774 do 777 775 # Check for options … … install() { 784 782 -s) OPTS="-s" 785 783 continue;; 786 784 esac 785 no_pkg="no" 787 786 788 787 echo "Calling sage-spkg on '$PKG'" 789 788 PKG_NAME=`echo "$PKG" | sed -e "s/\.spkg$//"` … … install() { 804 803 fi 805 804 shift 806 805 done 806 if [ $no_pkg = "yes" ]; then 807 exec sage-spkg 808 fi 807 809 exit $? 808 810 }
There are probably better ways of doing this.
Otherwise, I think this is looking good.
This is a possible suggestion for another ticket: have a command-line flag for spkg-install which sets SAGE_CHECK
to be "yes". That might be more convenient than setting SAGE_CHECK=yes
, running sage -i ...
, then unsetting SAGE_CHECK
.
Changed 11 years ago by
Attachment: | 12479_clean_sage_spkg.patch added |
---|
comment:19 follow-up: 20 Changed 11 years ago by
Authors: | → Jeroen Demeyer |
---|---|
Reviewers: | → John Palmieri |
Status: | needs_work → needs_review |
Fixed, needs review.
I like your idea of a SPKG_CHECK=yes command line option, how about -c for check? But not on this ticket.
comment:20 Changed 11 years ago by
Description: | modified (diff) |
---|---|
Status: | needs_review → positive_review |
Okay, this looks good.
I like your idea of a SPKG_CHECK=yes command line option, how about -c for check? But not on this ticket.
See #12613.
comment:21 Changed 11 years ago by
This shouldn't have anything to do with positive review here, but aren't there a few spkgs that run tests automatically with or without SAGE_CHECK
, and don't have spkg-check files? I see them sometimes while waiting for Sage to build... For instance, R does this, I believe, and maybe one of the elliptic curve packages.
comment:22 Changed 11 years ago by
I'm not sure. Some spkgs (like zn_poly) run a small batch of tests always, and then run much longer tests if SAGE_CHECK
is set. Anything on this ticket or related ones won't affect tests which are run by the spkg-install
script; these just affect whether the spkg-check
script is run.
comment:23 Changed 11 years ago by
I guess I was referring to echo "Package $PKG_NAME has no test suite."
, which wouldn't strictly be true if there was a full test suite run that didn't happen to be in an spkg-check file. Well, not so important.
comment:24 Changed 11 years ago by
Merged in: | → sage-5.0.beta7 |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
comment:25 follow-up: 26 Changed 11 years ago by
I don't know to which ticket / change it exactly belongs, but I noticed that one meanwhile gets an error when using sage -i ...
before building Sage (something at the of sage-spkg
trying to cd
to some not-yet-existent directory, $SAGE_LOCAL/bin/
IIRC). The script doesn't exit with an error, just the shell complains, i.e. the spkg apparently gets installed, I'm just not sure whether some more or less important code got skipped.
Atm too tired to investigate myself... :P
comment:26 Changed 11 years ago by
Replying to leif:
I don't know to which ticket / change it exactly belongs, but I noticed that one meanwhile gets an error when using
sage -i ...
before building Sage (something at the ofsage-spkg
trying tocd
to some not-yet-existent directory,$SAGE_LOCAL/bin/
IIRC).
It must be the spkg-install
file doing that. With patch
(the first spkg I tried), there are no errors:
$ ./sage -i patch Calling sage-spkg on 'patch' patch-2.5.9.p2 ==================================================== Extracting package /padic/scratch/jdemeyer/sage-5.0.beta12/spkg/standard/patch-2.5.9.p2.spkg -rw-r--r-- 1 jdemeyer jdemeyer 174221 Aug 16 2011 /padic/scratch/jdemeyer/sage-5.0.beta12/spkg/standard/patch-2.5.9.p2.spkg Finished extraction **************************************************** Host system: Linux boxen 2.6.24-24-server #1 SMP Fri Sep 18 16:47:05 UTC 2009 x86_64 GNU/Linux **************************************************** C compiler: gcc C compiler version: Using built-in specs. COLLECT_GCC=gcc COLLECT_LTO_WRAPPER=/home/jdemeyer/local/libexec/gcc/x86_64-unknown-linux-gnu/4.6.1/lto-wrapper Target: x86_64-unknown-linux-gnu Configured with: ../gcc-4.6.1/configure --prefix=/home/jdemeyer/local Thread model: posix gcc version 4.6.1 (GCC) **************************************************** checking for gcc... gcc checking for C compiler default output... a.out checking whether the C compiler works... yes [...]
See also #11021.