Opened 9 years ago

Closed 9 years ago

#13963 closed defect (fixed)

Fix checking for and resetting GAP workspaces

Reported by: jdemeyer Owned by: GeorgSWeber
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 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: 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 jdemeyer 9 years ago.
gap_packages-4.5.7.p0.diff (728 bytes) - added by jdemeyer 9 years ago.
gap-4.5.7.p2.diff (4.1 KB) - added by jdemeyer 9 years ago.

Download all attachments as: .zip

Change History (31)

comment:1 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 9 years ago by jdemeyer

  • Description modified (diff)

Changed 9 years ago by jdemeyer

comment:3 Changed 9 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Description modified (diff)
  • Status changed from new to needs_review

comment:4 Changed 9 years ago by jdemeyer

  • Summary changed from Failure running Sage when local/share/sage/ext doesn't exist to Sage doesn't start if SAGE_EXTCODE doesn't exist

comment:5 Changed 9 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Sage doesn't start if SAGE_EXTCODE doesn't exist to Fix checking for and resetting GAP workspaces

comment:6 Changed 9 years ago by vbraun

  • Reviewers set to Volker Braun
  • Status changed from needs_review to positive_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 follow-up: Changed 9 years ago by leif

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 ; follow-up: Changed 9 years ago by 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.

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 9 years ago by leif

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 9 years ago by leif

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 9 years ago by leif

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 follow-ups: Changed 9 years ago by vbraun

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 9 years ago by leif

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 9 years ago by jdemeyer

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 follow-up: Changed 9 years ago by vbraun

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 9 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from positive_review to needs_work

comment:17 in reply to: ↑ 15 Changed 9 years ago by jdemeyer

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 9 years ago by jdemeyer

comment:18 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:19 Changed 9 years ago by jdemeyer

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

comment:20 Changed 9 years ago by vbraun

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

comment:21 follow-up: Changed 9 years ago by 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.

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 ; follow-up: Changed 9 years ago by 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"

Changed 9 years ago by jdemeyer

comment:23 Changed 9 years ago by jdemeyer

  • Status changed from needs_work to needs_review

comment:24 Changed 9 years ago by jdemeyer

Can this be reviewed again please?

comment:25 Changed 9 years ago by vbraun

  • Status changed from needs_review to positive_review

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

comment:26 in reply to: ↑ 22 ; follow-up: Changed 9 years ago by leif

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 9 years ago by jdemeyer

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 9 years ago by jdemeyer

  • Merged in set to sage-5.7.beta0
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.