Stop greping for a non-existent sage-banner #9434

sagetrac-drkirkby · 2010-07-06T06:50:04Z

In install.log, we often see:

drkirkby@hawk:~/f/sage-4.5.alpha3$ grep sage-banner install.log
grep: can't open /export/home/drkirkby/f/sage-4.5.alpha3/local/bin/sage-banner
grep: can't open /export/home/drkirkby/f/sage-4.5.alpha3/local/bin/sage-banner
grep: can't open /export/home/drkirkby/f/sage-4.5.alpha3/local/bin/sage-banner
grep: can't open /export/home/drkirkby/f/sage-4.5.alpha3/local/bin/sage-banner
sage_scripts-4.5.alpha3/.hg/store/data/sage-banner.i
sage_scripts-4.5.alpha3/sage-banner
drkirkby@hawk:~/f/sage-4.5.alpha3$

I think this is probably due to some code in sage-sage

if [ "$1" = '-v' -o "$1" = '-version' -o "$1" = '--version' ]; then
    cat "$SAGE_LOCAL/bin/sage-banner" | grep -i "version" | sed "s/\| //" | sed "s/ *\|//"
    exit $?
fi

This will obviously fail if sage-banner does not exist.

Also

There is an useless use of 'cat'. Perhaps the author was hoping to get a Useless Use of Cat Award (Well worth a read - it's both funny and educational.)
There is an an unnecessary use of double-quotes around 'version'.

The following will save a few bytes of disk space and a few CPU cycles, as it will invoke one less process.

    grep -i version "$SAGE_LOCAL/bin/sage-banner" | sed "s/\| //" | sed "s/ *\|//"

More importantly, we should check that sage-banner exists before doing this, so it does not produce potentially confusing error messages.

Dave

CC: @jhpalmieri

Component: build

Keywords: scripts VERSION.txt

Author: John Palmieri

Reviewer: Jeroen Demeyer, David Kirkby

Merged: sage-4.6.1.rc0

Issue created by migration from https://trac.sagemath.org/ticket/9434

The text was updated successfully, but these errors were encountered:

nexttime · 2010-07-06T11:38:39Z

comment:1

Well, reading carefully, your error messages can't come from sage-sage... (I know you haven't had much sleep... ;-) )

Btw, there are more superfluous quotes.

If you want to save another "redundant" process invocation (there are in general many), at the expense of losing some parallelism, substitute

... | sed "s/\| //" | sed "s/ *\|//"

by

... | sed -e "s/\| //" -e "s/ *\|//"

(The whole line could be replaced by a single invocation of sed.)

nexttime · 2010-07-10T05:47:35Z

comment:2

Just for the record:

grep -c "^grep:" install.log

gives me zero for both sequential and parallel builds of Sage 4.5.alpha4 (Ubuntu 9.04).

sagetrac-drkirkby · 2010-07-10T06:45:20Z

comment:3

Hi leif

I was not aware of that shorter sed sequence. I just found the 'cat' a bit amuzing.
Yes, there are loads of unneeded quotes in this bit of Sage.
Interesting that you don't see this error message - I'm not the only one that gets it. I forget who posted the log with this, and commented on it, but someone did.

I'll have to look at this, but its not exactly the most serious Sage bug.

Dave

nexttime · 2010-07-10T09:14:39Z

comment:4

Replying to @sagetrac-drkirkby:

I was not aware of that shorter sed sequence. I just found the 'cat' a bit amuzing.

Yes, though I love cats. One of my favorites is:

sed -n "/[Vv]ersion/s/ *| *//gp" $SAGE_LOCAL/bin/sage-banner

(Spaces in $SAGE_LOCAL are forbidden, otherwise we'd need quotes there.)

Note that the original version gives two lines for non-finals; it does not remove the vertical bars (nor the whitespace) because the pipe symbol is "superfluously" escaped: :)

leif64@portland:~/Sage/sage-4.5.alpha4-serial$ ./sage -v
| Sage Version 4.5.alpha4, Release Date: 2010-07-06                  |
* Warning: this is a prerelease version, and it may be unstable.     *

More important, the discussion is still somewhat off-topic or off-ticket, since - as mentioned - the error messages do not originate from sage-sage, so the issue has to be fixed elsewhere.

I'll have to look at this, but its not exactly the most serious Sage bug.

Obviously, I agree.

nexttime · 2010-07-12T23:10:56Z

comment:5

I think I've found the real culprit:

    # Mark that the new package has been installed. 
    # This file will eventually be a certificate like in OS X.
    echo "PACKAGE NAME: $PKG_NAME" > "$PKG_NAME"
    echo "INSTALL DATE: `date`" >> "$PKG_NAME"
    echo "UNAME: `uname -a`" >> "$PKG_NAME"
    echo "Sage VERSION: `grep Sage $SAGE_LOCAL/bin/sage-banner`" >> "$PKG_NAME"
    echo "Successfully installed $PKG_NAME"

(This is from sage-spkg.)

sagetrac-drkirkby · 2010-07-12T23:24:53Z

comment:6

Replying to @nexttime:

I think I've found the real culprit:

    # Mark that the new package has been installed. 
    # This file will eventually be a certificate like in OS X.
    echo "PACKAGE NAME: $PKG_NAME" > "$PKG_NAME"
    echo "INSTALL DATE: `date`" >> "$PKG_NAME"
    echo "UNAME: `uname -a`" >> "$PKG_NAME"
    echo "Sage VERSION: `grep Sage $SAGE_LOCAL/bin/sage-banner`" >> "$PKG_NAME"
    echo "Successfully installed $PKG_NAME"

(This is from sage-spkg.)

Yes, it looks like you have. It seems the author was not trying to win a Useless Use of Cat Award.

Do you have any idea why some people do not see this error message? If you can produce a patch, I can test it.

Dave

jhpalmieri · 2010-07-12T23:38:43Z

comment:7

Since sage-spkg is in spkg/base and in sage_scripts, while sage-banner is only in sage_scripts, you should see this message for any spkgs installed before sage-scripts. In the most recent version of Sage, deps should install sage_scripts right at the beginning:

BASE = $(INST)/$(PREREQ) $(INST)/$(DIR) $(INST)/$(SAGE_BZIP2)

# Also install scripts before we continue with other spkgs
BASE += $(INST)/$(SAGE_SCRIPTS)

So I hope this problem has already been taken care of. Installing sage_scripts itself looks like the only possible problem.

nexttime · 2010-07-12T23:58:23Z

comment:8

Replying to @sagetrac-drkirkby:

If you can produce a patch, I can test it.

Milestone: Sage 5.0 ;-)

I think a file containing (just) the Sage version number should be in $SAGE_ROOT anyhow; then we could cat that. (Testing for the existence of files is though not a bad idea...)

jhpalmieri · 2010-09-29T21:03:36Z

comment:9

Here's an attempt at fixing this. Three comments:

It stores the current Sage version in SAGE_ROOT/VERSION.txt, as suggested by Leif -- see Add release notes to SAGE_ROOT #9922.
Right now, upgrading just adds the new version to the top, with "Updated from" and then the old version, so if you've somehow managed to update 6 times, the file will have 7 lines in it, recording the full update history.
It uses cat. Sorry, Dave.

jhpalmieri · 2010-09-29T21:08:31Z

comment:10

To be more precise: with the patch, in a non-upgrade, the file SAGE_ROOT/VERSION.txt will look like this:

Sage version: 4.6.alpha2, Release date: 2010-09-29

I haven't tested the upgrade script yet, but it should produce

Sage version: 4.6.alpha2, Release date: 2010-09-29
Upgraded from Sage version: 4.6.alpha1, Release date: 2010-09-18
Upgraded from ...

nexttime · 2010-09-29T21:27:16Z

comment:11

Hmmm, besides I don't like the date format string (which is local time), I don't think $SAGE_RELEASE_DATE is available (i.e. set) in sage-upgrade.

Do you intentionally export OLD_VERSION?

A non-existing $SAGE_ROOT/VERSION.txt could be handled as well.

I'd prefer having "Sage version: ... [Last] Upgraded from: ..." on a single line (at least in the log).

nexttime · 2010-09-29T21:37:59Z

comment:12

You could use

    ...
    if [ -f "$SAGE_ROOT/VERSION.txt" ]; then
        sed -i -e "1iSage version: $SAGE_VERSION, Release date: $SAGE_RELEASE_DATE\nUpdated from $OLD_VERSION"
    else
        ...

to avoid cat ... ;-)

(Perhaps omit the newline, i.e. replace it by e.g. two spaces.)

nexttime · 2010-09-29T21:42:08Z

comment:13

And we'd have to make sure the version file doesn't get overwritten later once we have the root repo.

jhpalmieri · 2010-09-29T22:09:28Z

comment:14

Replying to @nexttime:

Hmmm, besides I don't like the date format string (which is local time), I don't think $SAGE_RELEASE_DATE is available (i.e. set) in sage-upgrade.

Do you intentionally export OLD_VERSION?

Right, I'll fix these.

A non-existing $SAGE_ROOT/VERSION.txt could be handled as well.

I'd prefer having "Sage version: ... [Last] Upgraded from: ..." on a single line (at least in the log).

Note that the Sage version doesn't appear in the log: it should only appear in the files in spkg/installed/. But maybe a single line is cleaner.

And we'd have to make sure the version file doesn't get overwritten later once we have the root repo.

We can add VERSION.txt to .hgignore; I think that should do it.

jhpalmieri · 2010-09-29T22:11:31Z

scripts repo

jhpalmieri · 2010-09-29T22:12:45Z

comment:16

Attachment: trac_9434-VERSION.txt.patch.gz

The new patch also modifies sage-make_devel_packages: when preparing the scripts package, it no longer indiscriminately copies all *.txt files from SAGE_ROOT into the scripts spkg, it just copies COPYING.txt and README.txt. (Thus it won't copy VERSION.txt.)

nexttime · 2010-09-29T23:38:51Z

comment:17

I wouldn't call sage in sage-upgrade. Also, sage-spkg is run before sage-upgrade updates VERSION.txt, so (just) the old version would be logged. (I consider the files in spkg/installed/ also logs, though they have no .log extension.)

Rather than omitting VERSION.txt from the sage_scripts spkg, I would put the code to update (or better: not overwrite) an existing VERSION.txt in its spkg-install.

As noted, the version file should be updated before spkg/install is called (and that's also before a new scripts spkg gets installed).

We'd have to extract the new version from some newly downloaded file. (This only works for later Sage versions anyway, unless we handle it in spkg/install, too.)

"While you're at it"^TM, would you mind quoting more instances of $SAGE_ROOT etc.?

nexttime · 2010-09-29T23:49:28Z

comment:18

P.S.: See also #9905 (for the date format).

sagetrac-drkirkby · 2010-09-30T05:17:15Z

comment:19

Replying to @nexttime:

You could use

    ...
    if [ -f "$SAGE_ROOT/VERSION.txt" ]; then
        sed -i -e "1iSage version: $SAGE_VERSION, Release date: $SAGE_RELEASE_DATE\nUpdated from $OLD_VERSION"
    else
        ...

to avoid cat ... ;-)

(Perhaps omit the newline, i.e. replace it by e.g. two spaces.)

But that would be an even bigger mistake than to use an unnecessary cat, as you are making use of non-POSIX options to sed - see POSIX specifiction of sed I can guarantee that will fail on Solaris and AIX and probably other Unix systems too.

Dave

jhpalmieri · 2010-09-30T05:46:49Z

comment:20

Replying to @nexttime:

I wouldn't call sage in sage-upgrade.

I can see from your other reasons that we need to update the version number earlier so my solution won't work, but I see no reason, a priori, not to call sage in sage-upgrade, after the upgrade process has completed, apparently successfully. It actually tests further whether the upgrade succeeded.

Also, sage-spkg is run before sage-upgrade updates VERSION.txt, so (just) the old version would be logged. (I consider the files in spkg/installed/ also logs, though they have no .log extension.)

That makes sense.

Rather than omitting VERSION.txt from the sage_scripts spkg, I would put the code to update (or better: not overwrite) an existing VERSION.txt in its spkg-install.

Why? In general, copying over all *.txt files is a little dangerous, and is part of the reason that the file sage-README-osx.txt appears in the wrong places and isn't currently tracked in any repo. I think if we want to include a file, it should be tracked. If it's automatically generated, I don't see any reason to include it in a repo. If #9433 ever gets reviewed and merged, then all of the *.txt files in SAGE_ROOT will be taken out of the scripts repo in any case.

As noted, the version file should be updated before spkg/install is called (and that's also before a new scripts spkg gets installed).

Right.

We'd have to extract the new version from some newly downloaded file. (This only works for later Sage versions anyway, unless we handle it in spkg/install, too.)

How about extracting it from VERSION.txt? I'm attaching a patch which does this, perhaps not in the optimal way, but I think it should work.

"While you're at it"^TM, would you mind quoting more instances of $SAGE_ROOT etc.?

I don't have time to do this now.

qed777 · 2010-12-14T15:25:07Z

comment:40

Using download_file("../VERSION.txt") may also work (I've tested this only once).

For me, the upgrades that failed were parallel spkg builds. But some parallel spkg upgrades did "succeed," i.e., modulo SageNB not being upgraded. (Were there any other packages not upgraded?)
All purely serial and parallel-only-within-spkg upgrades "succeeded."

Could when the new sage_scripts package is installed matter for either the VERSION.txt discrepancy or the apparently separate SageNB problem?

jdemeyer · 2010-12-14T16:05:11Z

comment:41

Replying to @qed777:

Using download_file("../VERSION.txt") may also work (I've tested this only once).

If that's true, it's probably a better solution (but it needs to be tested properly).

jhpalmieri · 2010-12-14T18:50:10Z

comment:42

Replying to @qed777:

Using download_file("../VERSION.txt") may also work (I've tested this only once).

I would expect it to work from an upgrade path like the one I provided, or like the ones provided by release managers for alpha releases. But if I try to download from a URL like "http://boxen.math.washington.edu/sage/spkg/../COPYING.txt", the file is not found. I think the same would be true of VERSION.txt: I don't think the top-level directory is available on the official Sage mirrors.

jdemeyer · 2010-12-14T19:15:15Z

comment:43

Replying to @jhpalmieri:

Replying to @qed777:

Using download_file("../VERSION.txt") may also work (I've tested this only once).

I would expect it to work from an upgrade path like the one I provided, or like the ones provided by release managers for alpha releases. But if I try to download from a URL like "http://boxen.math.washington.edu/sage/spkg/../COPYING.txt", the file is not found.

Well, the file is not found because it doesn't actually exist. So your comment doesn't really prove anything.

jdemeyer · 2010-12-14T19:27:18Z

comment:44

I don't see any reason why http://boxen.math.washington.edu/sage/spkg/../COPYING.txt shouldn't work while John's URL works.

jhpalmieri · 2010-12-14T19:54:51Z

comment:45

Replying to @jdemeyer:

I don't see any reason why http://boxen.math.washington.edu/sage/spkg/../COPYING.txt shouldn't work while John's URL works.

I think that on the official Sage mirrors, none of the top-level files (makefile or Makefile, COPYING.txt, etc.) are available for download via the URLs in the sage-update script. So if we have VERSION.txt only in the top-level, it won't be available for download (unless the scripts which make the official release available are rewritten). That is, after Sage 4.6.1 is released, running "sage -upgrade" with no arguments will fail to download VERSION.txt because that file won't be anywhere on the server.

I could be wrong, though. Does anyone know for sure what files are mirrored on the official servers? Browsing around the /home/sagemath directory on sage.math, for example the www-files subdirectory, I don't see 'makefile' anywhere...

jhpalmieri · 2010-12-14T19:59:34Z

comment:46

Here is the patch quoted above. I think this will work, and I don't think that downloading "../VERSION.txt" will. I'm marking it as needing review, but if anyone wants to try a different approach, please go ahead.

jdemeyer · 2010-12-14T20:12:45Z

comment:47

Replying to @jhpalmieri:

I could be wrong, though. Does anyone know for sure what files are mirrored on the official servers? Browsing around the /home/sagemath directory on sage.math, for example the www-files subdirectory, I don't see 'makefile' anywhere...

Does this mean that Makefile will never get upgraded? That doesn't sound good, especially given #9799, #10156.

jhpalmieri · 2010-12-14T23:01:09Z

comment:48

Well, I think Makefile never gets run during an upgrade, so while it doesn't get upgraded, that doesn't affect the upgrade process. I suppose if someone ran "sage -upgrade ..." and then did "make clean" and "make", it would use the old version of Makefile (or makefile), but the spkgs should be up to date. (This is yet another reason to work on getting #9433 merged...)

nexttime · 2010-12-16T03:38:16Z

comment:49

Replying to @qed777:

I've reported on sage-devel a possible problem involving VERSION.txt and failed upgrades.

Just for the record (from IRC):

<kini>
  er... I just ran `sage -upgrade` (from 4.6) to see if it would pull a development version
  - sage told me that everything was already upgraded, and it exited without seeming to do
  anything but now when I load sage it's telling me I have 4.6.rc0
  odd
  curiouser and curiouser
  sage.misc.banner.banner() returns the correct, 4.6 banner
  ... hm
  so apparently `sage -version` just cats a text file,  $SAGE_ROOT/local/bin/sage-banner
  odd, my $SAGE_ROOT/local/bin repository's files are for some reason marked as descended
  from 4.6.rc0
  well, never mind

jdemeyer · 2010-12-16T09:02:22Z

comment:50

Why not rename/copy the file "SAGE_ROOT/spkg/standard/.VERSION.txt" to "SAGE_ROOT/VERSION.txt" in spkg-upgrade? That way, there will be a top-level VERSION.txt just like one would have with a clean build.
I would not make the file hidden, I don't think there is a problem with a file spkg/standard/VERSION.txt.
I would copy using cp -p.

jhpalmieri · 2010-12-16T21:25:38Z

comment:51

Replying to @jdemeyer:

Why not rename/copy the file "SAGE_ROOT/spkg/standard/.VERSION.txt" to "SAGE_ROOT/VERSION.txt" in spkg-upgrade? That way, there will be a top-level VERSION.txt just like one would have with a clean build.

I agree that the top-level VERSION.txt should be the same as spkg/standard/VERSION.txt, which was not the case in my previous patch. I've fixed that so that once the top-level file is created, it's copied to spkg/standard. (The top-level file contains upgrade information, and since upgrade info may be helpful in tracking down problems, we should keep that one rather than the downloaded one.)

I would not make the file hidden, I don't think there is a problem with a file spkg/standard/VERSION.txt.

Okay.

I would copy using cp -p.

Good idea.

I'm attaching two patches, one of which just fixes these issues. The other patch, which could be applied on top of the others, does a little miscellaneous clean-up in sage-update, using os.path.join(x,y) instead of "%s/%s" %(x,y). Feel free to ignore the second patch completely; it is entirely optional right now.

jhpalmieri · 2010-12-16T21:26:11Z

Attachment: trac_9434-VERSION-update.patch.gz

scripts repo; apply on top of other patches

jhpalmieri · 2010-12-16T21:27:08Z

Attachment: trac_9434-os.path.join.patch.gz

scripts repo; this patch is optional, so reviewing it should have lower priority

jdemeyer · 2010-12-17T14:20:20Z

Changed work issues from Fix download of VERSION.txt to none

jdemeyer · 2010-12-17T14:20:20Z

comment:52

The patches look good to me, but I need to do more testing before this can get positive review.

jhpalmieri · 2010-12-17T16:28:59Z

comment:53

I just created new upgrade paths for further testing: I took a vanilla version of 4.6.1.alpha3 and applied "trac_9434-VERSION-update.patch", then did "./sage -sdist 4.6.1.V0" and "./sage -sdist 4.6.1.V1". Then I applied "trac_9434-os.path.join.patch" and did "./sage -sdist 4.6.1.W0" and "./sage -sdist 4.6.1.W1". The resulting upgrade paths are

http://sage.math.washington.edu/home/palmieri/misc/sage-4.6.1.V0/
http://sage.math.washington.edu/home/palmieri/misc/sage-4.6.1.V1/
http://sage.math.washington.edu/home/palmieri/misc/sage-4.6.1.W0/ (with os.path.join patch)
http://sage.math.washington.edu/home/palmieri/misc/sage-4.6.1.W1/ (with os.path.join patch)

jdemeyer · 2010-12-18T09:36:22Z

comment:54

I'm also testing with the upgrade path http://sage.math.washington.edu/home/release/sage-4.6.1.rc0/sage-4.6.1.rc0/ (including also other tickets such as #10176).

I currently get the following problem (building an "nameless" package), still investigating:

/mnt/usb1/scratch/jdemeyer/sage-4.6.1.rc0_upgraded/spkg/pipestatus "sage-spkg ${SAGE_SPKG_OPTS}  2>&1" "tee -a /mnt/usb1/scratch/jdemeyer/sage-4.6.1.rc0_upgraded/spkg/logs/.log"

jdemeyer · 2010-12-19T09:33:33Z

comment:55

Looks good to me ("merged" changed to rc0 because there is an additional patch).

jdemeyer · 2010-12-19T09:33:33Z

Changed merged from sage-4.6.1.alpha3 to sage-4.6.1.rc0

sagetrac-drkirkby mannequin added this to the sage-5.0 milestone Jul 6, 2010

sagetrac-drkirkby mannequin added c: build labels Jul 6, 2010

sagetrac-drkirkby mannequin assigned sagetrac-GeorgSWeber Jul 6, 2010

sagetrac-drkirkby mannequin assigned sagetrac-drkirkby and unassigned sagetrac-GeorgSWeber Jul 10, 2010

jhpalmieri added the s: needs review label Sep 29, 2010

jhpalmieri modified the milestones: sage-5.0, sage-4.6 Sep 29, 2010

jdemeyer added the s: needs work label Dec 14, 2010

jhpalmieri added s: needs review and removed s: needs work labels Dec 14, 2010

jdemeyer added s: needs work and removed s: needs review labels Dec 16, 2010

jhpalmieri added s: needs review and removed s: needs work labels Dec 16, 2010

jdemeyer removed the s: needs review label Dec 19, 2010

jdemeyer closed this as completed Dec 19, 2010

nexttime mannequin mentioned this issue Sep 9, 2010

Change Cliquer compiler flags on Solaris to build without text relocations. #9871

Closed

Stop greping for a non-existent sage-banner #9434

Stop greping for a non-existent sage-banner #9434

Comments

sagetrac-drkirkby mannequin commented Jul 6, 2010

nexttime mannequin commented Jul 6, 2010

nexttime mannequin commented Jul 10, 2010

sagetrac-drkirkby mannequin commented Jul 10, 2010

nexttime mannequin commented Jul 10, 2010

nexttime mannequin commented Jul 12, 2010

sagetrac-drkirkby mannequin commented Jul 12, 2010

jhpalmieri commented Jul 12, 2010

nexttime mannequin commented Jul 12, 2010

jhpalmieri commented Sep 29, 2010

jhpalmieri commented Sep 29, 2010

nexttime mannequin commented Sep 29, 2010

nexttime mannequin commented Sep 29, 2010

nexttime mannequin commented Sep 29, 2010

jhpalmieri commented Sep 29, 2010

jhpalmieri commented Sep 29, 2010

jhpalmieri commented Sep 29, 2010

nexttime mannequin commented Sep 29, 2010

nexttime mannequin commented Sep 29, 2010

sagetrac-drkirkby mannequin commented Sep 30, 2010

jhpalmieri commented Sep 30, 2010

qed777 mannequin commented Dec 14, 2010

jdemeyer commented Dec 14, 2010

jhpalmieri commented Dec 14, 2010

jdemeyer commented Dec 14, 2010

jdemeyer commented Dec 14, 2010

jhpalmieri commented Dec 14, 2010

jhpalmieri commented Dec 14, 2010

jdemeyer commented Dec 14, 2010

jhpalmieri commented Dec 14, 2010

nexttime mannequin commented Dec 16, 2010

jdemeyer commented Dec 16, 2010

jhpalmieri commented Dec 16, 2010

jhpalmieri commented Dec 16, 2010

jhpalmieri commented Dec 16, 2010

jdemeyer commented Dec 17, 2010

jdemeyer commented Dec 17, 2010

jhpalmieri commented Dec 17, 2010

jdemeyer commented Dec 18, 2010

jdemeyer commented Dec 19, 2010

jdemeyer commented Dec 19, 2010