Opened 11 years ago
Last modified 10 years ago
#11021 closed defect
Fix "sage -info" and a bug when sourcing sage-env more than once — at Version 19
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: | |
Authors: | Leif Leonhardy, Kelvin Li | Reviewers: | Kelvin Li, Leif Leonhardy |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | Stopgaps: |
Description (last modified by )
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.
Apply
- trac_11021-sage-spkg-cleanup-v2-rebased_to_4.7.1.alpha4.patch
- trac_11021-additional_changes_to_sage-spkg.scripts.patch (somewhat optional, can be reviewed separately)
- trac_11021-support_and_document_sage_-info_in_sage-sage.scripts.patch
- trac_11021-export_BUILD_in_sage-env.scripts.patch
to the scripts repo.
Change History (23)
comment:1 Changed 11 years ago by
- Status changed from new to needs_review
comment:2 Changed 11 years ago by
comment:3 Changed 11 years ago by
- Description modified (diff)
comment:4 Changed 11 years ago by
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: ↓ 6 Changed 11 years ago by
- 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: ↓ 8 Changed 11 years ago by
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 longSPKG.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 11 years ago by
- Reviewers set to Kelvin Li
comment:8 in reply to: ↑ 6 ; follow-up: ↓ 9 Changed 11 years ago by
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 ofsed -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 11 years ago by
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 ofsed -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: ↓ 11 ↓ 12 Changed 11 years ago by
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 11 years ago by
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 11 years ago by
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 11 years ago by
- Keywords SPKG.txt SAGE.txt -info added
comment:14 follow-up: ↓ 15 Changed 11 years ago by
- 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... :) )
comment:15 in reply to: ↑ 14 Changed 11 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
- Work issues Rebase to 4.7.1.alpha4 deleted
Changed 11 years ago by
SCRIPTS repo. Fix BUILD not being set if sage-env is sourced a second time. Based on Sage 4.7.1.alpha4.
Changed 11 years ago by
SCRIPTS repo. Implements/supports & documents "sage -info ..." in sage-sage; minor fixes. Based on Sage 4.7.1.alpha4.
comment:16 Changed 11 years ago by
- 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 11 years ago by
- 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 11 years ago by
- 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 thatBUILD
is really set, cf. patch tosage-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 thanmake
($MAKE -j1
for installation). spkg-check
is nowtime
d, 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 11 years ago by
SCRIPTS repo. Bug fixes, more error checks, improved messages, restructuring. Apply on top of the rebased v2-patch.
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 statementecho ""
, because that overwrites$?
to0
. I can't figure out the intent of the precedingtar
command, so I don't know how to fix this problem. Could someone help me?