Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#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 jhpalmieri)

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.

Attachments (1)

12479_clean_sage_spkg.patch (22.1 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 8 years ago by jhpalmieri

See also #11021.

comment:2 Changed 8 years ago by jdemeyer

  • 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 8 years ago by jdemeyer

  • Dependencies set to #4949

comment:4 Changed 8 years ago by jdemeyer

  • Dependencies changed from #4949 to #4949, #10192

comment:5 follow-ups: Changed 8 years ago by 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?

comment:6 in reply to: ↑ 5 Changed 8 years ago by 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?

comment:7 in reply to: ↑ 5 Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

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

Done.

comment:8 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:9 Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

John, could you please review this?

comment:10 Changed 8 years ago by jdemeyer

  • Description modified (diff)

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

comment:12 follow-up: Changed 8 years ago by 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.

comment:13 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:14 in reply to: ↑ 12 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to 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 in reply to: ↑ 11 ; follow-up: Changed 8 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 8 years ago by 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.

comment:17 Changed 8 years ago by jdemeyer

  • Status changed from needs_work to 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 8 years ago by jhpalmieri

  • Status changed from needs_review to 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 
    767767
    768768install() {
    769769    cd "$SAGE_ROOT/spkg"
    770     if [ $# -le 1 ]; then
    771         exec sage-spkg
    772     fi
    773770    SAGE_LOGS="$SAGE_ROOT/spkg/logs"
    774771    mkdir -p "$SAGE_LOGS"
     772    no_pkg="yes"
    775773    for PKG in "$@"
    776774    do
    777775        # Check for options
    install() { 
    784782            -s) OPTS="-s"
    785783                continue;;
    786784        esac
     785        no_pkg="no"
    787786
    788787        echo "Calling sage-spkg on '$PKG'"
    789788        PKG_NAME=`echo "$PKG" | sed -e "s/\.spkg$//"`
    install() { 
    804803        fi
    805804        shift
    806805    done
     806    if [ $no_pkg = "yes" ]; then
     807        exec sage-spkg
     808    fi
    807809    exit $?
    808810}

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 8 years ago by jdemeyer

comment:19 follow-up: Changed 8 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Reviewers set to John Palmieri
  • Status changed from needs_work to 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 in reply to: ↑ 19 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Status changed from needs_review to 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 8 years ago by 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.

comment:22 Changed 8 years ago by 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.

comment:23 Changed 8 years ago by 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.

comment:24 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta7
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:25 follow-up: Changed 8 years ago by 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

comment:26 in reply to: ↑ 25 Changed 8 years ago by 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
[...]
Note: See TracTickets for help on using tickets.