Clean up sage-spkg

[jdemeyer]

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:

1. #12579: allow selective SAGE_CHECK
2. #12602: rework downloading/extracting of .spkg files
3. #12606: Fix sage --info <package>
4. #12613: Add '-c' option so sage -i -c <pkg> installs the package and runs its test suite.

[jhpalmieri]

See also #11021.

[jdemeyer]

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).

[jhpalmieri]

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?

[jdemeyer]

Replying to jhpalmieri:

What is the file SAGE.txt referred to in this script? Should it be SPKG.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 with time ./spkg-check?

[jdemeyer]

Replying to jhpalmieri:

What do you think of replacing ./spkg-check with time ./spkg-check?

Done.

[jdemeyer]

John, could you please review this?

[jhpalmieri]

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.

[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.

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.

[jdemeyer]

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.

[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 creates SAGE_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.

[jdemeyer]

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 creates SAGE_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.

[jdemeyer]

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

[jhpalmieri]

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

@@ -767 +767 @@
 
 install() {
     cd "$SAGE_ROOT/spkg"
-    if [ $# -le 1 ]; then
-        exec sage-spkg
-    fi
     SAGE_LOGS="$SAGE_ROOT/spkg/logs"
     mkdir -p "$SAGE_LOGS"
+    no_pkg="yes"
     for PKG in "$@"
     do
         # Check for options
@@ -784 +782 @@
             -s) OPTS="-s"
                 continue;;
         esac
+        no_pkg="no"
 
         echo "Calling sage-spkg on '$PKG'"
         PKG_NAME=`echo "$PKG" | sed -e "s/\.spkg$//"`
@@ -804 +803 @@
         fi
         shift
     done
+    if [ $no_pkg = "yes" ]; then
+        exec sage-spkg
+    fi
     exit $?
 }

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.

[jdemeyer]

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.

[jhpalmieri]

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.

[kcrisman]

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.

[jhpalmieri]

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.

[kcrisman]

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.

[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 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

[jdemeyer]

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 of sage-spkg trying to cd 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
[...]