Opened 9 years ago

Closed 7 years ago

#11021 closed defect (fixed)

Fix install_package() library function

Reported by: jhpalmieri Owned by: leif
Priority: major Milestone: sage-5.3
Component: packages: standard Keywords: SPKG.txt SAGE.txt -info BUILD sage-env sage-sage sage-spkg
Cc: drkirkby, leif, fbissey Merged in: sage-5.4.beta0
Authors: Leif Leonhardy, Kelvin Li, Jeroen Demeyer Reviewers: Kelvin Li, Leif Leonhardy, John Palmieri
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jhpalmieri)

Apply

  1. trac_11021-fix_sage.misc.install_package.sagelib.patch

to the Sage library.

  1. trac_11021-root.patch to SAGE_ROOT.

OLD STUFF superseded by other tickets, no longer relevant:

Fix "sage -info" and a bug when sourcing sage-env more than once

One of the patches at #9960 did a lot of clean-up to the file sage-spkg: quoting environment variables, replacing tabs with spaces, etc., also adding comments and TODOs. Since those changes were not related to the issue at #9960, I've split them off and put them here instead. The main change of any content is to look at the file SPKG.txt rather than SAGE.txt when the "-info" flag is passed so sage-spkg (through sage -info ...). Also, warnings and error messages are now redirected to stderr.

The additional patch to sage-spkg is based on the v2 "clean-up" patch, fixing some more bugs, adding error checks, improving some messages (see comment below / commit message for some more details).

Apparently support for sage -info ... was removed at some point (or never existed); the patch to sage-sage fixes that.

The patch to sage-env fixes a bug caused or enabled by #10469, which through the patch to sage-spkg now becomes more visible and potentially worse.

DO NOT apply:

  1. trac_11021-sage-spkg-cleanup-v2-rebased_to_4.7.1.alpha4.patch
  2. trac_11021-additional_changes_to_sage-spkg.scripts.patch (somewhat optional, can be reviewed separately)
  3. trac_11021-support_and_document_sage_-info_in_sage-sage.scripts.patch
  4. trac_11021-export_BUILD_in_sage-env.scripts.patch

to the scripts repo.

Attachments (7)

trac_11021-sage-spkg-cleanup-v2-rebased_to_4.7.1.alpha4.patch (16.0 KB) - added by leif 8 years ago.
SCRIPTS repo. Version 2 simply rebased to Sage 4.7.1.alpha4.
trac_11021-export_BUILD_in_sage-env.scripts.patch (986 bytes) - added by leif 8 years ago.
SCRIPTS repo. Fix BUILD not being set if sage-env is sourced a second time. Based on Sage 4.7.1.alpha4.
trac_11021-support_and_document_sage_-info_in_sage-sage.scripts.patch (5.8 KB) - added by leif 8 years ago.
SCRIPTS repo. Implements/supports & documents "sage -info ..." in sage-sage; minor fixes. Based on Sage 4.7.1.alpha4.
trac_11021-additional_changes_to_sage-spkg.scripts.patch (20.1 KB) - added by leif 8 years ago.
SCRIPTS repo. Bug fixes, more error checks, improved messages, restructuring. Apply on top of the rebased v2-patch.
trac_11021-fix_sage.misc.install_package.sagelib.patch (3.6 KB) - added by jdemeyer 7 years ago.
Patch for the Sage library
11021_spkg.patch (601 bytes) - added by jdemeyer 7 years ago.
trac_11021-root.patch (573 bytes) - added by jhpalmieri 7 years ago.
root repo

Download all attachments as: .zip

Change History (75)

comment:1 Changed 9 years ago by jhpalmieri

  • Status changed from new to needs_review

comment:2 Changed 8 years ago by ltw

Patch has been rebased to sage 4.7. However, there is a problem on lines 136-143 in sage-spkg (after patch has been applied). The return code check doesn't do anything after the statement echo "", because that overwrites $? to 0. I can't figure out the intent of the preceding tar command, so I don't know how to fix this problem. Could someone help me?

comment:3 Changed 8 years ago by ltw

  • Description modified (diff)

comment:4 Changed 8 years ago by leif

Well, the code is a bit weird anyway; there should be an exit 1 if the package ($PKG_SRC) does not exist:

if [ "$INFO" -ne 0 ]; then
    if [ ! -f "$PKG_SRC" ]; then
        echo "Package $PKG_NAME not found"
        # EXIT HERE
    fi
    ...

I don't think we really need the echo "", since SPKG.txt files should be newline-terminated. (The tar commands extract this file to stdout.)


As far as I know, there are indeed (at least some old, optional) packages that do have a short SAGE.txt (rather than SPKG.txt), so I'm not sure if we shouldn't check for both.

Also, typical SPKG.txt files are quite long, so we might omit some parts or e.g. "cut" them where the Changelog section starts.

comment:5 follow-up: Changed 8 years ago by ltw

  • Description modified (diff)

Leif, I have updated the patch according to your first two suggestions. I don't know what to do with SAGE.txt or truncating long SPKG.txt files. I'm thinking that we should put those "real" changes in another ticket (or does one already exist?).

comment:6 in reply to: ↑ 5 ; follow-up: Changed 8 years ago by leif

Replying to ltw:

Leif, I have updated the patch according to your first two suggestions.

Ok, though I wouldn't have changed the "error" message; the previous one sounds a bit friendlier. (Actually a non-zero status code can have a couple of reasons there, but I don't mind. ;-) )


I don't know what to do with SAGE.txt or truncating long SPKG.txt files. I'm thinking that we should put those "real" changes in another ticket (or does one already exist?).

Maybe John has some idea or opinion on that...

(We could e.g. pipe the tar output to some sort of sed -e '/== Changelog ==/,$d' and perhaps give some message indicating it was omitted. Not important to me; maybe users less familiar with more or less could enjoy that - I personally don't use the -info option at all.)

comment:7 Changed 8 years ago by leif

  • Reviewers set to Kelvin Li

comment:8 in reply to: ↑ 6 ; follow-up: Changed 8 years ago by ltw

Replying to leif:

Ok, though I wouldn't have changed the "error" message; the previous one sounds a bit friendlier. (Actually a non-zero status code can have a couple of reasons there, but I don't mind. ;-) )

A few lines above, there is an error message "Package $PKG_NAME not found". So I thought "File SPKG.txt not found in package $PKG_NAME" would follow the same grammar. But I can change it back, no problem. :-)

(We could e.g. pipe the tar output to some sort of sed -e '/== Changelog ==/,$d' and perhaps give some message indicating it was omitted.

That's pretty hackish in my opinion. It would impose an undocumented constraint on the format of SPKG.txt, which does not have any formatting validation otherwise. I'm not sure that truncating SPKG.txt is even desirable.

comment:9 in reply to: ↑ 8 Changed 8 years ago by leif

Replying to ltw:

Replying to leif:

Ok, though I wouldn't have changed the "error" message; the previous one sounds a bit friendlier. (Actually a non-zero status code can have a couple of reasons there, but I don't mind. ;-) )

A few lines above, there is an error message "Package $PKG_NAME not found". So I thought "File SPKG.txt not found in package $PKG_NAME" would follow the same grammar. But I can change it back, no problem. :-)

What about "Could not extract SPKG.txt." or better "Package $PKG_NAME appears to have no SPKG.txt."?

Redirecting stderr of the tar commands to /dev/null is a bit odd there. On the other hand, the presence of $PKG_SRC (the spkg file) itself is checked twice a few lines above, so a non-zero exit code of tar most probably means either a corrupted tarball or the file to extract wasn't found. Maybe the proper way would be to first try to list the file (tar tf - ... 2>/dev/null) into some shell variable and check if it is empty (or, better, list the tarball's whole contents grepping for $PKG_NAME/SPKG.txt), and use bunzip2 -t ... to test whether the spkg is compressed in advance.

Or verify the spkg wasn't corrupted in case of an error ($? -ne 0):

    ...
    if [ $? -ne 0 ]; then
        # Extracting SPKG.txt failed for *some* reason,
        # now check if it's simply missing:
        if (bunzip2 -c "$PKG_SRC" | tar tf -) &>/dev/null ||
           tar tf "$PKG_SRC" &>/dev/null
        then
            echo "Package $PKG_NAME lacks a description (SPKG.txt file)."
            exit 1
        else
            echo "$PKG_SRC seems to be corrupted. Exiting."
            exit 1 # or some other error code
        fi
    fi
    exit 0
fi

But IMHO a rather minor issue.


(We could e.g. pipe the tar output to some sort of sed -e '/== Changelog ==/,$d' and perhaps give some message indicating it was omitted.

That's pretty hackish in my opinion. It would impose an undocumented constraint on the format of SPKG.txt, which does not have any formatting validation otherwise. I'm not sure that truncating SPKG.txt is even desirable.

SPKG.txt files are supposed to have a quite clear structure, with ReST (section) mark-up (which an spkg reviewer should check btw). If there's no line matching == Changelog == -- or preferably a more generic, i.e. robust pattern, nothing would get omitted.

But as I said, I have no opinion on truncating them there.

comment:10 follow-ups: Changed 8 years ago by leif

P.S.: I wonder if we shouldn't also redirect all warning and error messages to stderr (echo >2 "..."). We just recently started doing so in a couple of other scripts, so it's not yet consistent within Sage, but an ongoing change.

comment:11 in reply to: ↑ 10 Changed 8 years ago by leif

Replying to leif:

I wonder if we shouldn't also redirect all warning and error messages to stderr (echo >2 "...").

echo >&2 "..." of course.

comment:12 in reply to: ↑ 10 Changed 8 years ago by ltw

Replying to leif:

What about "Could not extract SPKG.txt." or better "Package $PKG_NAME appears to have no SPKG.txt."?

I have decided to follow the original phrasing: "No file SPKG.txt in package $PKG_NAME".

Or verify the spkg wasn't corrupted in case of an error ($? -ne 0):

<SNIP>

But IMHO a rather minor issue.

I didn't touch it for this patch update; I'll have to think about it later. :-)

SPKG.txt files are supposed to have a quite clear structure, with ReST (section) mark-up (which an spkg reviewer should check btw). If there's no line matching == Changelog == -- or preferably a more generic, i.e. robust pattern, nothing would get omitted.

But as I said, I have no opinion on truncating them there.

Also didn't change anything here. I didn't know that SPKG.txt is supposed to be structured; honestly I've never even read one. O_o

Replying to leif:

P.S.: I wonder if we shouldn't also redirect all warning and error messages to stderr (echo >2 "..."). We just recently started doing so in a couple of other scripts, so it's not yet consistent within Sage, but an ongoing change.

I have changed a number of these. I wasn't sure on a few cases; please check them.

I also replaced a few instances of echo "*************" with a common function call (print_separator). I'm not sure whether this was a good idea. Please criticize!

comment:13 Changed 8 years ago by leif

  • Keywords SPKG.txt SAGE.txt -info added

comment:14 follow-up: Changed 8 years ago by leif

  • Status changed from needs_review to needs_work
  • Work issues set to Rebase to 4.7.1.alpha4

Hmmm, for Sage 4.7.1.alpha4, we'll have to rebase the patch:

applying /home/leif/Sage/patches/trac_11021-sage-spkg-cleanup-v2.patch
patching file sage-spkg
Hunk #3 succeeded at 59 (offset 4 lines).
Hunk #4 succeeded at 93 (offset 4 lines).
Hunk #5 succeeded at 139 (offset 4 lines).
Hunk #6 succeeded at 164 (offset 4 lines).
Hunk #7 succeeded at 183 (offset 4 lines).
Hunk #8 succeeded at 191 (offset 4 lines).
Hunk #9 succeeded at 226 (offset 4 lines).
Hunk #10 succeeded at 250 (offset 4 lines).
Hunk #11 succeeded at 273 (offset 4 lines).
Hunk #12 succeeded at 292 (offset 4 lines).
Hunk #13 FAILED at 304
Hunk #14 succeeded at 402 (offset 4 lines).
Hunk #15 succeeded at 413 (offset 4 lines).
Hunk #16 succeeded at 439 (offset 4 lines).
1 out of 16 hunks FAILED -- saving rejects to file sage-spkg.rej
abort: patch failed to apply

Sorry for the delay btw, been busy with other things.

Though there are still some things that could be improved (e.g. still two instances of build rather than $BUILD IIRC), and I would have put the >&2 right after echo to be more obvious, I'm ok with your changes.

I.e., I'll rebase the patch as is, test it, and if you're ok with my previous changes, we'd have a positive review.

(Other things like supporting SAGE_CHECK=ignore and what else we've mentioned on this ticket can be implemented on follow-ups, we just have to get this ticket merged first... :) )

Changed 8 years ago by leif

SCRIPTS repo. Version 2 simply rebased to Sage 4.7.1.alpha4.

comment:15 in reply to: ↑ 14 Changed 8 years ago by leif

  • Description modified (diff)
  • Status changed from needs_work to needs_review
  • Work issues Rebase to 4.7.1.alpha4 deleted

Replying to leif:

I'll rebase the patch as is.

Done.

Changed 8 years ago by leif

SCRIPTS repo. Fix BUILD not being set if sage-env is sourced a second time. Based on Sage 4.7.1.alpha4.

Changed 8 years ago by leif

SCRIPTS repo. Implements/supports & documents "sage -info ..." in sage-sage; minor fixes. Based on Sage 4.7.1.alpha4.

comment:16 Changed 8 years ago by leif

  • Authors changed from Leif Leonhardy to Leif Leonhardy, Kelvin Li
  • Description modified (diff)
  • Keywords BUILD sage-env sage-sage added
  • Priority changed from minor to major
  • Reviewers changed from Kelvin Li to Kelvin Li, Leif Leonhardy

The other two patches I've attached fix issues that came up while testing the patch to sage-spkg with Sage 4.7.1.alpha4.

They're both necessary to make sage -info ... as well as sage-spkg in general (with the changes made by the other patch) work.

Cf. http://trac.sagemath.org/sage_trac/ticket/10469#comment:24.

Please review!

comment:17 Changed 8 years ago by leif

  • Keywords sage-spkg added
  • Summary changed from clean up sage-spkg to Fix "sage -info" and a bug when sourcing sage-env more than once

comment:18 Changed 8 years ago by leif

  • Owner changed from tbd to leif

I've made some more changes to sage-spkg (more than I intended):

  • Fixed some bugs, e.g. not recording the version of a package found in spkg/{standard,optional} such that the installation later failed; superfluous second download now doesn't take place.
  • All instances of build now use $BUILD. (It is now also checked that BUILD is really set, cf. patch to sage-env.)
  • More error checks and more appropriate error messages.
  • Non-conforming spkgs are no longer said to be corrupted.
  • Default installation scripts now use $MAKE rather than make ($MAKE -j1 for installation).
  • spkg-check is now timed, too.
  • Trying to install an already installed spkg now suggests using -f.
  • Some restructuring and simplification, removed useless code.
  • Lots of comments (including TODOs) added, some cosmetic changes.

Harder to review, but please also do. ;-)

(Patch is based on the rebased v2, i.e. should be applied after that.)

Changed 8 years ago by leif

SCRIPTS repo. Bug fixes, more error checks, improved messages, restructuring. Apply on top of the rebased v2-patch.

comment:19 Changed 8 years ago by leif

  • Description modified (diff)

New patch is up now.

comment:20 Changed 8 years ago by leif

  • Description modified (diff)

I've added another, necessary patch to the Sage library, since otherwise e.g. sage -b would break (just because to detect if an optional spkg was installed the function finally called used sage -f instead of sage -i).

P.S.: The fully qualified name of the function is of courseTM sage.misc.package.install_package().

comment:21 Changed 8 years ago by fbissey

  • Cc fbissey added

comment:22 follow-up: Changed 8 years ago by jhpalmieri

  • Reviewers changed from Kelvin Li, Leif Leonhardy to Kelvin Li, Leif Leonhardy, John Palmieri

I'm happy with the patch to the Sage library. I'm attaching a referee patch for that one, adding a doctest and fixing the part marked "FIXME". I will try to look at the many patches to the scripts repo, but it may take me a while.

comment:23 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:24 follow-up: Changed 8 years ago by jhpalmieri

I'm utterly tired of the message "Warning: Attempted to overwrite SAGE_ROOT environment variable". Can we apply this patch to sage-env:

  • sage-env

    diff --git a/sage-env b/sage-env
    a b if [ "$SAGE_ROOT" = "" ]; then 
    5050else
    5151    if [ -f "$SAGE_ROOT"/sage -a -d "$SAGE_ROOT"/spkg ]; then
    5252        # SAGE_ROOT points to a sage installation as expected
    53         if [ "$SAGE_ROOT" != "$GUESSED_SAGE_ROOT" ]; then
     53        if [ -n "$GUESSED_SAGE_ROOT" -a "$SAGE_ROOT" != "$GUESSED_SAGE_ROOT" ]; then
    5454            echo "Warning: Attempted to overwrite SAGE_ROOT environment variable"
    5555        fi
    5656    else

That will eliminate some of its occurrences.

Meanwhile, I'm happy with trac_11021-support_and_document_sage_-info_in_sage-sage.scripts.patch and trac_11021-export_BUILD_in_sage-env.scripts.patch. trac_11021-sage-spkg-cleanup-v2-rebased_to_4.7.1.alpha4.patch also looks good, and anyway I think it's just a rebased version of a patch which already had a positive review. This leaves trac_11021-additional_changes_to_sage-spkg.scripts.patch. I'll try to work on that, and anyone else who wants to is welcome.

comment:25 Changed 8 years ago by jhpalmieri

Actually, a few minor comments about trac_11021-support_and_document_sage_-info_in_sage-sage.scripts.patch and trac_11021-export_BUILD_in_sage-env.scripts.patch . trac_11021-sage-spkg-cleanup:

  • The file.spkg argument should be documented in the -advanced option. Easy to fix, but I'll leave it to you in hopes that you'll fix the next problem as well.
  • If you run "sage -info zlib sqlite", it says "Listing SPKG.txt of zlib sqlite", which looks a little funny to me. This is not a big deal, but if you could fix it, that would be nice.

comment:26 Changed 8 years ago by jhpalmieri

Oh, and also, "sage --info ..." should work...

comment:27 Changed 8 years ago by jhpalmieri

Oy. The few minor comments are about the patch to sage-sage, in case that wasn't clear. Sorry about the mess in that comment.

comment:28 in reply to: ↑ 22 Changed 8 years ago by leif

Replying to jhpalmieri:

I'm happy with the patch to the Sage library. I'm attaching a referee patch for that one, adding a doctest and fixing the part marked "FIXME".

Looks good to me, except that I'm not sure whether the (first) doctest passes on all systems. Ok, thinking more about it, there'll be a fake spkg/installed/atlas-* on systems where ATLAS doesn't get built/installed, right?

Thanks for reviewing this.

comment:29 in reply to: ↑ 24 ; follow-up: Changed 8 years ago by leif

Replying to jhpalmieri:

I'm utterly tired of the message "Warning: Attempted to overwrite SAGE_ROOT environment variable". Can we apply this patch to sage-env [...]

Yes, yes, yes, please!

I really wonder others (especially "ordinary" Sage users) didn't already get totally annoyed by this odd message, at least I'm not aware of any complaints about that. Maybe it's just us...

comment:30 follow-up: Changed 8 years ago by jhpalmieri

Looks good to me, except that I'm not sure whether the (first) doctest passes on all systems. Ok, thinking more about it, there'll be a fake spkg/installed/atlas-* on systems where ATLAS doesn't get built/installed, right?

Right. First, it passed tests on my Mac OS X box, and the atlas spkg doesn't installed anything there. Second, the script sage-spkg writes a file to spkg/installed as long as spkg-install runs successfully, and since the atlas spkg-install always runs (even if it doesn't do anything), there will be a record in spkg/installed.

comment:31 follow-up: Changed 8 years ago by leif

Replying to jhpalmieri:

  • The file.spkg argument should be documented in the -advanced option. Easy to fix, but I'll leave it to you in hopes that you'll fix the next problem as well.

Well, that's not advanced usage ;-) , but I can add it there, too. (It's btw. not a new feature, I just documented it.)

  • If you run "sage -info zlib sqlite", it says "Listing SPKG.txt of zlib sqlite", which looks a little funny to me. This is not a big deal, but if you could fix it, that would be nice.

Hmmm, that's intentional. Do you want the spkg names to be separated by commas?

Oh, and also, "sage --info ..." should work...

We (currently) don't support long options consistently, but I can also add that (i.e., support --info, too).

comment:32 in reply to: ↑ 30 Changed 8 years ago by leif

Replying to jhpalmieri:

Looks good to me, except that I'm not sure whether the (first) doctest passes on all systems. Ok, thinking more about it, there'll be a fake spkg/installed/atlas-* on systems where ATLAS doesn't get built/installed, right?

Right. First, it passed tests on my Mac OS X box, and the atlas spkg doesn't installed anything there. Second, the script sage-spkg writes a file to spkg/installed as long as spkg-install runs successfully, and since the atlas spkg-install always runs (even if it doesn't do anything), there will be a record in spkg/installed.

Ok, hopefully this also works with SAGE_CHECK=yes... (since if the test suite -- does ATLAS have one, i.e. spkg-check, at all? -- fails, the file in .../installed/ gets deleted).

comment:33 Changed 8 years ago by jhpalmieri

A few issues with trac_11021-additional_changes_to_sage-spkg.scripts.patch -- line numbers refer to the file sage-spkg (as listed in the patch):

  • line 59, error >&2 "Error: Environment variable 'BUILD' not set!": "error" should be "echo"
  • lines 175-176: the code (bunzip2 -c "$PKG_SRC" | tar tf -) seems to have a zero return value if $PKG_SRC is not a valid bzip2 file. I'm trying to reach the error message echo >&2 "Error: '$PKG_SRC' seems to be corrupted. Exiting.", but I can't. This is on Mac OS X; on linux, the above code has return value "2". Running "bunzip2 -c blah" on an invalid bzip2 file returns a value of 2 on Mac OS X.

comment:34 Changed 8 years ago by jhpalmieri

The relevant part of the spkg-check file for atlas says:

if [ `uname` = "Darwin" ]; then
    exit 0
fi

comment:35 in reply to: ↑ 31 ; follow-up: Changed 8 years ago by jhpalmieri

Replying to leif:

Replying to jhpalmieri:

  • The file.spkg argument should be documented in the -advanced option. Easy to fix, but I'll leave it to you in hopes that you'll fix the next problem as well.

Well, that's not advanced usage ;-) , but I can add it there, too.

It would fit in well after the lines

Running Sage:
  file.[sage|py|spyx] -- run given .sage, .py or .spyx files

(It's btw. not a new feature, I just documented it.)

Right.

  • If you run "sage -info zlib sqlite", it says "Listing SPKG.txt of zlib sqlite", which looks a little funny to me. This is not a big deal, but if you could fix it, that would be nice.

Hmmm, that's intentional. Do you want the spkg names to be separated by commas?

If possible, commas would be nice. It looks fine with a single argument, but with multiple args, I think some punctuation would help.

Oh, and also, "sage --info ..." should work...

We (currently) don't support long options consistently, but I can also add that (i.e., support --info, too).

We say that we do: at the end of "sage -advanced", it says

You can also use -- before a long option, e.g., 'sage --optional'.

I see that a few options are missing, like --merge`, but most of them are there. The script sage-sage is a mess anyway, but cleaning it up belongs on another ticketTM.

comment:36 Changed 8 years ago by leif

[Hours later: Sorry, apparently my router is just about to die -- massive packet losses... :( Now via wireless LAN...]

Replying to jhpalmieri:

  • line 59, error >&2 "Error: Environment variable 'BUILD' not set!": "error" should be "echo"

Ooops, kind of inconvenient, though it should raise an error as is... ;-)


  • lines 175-176: the code (bunzip2 -c "$PKG_SRC" | tar tf -) seems to have a zero return value if $PKG_SRC is not a valid bzip2 file. I'm trying to reach the error message echo >&2 "Error: '$PKG_SRC' seems to be corrupted. Exiting.", but I can't. This is on Mac OS X; on linux, the above code has return value "2". Running "bunzip2 -c blah" on an invalid bzip2 file returns a value of 2 on Mac OS X.

MacOS... What does cat /dev/null | tar tf - give?


The relevant part of the spkg-check file for atlas says [...]

I haven't looked at the current ATLAS spkg; don't know if it also works on Cygwin, e.g. if SAGE_ATLAS_LIB is set there (in which case any tests are skipped as well).

comment:37 in reply to: ↑ 35 Changed 8 years ago by leif

Replying to jhpalmieri:

Replying to leif:

  • If you run "sage -info zlib sqlite", it says "Listing SPKG.txt of zlib sqlite", which looks a little funny to me. This is not a big deal, but if you could fix it, that would be nice.

Hmmm, that's intentional. Do you want the spkg names to be separated by commas?

If possible, commas would be nice. It looks fine with a single argument, but with multiple args, I think some punctuation would help.

Well, sage -i pkg1 pkg2 ... behaves the same ("Installing pkg1 pkg2 ...", and did before), but I can add commas to both -- easily, until we support (or use) spaces in spkg filenames...


We (currently) don't support long options consistently, but I can also add that (i.e., support --info, too).

We say that we do: at the end of "sage -advanced", it says

You can also use -- before a long option, e.g., 'sage --optional'.

I see that a few options are missing, like --merge`, but most of them are there.

sage -t --verbose ... doesn't work either; only sage-sage seems to support (some) long options with double dashs.

The script sage-sage is a mess anyway, but cleaning it up belongs on another ticketTM.

Agreed. I'd also remove long options with a single dash (i.e., only allow -- there), but others disagreed a while ago.

comment:38 follow-up: Changed 8 years ago by jhpalmieri

MacOS... What does cat /dev/null | tar tf - give?

$ cat /dev/null | tar tf -
$ echo $?
0

$ cat temp.pdf | tar jxf -
tar: Unrecognized archive format: Inappropriate file type or format
tar: Error exit delayed from previous errors.
$ echo $?
1

For the second example, the same thing happens with "tar xf" as with "tar jxf". The man page for tar said nothing helpful about return codes. It seems to be BSD tar, for what that's worth.

Well, sage -i pkg1 pkg2 ... behaves the same ("Installing pkg1 pkg2 ...", and did before), but I can add commas to both -- easily, until we support (or use) spaces in spkg filenames...

I don't care that much about it. If you want to change it, that would be fine, but don't worry about it too much.

Agreed. I'd also remove long options with a single dash (i.e., only allow -- there), but others disagreed a while ago.

I had a plan (at #21) to gradually phase them out, but that ticket has stalled. There are also some issues with the approach there, and I'm not planning on working on it any time soon.

comment:39 in reply to: ↑ 38 ; follow-ups: Changed 8 years ago by leif

Replying to jhpalmieri:

MacOS... What does cat /dev/null | tar tf - give?

$ cat /dev/null | tar tf -
$ echo $?
0

That's odd. IIRC we require GNU tar, which behaves correctly:

$ cat /dev/null | tar tf -
tar: This does not look like a tar archive
tar: Exiting with failure status due to previous errors

Maybe we can work around in a dumb way (which I'd still prefer over using pipestatus there) by doing

(bunzip2 -c "$PKG_SRC" || echo foo) | tar tf -

which should fail in any case if bunzip2 fails.

comment:40 in reply to: ↑ 39 Changed 8 years ago by leif

Replying to leif:

That's odd. IIRC we require GNU tar, which behaves correctly.

Ah, the Sage Installation Guide (though usually not a reliable source of contemporary information) states GNU tar should be used / installed on Solaris, OpenSolaris, HP-UX and AIX; BSD tar is said to be sufficient on MacOS X.

comment:41 in reply to: ↑ 39 Changed 8 years ago by jhpalmieri

Replying to leif:

(bunzip2 -c "$PKG_SRC" || echo foo) | tar tf -

which should fail in any case if bunzip2 fails.

That seems to work, both from the command line to test the return value and when inserted into sage-spkg in the appropriate place.

comment:42 in reply to: ↑ 29 Changed 8 years ago by drkirkby

Replying to leif:

Replying to jhpalmieri:

I'm utterly tired of the message "Warning: Attempted to overwrite SAGE_ROOT environment variable". Can we apply this patch to sage-env [...]

Yes, yes, yes, please!

I really wonder others (especially "ordinary" Sage users) didn't already get totally annoyed by this odd message, at least I'm not aware of any complaints about that. Maybe it's just us...

I think given that when building Sage a user will see thousands of warnings messages, they are not likely to report this.

It probably bothers John more as he knows it's code written by Sage developers that is generating the warning. But for "normal" users (non-developers), there's no reason this warning should stand out in any way.

As for the tar problem, there is no requirement for tar to even exist on a POSIX system, so it not really possible to say any particular behavior is wrong.

Dave

comment:43 follow-up: Changed 8 years ago by jhpalmieri

By the way, the same issue occurs on line 290 of sage-spkg, and the same fix seems to work. Probably the same on line 323

Re lines 236-243 (and I would guess a similar block in lines 308-314):

        PKG_NAME=`sage-latest-online-package "$PKG_NAME"`
        if [ $? -eq 0 ]; then
            echo "Found package '$PKG_NAME'."
            FOUND_VERSION='1'
        else
            echo >&2 "$PKG_NAME" # That's now an error message.
            exit 1
        fi

In the "else" part of this, $PKG_NAME will likely be an empty string, so the echo won't print anything. The script sage-latest-online-package prints a suitable error message so this is not a big deal, but it's kind of odd.


Regarding the comments about Atlas and spkg-check, in retrospect, I'm not sure I understand what you're saying. If spkg-check fails, then it makes sense for atlas not to be installed, in which case you don't have a complete Sage install, in which case I don't mind if a doctest fails. But as I said, I may be misunderstanding your point.

comment:44 Changed 8 years ago by jhpalmieri

A related issue with trac_11021-additional_changes_to_sage-spkg.scripts.patch: on OS X, if I insert

(bunzip2 -c "$PKG_SRC" || echo foo) | tar tf -

instead of the other code, then valid spkg files are not being extracted: I did this at around line 300, and I also put in "pwd" and "ls -l" a few lines later, and I got this (this is with a local spkg file, phc-2.3.60.spkg, specified with a full path):

Finished extraction.
/Applications/sage/spkg/build
total 8
drwxr-xr-x  76 palmieri  admin  2584 Jul  4 09:35 bzip2-1.0.5
drwxr-xr-x   8 palmieri  admin   272 Jul 12 15:10 chomp-20100213.p2
drwxr-xr-x   2 palmieri  admin    68 Jul 12 16:55 old
drwxr-xr-x@ 21 palmieri  admin   714 Jul  4 09:35 prereq-0.9
-rw-r--r--   1 palmieri  admin    19 Mar 24  2010 sagetex-2.2.5.cksum
Warning: After extraction the directory 'phc-2.3.60' does not exist.
This means that the corresponding .spkg needs to be downloaded again.

But maybe I didn't insert that code correctly. I'll try a new patch when you have chance to make one. Or if you want, just make the small changes to the sage-sage patch, and we can defer the more major changes to sage-spkg to another ticket. We should try to get the bug fixes in the other patches merged pretty soon, I think.

comment:45 in reply to: ↑ 43 Changed 8 years ago by leif

Replying to jhpalmieri:

Re lines 236-243 (and I would guess a similar block in lines 308-314):

        PKG_NAME=`sage-latest-online-package "$PKG_NAME"`
        if [ $? -eq 0 ]; then
            echo "Found package '$PKG_NAME'."
            FOUND_VERSION='1'
        else
            echo >&2 "$PKG_NAME" # That's now an error message.
            exit 1
        fi

In the "else" part of this, $PKG_NAME will likely be an empty string, so the echo won't print anything. The script sage-latest-online-package prints a suitable error message so this is not a big deal, but it's kind of odd.

Hmmm, shame on me. I was pretty sure sage-latest-online-package would print the error message to stdout (rather than stderr) since otherwise these two instances of echo $PKG_NAME really wouldn't make sense, so I just added an according comment -- without looking at the script. 8/

We can either just remove the echo ... or add 2>&1 to the command substitution.


Regarding the comments about Atlas and spkg-check, in retrospect, I'm not sure I understand what you're saying. If spkg-check fails, then it makes sense for atlas not to be installed, in which case you don't have a complete Sage install, in which case I don't mind if a doctest fails. But as I said, I may be misunderstanding your point.

Forget about that. I just wasn't sure whether ATLAS's spkg-check is "Cygwin-hard" at the moment. If it is not, the build might fail. At least without SAGE_CHECK=yes, spkg/installed/atlas-* should (currentlyTM) always be there, no matter if ATLAS really got (re)installed or not.

comment:46 Changed 8 years ago by jhpalmieri

We can either just remove the echo ... or add 2>&1 to the command substitution.

I think removing the echo command makes sense.

comment:47 Changed 8 years ago by leif

Trying to recap:

  • sage-sage:
    • Support also --info.
    • Document sage <file>.spkg in more places.
    • Print spkg names separated by commas ( sage <-i|-info|-f> ...).
  • sage-spkg:
    • Fix error typo.
    • Support BSD tar. (I meanwhile have a copy installed for testing.) W.r.t. your PHC spkg, you probably just inserted the wrong code snippet (perhaps with t instead of x?).
    • Remove useless echo $PKG_NAME if sage-latest-online-package failed (twice).

comment:48 Changed 8 years ago by jhpalmieri

That looks like everything. Adding the commas is the lowest priority, I would say. If you want to add the code from 24 about "Warning: Attempted to overwrite SAGE_ROOT environment variable", that would be fine, too: it's beyond the scope of the ticket, but it looks like a pretty safe change.

comment:49 Changed 8 years ago by leif

I wonder if there's a better (and more efficient) way to test if an spkg is (bzip2-) compressed, preferably in a shell function.

I suspect something like

if file "$spkg" | grep "bzip2" >/dev/null; then
    # it is compressed
    ...

wouldn't be very portable, since file might not recognize bzip2 files on some systems.

We could also directly check the magic header instead, with e.g. head and cut or dd.

Any further suggestions?

comment:50 follow-up: Changed 8 years ago by jhpalmieri

Not a shell function, but isn't this what "bunzip2 -t FILE" is for?

comment:51 Changed 8 years ago by leif

Replying to jhpalmieri:

That looks like everything. Adding the commas is the lowest priority, I would say.

Ok.


If you want to add the code from 24 about "Warning: Attempted to overwrite SAGE_ROOT environment variable", that would be fine, too: it's beyond the scope of the ticket, but it looks like a pretty safe change.

Ah, I didn't remember it was an "inline" patch; I thought you already attached a reviewer patch for that. Can add this, too, of course.


If all are happy, I can also provide cumulative patches later.

comment:52 in reply to: ↑ 50 ; follow-up: Changed 8 years ago by leif

Replying to jhpalmieri:

Not a shell function, but isn't this what "bunzip2 -t FILE" is for?

Well, that's quite inefficient if the file indeed is bzip2-compressed.

comment:53 in reply to: ↑ 52 Changed 8 years ago by jhpalmieri

Replying to leif:

Replying to jhpalmieri:

Not a shell function, but isn't this what "bunzip2 -t FILE" is for?

Well, that's quite inefficient if the file indeed is bzip2-compressed.

Yes and no. On my computer, in the worst case, it takes about 7 or 8 seconds (for the main sage spkg). Installing that package takes 35 minutes, so the added time is insignificant, taken in proportion. For most of the standard spkg's, this is the case. (Running time bunzip2 -t *.spkg on the directory SAGE_ROOT/spkg/standard takes 49 seconds on my machine, compared to 2 hours to build all of Sage.)

comment:54 Changed 8 years ago by jhpalmieri

I think I found another consequence of not exporting BUILD: because of the stupid way the RPy package is installed, its build directory ends up being SAGE_ROOT/spkg/rpy2-2.0.8, rather than SAGE_ROOT/spkg/build/rpy2-2.0.8.

comment:55 Changed 8 years ago by jhpalmieri

Any progress on this?

comment:56 follow-up: Changed 8 years ago by jhpalmieri

  • Status changed from needs_review to needs_work

Large parts of this have been subsumed by #12479 and related tickets. I think there are still worthwhile changes here; should we keep this open, or close it and open more specific tickets? For example, this change still looks like a good idea:

  • spkg/bin/sage-spkg

    diff --git a/spkg/bin/sage-spkg b/spkg/bin/sage-spkg
    a b fi 
    361361if [ ! -f spkg-install ]; then
    362362    echo '#!/usr/bin/env bash' > spkg-install
    363363    if [ -x configure ]; then
    364         echo './configure --prefix="$SAGE_LOCAL" && make && make install' >> spkg-install
     364        echo './configure --prefix=\"\$SAGE_LOCAL\" && make && make install' >> spkg-install
    365365    elif [ -f setup.py ]; then
    366366        echo 'python setup.py install' >> spkg-install
    367367    else

So anyway, this ticket needs work.

comment:57 in reply to: ↑ 56 Changed 8 years ago by leif

Replying to jhpalmieri:

Large parts of this have been subsumed by #12479 and related tickets. I think there are still worthwhile changes here; should we keep this open, or close it and open more specific tickets?

Yes, it would have been much easier if people had based their work on this here (or simply [first] completed this ticket; there wasn't much missing, and everything was well documented)... 8/

I wouldn't close it yet, but I currently don't know what's meanwhile been solved elsewhere.

At least rebasing seems impossible, i.e., all changes would have to manually be put into one or more completely new patches.

comment:58 Changed 7 years ago by jdemeyer

  • Description modified (diff)

I had a quick look over the scripts patches and I think everything there has been fixed in other patches (most recently, #12606, which needs review by the way).

I think we should still keep the Sage library patches.

comment:59 Changed 7 years ago by jdemeyer

  • Authors changed from Leif Leonhardy, Kelvin Li to Leif Leonhardy, Kelvin Li, Jeroen Demeyer
  • Description modified (diff)
  • Status changed from needs_work to needs_review

comment:60 Changed 7 years ago by jdemeyer

I have merged the two Sage library patches into one and made some further cosmetic changes.

Could anybody have a look please?

Changed 7 years ago by jdemeyer

Patch for the Sage library

comment:61 follow-up: Changed 7 years ago by jhpalmieri

The Sage library patch looks okay to me. I'm not completely happy with a feature of both the old and new implementation. In Sage 5.3.rc0, cvxopt (for example) has been upgraded compared to the version on the web page, so if I do install_package('cvxopt'), it tries to download and install the previous version. Should the behavior of standard_packages() be changed? Or should its output be modified before use in install_package(...), taking other (later) versions of packages into account?

By the way, I see a regression in the behavior of Sage: sage file.spkg doesn't work anymore. It used to (as of Sage 4.7, at least) install the given spkg. When I run it now, I get an error. On sage.math:

$ sage /scratch/palmieri/sage/spkg/standard/cvxopt-1.1.5.spkg 
Calling sage-spkg on ''
/scratch/palmieri/sage-5.3.beta2/spkg/bin/sage-spkg: line 152: [: =: unary operator expected
/scratch/palmieri/sage-5.3.beta2/spkg/bin/sage-spkg: line 158: [: =: unary operator expected
/scratch/palmieri/sage-5.3.beta2/spkg/bin/sage-spkg: line 163: [: =: unary operator expected
/scratch/palmieri/sage-5.3.beta2/spkg/bin/sage-spkg: line 168: [: =: unary operator expected
Attempting to download package 
Searching for latest online version of 
Could not find a version for .

I guess it's from these lines, in particular the second line, from spkg/bin/sage:

   if [ "$T " = "spkg " ]; then
       install "" "$@"
       exit $?
   fi

Since this isn't documented, I guess it's not a big deal, but perhaps it could be fixed. Another ticket?

Edit: with the patch at #12606, the error messages don't appear, but it still doesn't work.

Last edited 7 years ago by jhpalmieri (previous) (diff)

Changed 7 years ago by jdemeyer

comment:62 follow-up: Changed 7 years ago by jdemeyer

  • Description modified (diff)

I added a patch to fix

./sage foo.spkg

It now force-installs foo.spkg.

comment:63 in reply to: ↑ 61 Changed 7 years ago by jdemeyer

Replying to jhpalmieri:

The Sage library patch looks okay to me. I'm not completely happy with a feature of both the old and new implementation. In Sage 5.3.rc0, cvxopt (for example) has been upgraded compared to the version on the web page, so if I do install_package('cvxopt'), it tries to download and install the previous version. Should the behavior of standard_packages() be changed? Or should its output be modified before use in install_package(...), taking other (later) versions of packages into account?

How about simply not using standard_packages() at all and simply calling sage -i cvxopt? Then sage-spkg will figure out which version to install (it would install spkg/standard/cvxopt-NNN.spkg)

comment:64 Changed 7 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Fix "sage -info" and a bug when sourcing sage-env more than once to Fix install_package() library function

comment:65 Changed 7 years ago by jdemeyer

* ping *

comment:66 in reply to: ↑ 62 Changed 7 years ago by jhpalmieri

Replying to jdemeyer:

I added a patch to fix

./sage foo.spkg

It now force-installs foo.spkg.

It used to install without forcing. The old behavior seems more natural to me, so I'm suggesting my patch instead. Positive review for the Sage library patch.

How about simply not using standard_packages() at all and simply calling sage -i cvxopt? Then sage-spkg will figure out which version to install (it would install spkg/standard/cvxopt-NNN.spkg)

Sounds fine to me, although let's do this on another ticket.

Changed 7 years ago by jhpalmieri

root repo

comment:67 Changed 7 years ago by jhpalmieri

  • Description modified (diff)

comment:68 Changed 7 years ago by jdemeyer

  • Merged in set to sage-5.4.beta0
  • Resolution set to fixed
  • Status changed from needs_review to closed

Positive review then.

Note: See TracTickets for help on using tickets.