Opened 10 years ago

Closed 10 years ago

#13963 closed defect (fixed)

Fix checking for and resetting GAP workspaces

Reported by: Jeroen Demeyer Owned by: Georg S. Weber
Priority: critical Milestone: sage-5.7
Component: build Keywords:
Cc: Merged in: sage-5.7.beta0
Authors: Jeroen Demeyer Reviewers: Volker Braun
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by Jeroen Demeyer)

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: 13963_gap_stamp.patch

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

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

Attachments (3)

13963_gap_stamp.patch (5.7 KB) - added by Jeroen Demeyer 10 years ago.
gap_packages-4.5.7.p0.diff (728 bytes) - added by Jeroen Demeyer 10 years ago.
gap-4.5.7.p2.diff (4.1 KB) - added by Jeroen Demeyer 10 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:2 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

Changed 10 years ago by Jeroen Demeyer

Attachment: 13963_gap_stamp.patch added

comment:3 Changed 10 years ago by Jeroen Demeyer

Authors: Jeroen Demeyer
Description: modified (diff)
Status: newneeds_review

comment:4 Changed 10 years ago by Jeroen Demeyer

Summary: Failure running Sage when local/share/sage/ext doesn't existSage doesn't start if SAGE_EXTCODE doesn't exist

comment:5 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)
Summary: Sage doesn't start if SAGE_EXTCODE doesn't existFix checking for and resetting GAP workspaces

comment:6 Changed 10 years ago by Volker Braun

Reviewers: Volker Braun
Status: needs_reviewpositive_review

+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

comment:7 Changed 10 years ago by Leif Leonhardy

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... ;-)

comment:8 in reply to:  7 ; Changed 10 years ago by Jeroen Demeyer

Replying to leif:

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.

comment:9 Changed 10 years ago by Leif Leonhardy

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.

comment:10 in reply to:  8 Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to leif:

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.)

comment:11 Changed 10 years ago by Leif Leonhardy

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"

comment:12 Changed 10 years ago by Volker Braun

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

comment:13 in reply to:  12 Changed 10 years ago by Leif Leonhardy

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... ;-)

comment:14 in reply to:  12 Changed 10 years ago by Jeroen Demeyer

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?

comment:15 Changed 10 years ago by Volker Braun

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

comment:16 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)
Status: positive_reviewneeds_work

comment:17 in reply to:  15 Changed 10 years ago by Jeroen Demeyer

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?

Changed 10 years ago by Jeroen Demeyer

Attachment: gap_packages-4.5.7.p0.diff added

comment:18 Changed 10 years ago by Jeroen Demeyer

Description: modified (diff)

comment:19 Changed 10 years ago by Jeroen Demeyer

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

comment:20 Changed 10 years ago by Volker Braun

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

comment:21 Changed 10 years ago by Leif Leonhardy

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).

comment:22 in reply to:  21 ; Changed 10 years ago by Jeroen Demeyer

Replying to leif:

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"

Changed 10 years ago by Jeroen Demeyer

Attachment: gap-4.5.7.p2.diff added

comment:23 Changed 10 years ago by Jeroen Demeyer

Status: needs_workneeds_review

comment:24 Changed 10 years ago by Jeroen Demeyer

Can this be reviewed again please?

comment:25 Changed 10 years ago by Volker Braun

Status: needs_reviewpositive_review

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

comment:26 in reply to:  22 ; Changed 10 years ago by Leif Leonhardy

Replying to jdemeyer:

Replying to leif:

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).

comment:27 in reply to:  26 Changed 10 years ago by Jeroen Demeyer

Replying to leif:

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?

comment:28 Changed 10 years ago by Jeroen Demeyer

Merged in: sage-5.7.beta0
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.