Fix checking for and resetting GAP workspaces

[jdemeyer]

It seems that nothing really guarantees the existence of the directory SAGE_EXTCODE ($SAGE_ROOT/local/share/sage/ext) apart from the installers of some packages.

If Sage is started when that directory doesn't exist:

[...lots of stuff...]
/release/merger/sage-5.7.beta0/local/lib/python2.7/site-packages/sage/interfaces/gap.py in <module>()
   1457 # if the modification time of the gap link has changed (which signals
   1458 # that gap has been somehow upgraded).
-> 1459 if not os.path.exists(WORKSPACE) or os.path.getmtime(WORKSPACE) < os.path.getmtime(GAP_STAMP):
   1460     #print "Automatically updating the cached Gap workspace:"
   1461     #print WORKSPACE

/release/merger/sage-5.7.beta0/local/lib/python/genericpath.py in getmtime(filename)
     52 def getmtime(filename):
     53     """Return the last modification time of a file, reported by os.stat()."""
---> 54     return os.stat(filename).st_mtime
     55
     56

This is a regression because conway_polynomials from #12205 runs Sage code at build time at a time when the existence of SAGE_EXTCODE is not guaranteed.

The attached patch solves this problem in two ways:

• It only checks for and generates workspaces when actually starting GAP, not when Sage is started.
• Instead of using SAGE_EXTCODE (which doesn't really make sense), check the modification time of $SAGE_LOCAL/bin/gap.

Apply: attachment: 13963_gap_stamp.patch

spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/gap-4.5.7.p2.spkg (diff: attachment: gap-4.5.7.p2.diff)

optional spkg: http://boxen.math.washington.edu/home/jdemeyer/spkg/gap_packages-4.5.7.p0.spkg (diff: attachment: gap_packages-4.5.7.p0.diff)

Component: build

Author: Jeroen Demeyer

Reviewer: Volker Braun

Merged: sage-5.7.beta0

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

[jdemeyer]

Attachment: 13963_gap_stamp.patch.gz

[jdemeyer]

Author: Jeroen Demeyer

[vbraun]

comment:6

+1

In the long run it would also be nice to have just a single cache dir with a single hashing scheme for SAGE_ROOT. Right now we have at least

~/.sage/gap/workspace-1816283156629376178
~/.sage/cache/_home_vbraun_opt_sage-5.6.rc0_devel_sage-main-lazy_import_cache.pickle

[vbraun]

Reviewer: Volker Braun

[nexttime]

comment:7

Minor comments:

• GAP_BINARY is misleading, as $SAGE_LOCAL/bin/gap is a shell script.

• A failure upon creation of README.txt is silently ignored in the first place. (This has been in before.)

• The created README could contain a note on the automatic deletion of old GAP workspaces.

Avoiding to think of the race conditions we may run into here... ;-)

[jdemeyer]

comment:8

Replying to @nexttime:

Minor comments:

• GAP_BINARY is misleading, as $SAGE_LOCAL/bin/gap is a shell script.

I think that the name GAP_SCRIPT would be even more misleading. Besides, for this code it doesn't really matter whether it's a binary or a script, it's only needed to check the time when GAP was installed.

Avoiding to think of the race conditions we may run into here... ;-)

It mainly depends on how GAP's SaveWorkspace() is implemented because that would be the most obvious place to check for race conditions.

[nexttime]

comment:9

P.S.:

GAP_DIR (here meaning the directory for GAP workspaces) should also get renamed, as that name is already used for the directory where GAP lives, in other contexts of course.

[nexttime]

comment:10

Replying to @jdemeyer:

Replying to @nexttime:

Minor comments:

• GAP_BINARY is misleading, as $SAGE_LOCAL/bin/gap is a shell script.

I think that the name GAP_SCRIPT would be even more misleading. Besides, for this code it doesn't really matter whether it's a binary or a script, it's only needed to check the time when GAP was installed.

Well, a binary is more likely to change upon reinstallation than some (more or less generic) script.

(Haven't checked whether it actually gets [re-]created by the GAP spkg.)

[nexttime]

comment:11

I'd say this needs work, as in GAP's most recent spkg-install we have:

# Indicate that GAP has somehow been updated, which invalidates all workspaces:
touch "$SAGE_LOCAL/bin/gap_stamp"

[vbraun]

comment:12

The gap_stamp gets touched by the gap and gap_packages spkgs. Putting it in /bin was of course a bad call.

[nexttime]

comment:13

Replying to @vbraun:

The gap_stamp gets touched by the gap and gap_packages spkgs. Putting it in /bin was of course a bad call.

No matter what we use as the stamp, it wouldn't be bad to use the same in GAP-related spkgs as well as the Sage library... ;-)

[jdemeyer]

comment:14

Replying to @vbraun:

The gap_stamp gets touched by the gap and gap_packages spkgs.

Does installing a new GAP package require a rebuild of the workspace?

[vbraun]

comment:15

Yes... just the other day I spent a fun few hours figuring out why some GAP command mysteriously crashes. Wrong gap workspace %-)

[jdemeyer]

comment:17

Replying to @vbraun:

Yes... just the other day I spent a fun few hours figuring out why some GAP command mysteriously crashes. Wrong gap workspace %-)

Any suggestions for a way of properly checking whether a new package was installed?

[jdemeyer]

comment:18

Attachment: gap_packages-4.5.7.p0.diff.gz

[jdemeyer]

comment:19

This seems the best solution, although it is not very elegant.

[vbraun]

comment:20

How about we check the date of gap_stamp for rebuilding the workspace, thats what its meant for.

[nexttime]

comment:21

I'd still let -O0 override a user's O-level (in $CFLAGS) if SAGE_DEBUG=yes, like most -- if not all -- other spkgs do.

And while you're at it, change the == to = there, and/or use [[ ... ]].

There are also a few error checks missing, and SPKG_ROOT isn't always quoted. (One should also probably do some sanity check on $VERSION at least.)

The -f in ln -sf ... latest is redundant, as we always first delete the symbolic link (if present).

[jdemeyer]

comment:22

Replying to @nexttime:

I'd still let -O0 override a user's O-level (in $CFLAGS) if SAGE_DEBUG=yes, like most -- if not all -- other spkgs do.

I don't think Sage should ever override user-chosen flags, so we should always do

CFLAGS="-sage -specific -flags $CFLAGS"

[jdemeyer]

Attachment: gap-4.5.7.p2.diff.gz

[jdemeyer]

comment:24

Can this be reviewed again please?

[vbraun]

comment:25

Its still fugly to touch bin/gap but at least it works. Cute trick with autoconf --trace...

[nexttime]

comment:26

Replying to @jdemeyer:

Replying to @nexttime:

I'd still let -O0 override a user's O-level (in $CFLAGS) if SAGE_DEBUG=yes, like most -- if not all -- other spkgs do.

I don't think Sage should ever override user-chosen flags, so we should always do

CFLAGS="-sage -specific -flags $CFLAGS"

Well, IMHO depends. If -foo is mandatory to not break the build, it should be appended; same for options to configure. (-L is even more troublesome, and -I- for example isn't portable.)

Unfortunately there's not always an option to toggle or invalidate a previously (i.e., "user-specified") option, and flags may even conflict. (The same is true for the opposite case, when a user wants to override some option added by Sage or an spkg's build system. This especially holds for -g, which we almost always add by default -- thereby probably already overriding a user's choice; stripping afterwards may be inappropriate in some cases.)

If a user sets SAGE_DEBUG=yes, he will or should be aware of the consequences, which are documented (I think), and usually subject of a corresponding warning message. If someone really wants to do debugging without e.g. -O0, he'll certainly be able to modify spkg-install for that purpose, which is trivial. Temporarily modifying C*FLAGS is more tedious, and less experienced users may have (more or less intentionally) some of them set, while still expecting SAGE_DEBUG to work as advertised, without having to mess with other environment variables.

Ideally, we'd have some SAGE_DEBUG_LEVEL, or we'd at least separate "standard" compiler options probably useful for debugging (such as -O0) from usually package-specific ones, like -DDEBUG_FOO; configure may also take special debug options specific to the package (which IMHO is the main reason we do have SAGE_DEBUG).

Until we have SAGE_DEBUG_CFLAGS, say, which could easily be set/modified by the user, I'd still (try to) override e.g. -O2 by default (if SAGE_DEBUG=yes of course).

[jdemeyer]

comment:27

Replying to @nexttime:

If -foo is mandatory to not break the build, it should be appended

I don't think there are many situations where it's known that a flag is absolutely required to not break the build. I like the principle of "the user knows best" and not "the Sage developers know best".

This especially holds for -g

The -g option can be cancelled by -g0.

If a user sets SAGE_DEBUG=yes, he will or should be aware of the consequences

Sure, but exactly the same holds for CFLAGS.

If someone really wants to do debugging without e.g. -O0, he'll certainly be able to modify spkg-install for that purpose, which is trivial.

So now we require users to modify a spkg to get the options they want? Why make things hard which should be easy?

Temporarily modifying C*FLAGS is more tedious

Why that? Changing CFLAGS is equally easy as changing SAGE_DEBUG.

and less experienced users may have (more or less intentionally) some of them set

Why do you think that? Are there any distributions which have CFLAGS set in the environment by default?

[jdemeyer]

Merged: sage-5.7.beta0