Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#4949 closed enhancement (fixed)

Optionally build spkgs in $SAGE_BUILD_DIR

Reported by: mabshoff Owned by: mabshoff
Priority: minor Milestone: sage-5.0
Component: build Keywords: sd32
Cc: drkirkby, leif Merged in: sage-5.0.beta5
Authors: John Palmieri Reviewers: Mariah Lenox, Leif Leonhardy, Maarten Derickx, Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

$SAGE_ROOT/spkg/build can be slow in case it is NFS-mounted for example. So using local scratch space or even better a RAM disk should speed up the build by a nice factor. To do so, use $SAGE_BUILD_DIR instead of $SAGE_ROOT/spkg/build/.


Apply trac_4949-root.v5.patch and 4949_review.patch to the Sage root repository.

Apply trac_4949-installation.v3.patch to the Sage library.

Attachments (15)

trac_4949-scripts.patch (2.7 KB) - added by jhpalmieri 8 years ago.
scripts repo
trac_4949-installation.patch (3.5 KB) - added by jhpalmieri 8 years ago.
sage repo: update installation guide
trac_4949-root.patch (3.1 KB) - added by jhpalmieri 8 years ago.
root repo
trac_4949-root.v2.patch (4.7 KB) - added by jhpalmieri 8 years ago.
root repo
trac_4949-root.v3.patch (5.5 KB) - added by jhpalmieri 8 years ago.
root repo
trac_4949-root-delta2to3.patch (1.6 KB) - added by jhpalmieri 8 years ago.
root repo: diff between v2 and v3
trac_4949-installation.v2.patch (3.8 KB) - added by jhpalmieri 8 years ago.
sage repo: update installation guide
trac_4949-installation-delta1to2.patch (2.6 KB) - added by jhpalmieri 8 years ago.
sage repo: diff between original and v2
trac_4949-installation-delta2to3.patch (2.4 KB) - added by jhpalmieri 8 years ago.
installation guide: diff between v2 and v3
trac_4949-installation.v3.patch (3.8 KB) - added by jhpalmieri 8 years ago.
sage repo: update installation guide
trac_4949-root-delta3to4.patch (6.6 KB) - added by jhpalmieri 8 years ago.
root repo: diff between v3 and v4
trac_4949-root.v4.patch (7.9 KB) - added by jhpalmieri 8 years ago.
root repo
trac_4949-root-delta4to5.patch (1.0 KB) - added by jhpalmieri 8 years ago.
trac_4949-root.v5.patch (7.8 KB) - added by jhpalmieri 8 years ago.
4949_review.patch (1.5 KB) - added by jdemeyer 8 years ago.

Download all attachments as: .zip

Change History (74)

comment:1 Changed 11 years ago by mabshoff

  • Milestone changed from sage-3.3 to sage-3.2.4

comment:2 Changed 11 years ago by was

As a temporary hack to see how this "feels" you could delete spkg/build, then make it a symlink to /tmp/build/.

comment:3 Changed 9 years ago by jhpalmieri

  • Authors set to John Palmieri
  • Priority changed from critical to minor
  • Report Upstream set to N/A
  • Status changed from new to needs_review

Here's a patch. This implements both SAGE_BUILD_TMPDIR and SAGE_KEEP_BUILT_SPKGS -- see #9444. (This is an incremental change rather than a complete reworking of the sage-spkg script, which might be called for.)

comment:4 Changed 9 years ago by jhpalmieri

A little explanation: BUILD is defined (as "build") by sage-env, but it was used sporadically in sage-spkg. With this patch, it is used more consistently.

comment:5 Changed 9 years ago by mpatel

If/when this merges, we should consider closing #6550. SAGE_KEEP_BUILT_SPKGS is a much better name than SAGE_KEEP_SPKG_BUILD.

comment:6 Changed 9 years ago by leif

  • Cc drkirkby leif added
  • Description modified (diff)

Nice to see progress in the build process... :)

See also http://trac.sagemath.org/sage_trac/ticket/6550#comment:7 .

comment:7 follow-up: Changed 9 years ago by leif

Should SAGE_BUILD_TMPDIR default to SAGE_TMPDIR?

(We have btw. lots of - in some cases not very well-named - environment variables.)

Making use of e.g. a RAM disk (or some user-provided directory) for doctesting is also worth doing.

comment:8 in reply to: ↑ 7 Changed 9 years ago by mpatel

Replying to leif:

Making use of e.g. a RAM disk (or some user-provided directory) for doctesting is also worth doing.

You can already set SAGE_TESTDIR (or DOT_SAGE) to do this. Or maybe I misunderstand?

comment:9 Changed 9 years ago by mariah

  • Milestone changed from sage-4.7 to sage-4.7.1
  • Status changed from needs_review to needs_work

trac_4949-scripts.patch does not apply:

sage: hg_sage.apply("/home/mariah/trac_4949-scripts.patch")
cd "/home/mariah/sage/sage-4.7.rc2-x86_64-Linux-core2-fc/devel/sage" && hg status
cd "/home/mariah/sage/sage-4.7.rc2-x86_64-Linux-core2-fc/devel/sage" && hg status
cd "/home/mariah/sage/sage-4.7.rc2-x86_64-Linux-core2-fc/devel/sage" && hg import   "/home/mariah/trac_4949-scripts.patch"
applying /home/mariah/trac_4949-scripts.patch
unable to find 'sage-spkg' for patching
5 out of 5 hunks FAILED -- saving rejects to file sage-spkg.rej
abort: patch failed to apply
sage: 

comment:10 Changed 9 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Here's a rebased version of trac_4949-scripts.patch. Note that it's for the scripts repository, so you have to apply it with "hg_scripts.apply(...)" rather than "hg_sage.apply(...)".

comment:11 Changed 9 years ago by mariah

  • Description modified (diff)
  • Reviewers set to Mariah Lenox
  • Status changed from needs_review to positive_review

I applied the patch trac_4949-scripts.patch, then moved the modified sage-spkg file to a fresh source directory of sage-4.7.rc4. I set SAGE_BUILD_TMPDIR and SAGE_KEEP_BUILT_SPKGS, and did 'make testlong'. The builds took place in the location SAGE_BUILD_TMPDIR and all tests passed. I applied the patch trac_4949-installation.patch, did 'sage -b', then 'sage -docbuild installation html' and verified that the documentation change was made and makes sense. Positive review.

comment:12 Changed 9 years ago by jdemeyer

  • Description modified (diff)

comment:13 follow-up: Changed 9 years ago by jdemeyer

  • Status changed from positive_review to needs_info

I think one should add a note in the documentation about how much disk space this is expected to use. Are the spkgs first all built and then all deleted or are they built-deleted one by one?

comment:14 Changed 8 years ago by leif

Wouldn't it be sufficient to just (keep the -- perhaps slightly modified -- documentation and the three lines honoring SAGE_KEEP_BUILT_SPKGS and) change the variable BUILD (bad name btw.) in sage-env if SAGE_BUILD_TMPDIR is set?

That way this ticket would hardly collide with #11021, which fixes a lot in sage-spkg (and a bug in sage-env w.r.t. BUILD), also using $BUILD consistently there.

I also would use SAGE_BUILD_TMPDIR "directly", without creating yet another subdirectory (build) in it; the spkgs are extracted into their own directories anyway.

comment:15 Changed 8 years ago by leif

Ooops, unfortunately it's not that easy, because $BUILD is also just used as a subdirectory name in sage-spkg.

comment:16 in reply to: ↑ 13 ; follow-up: Changed 8 years ago by jhpalmieri

  • Status changed from needs_info to needs_review

Replying to jdemeyer:

I think one should add a note in the documentation about how much disk space this is expected to use. Are the spkgs first all built and then all deleted or are they built-deleted one by one?

I've modified the documentation to try to address this. I built Sage on various machines (sage.math, David Kirkby's machine hawk, various skynet machines), and found that

  • the single largest subdirectory of "build" can be up to 1165M (building eclib on the skynet machines iras and cleo, ia64 processors). On all of the other machines, it took at most 880M. On sage.math, cicero, and my mac, the largest took 320M.
  • the total amount of disk space, if you keep all of the subdirectories can be as large as 5.3G (iras and cleo again) or as small as 2.2G (hawk).

I've put in conservative estimates for these in the documentation.

comment:17 in reply to: ↑ 16 ; follow-up: Changed 8 years ago by leif

  • Reviewers changed from Mariah Lenox to Mariah Lenox, Leif Leonhardy

Replying to jhpalmieri:

I've modified the documentation to try to address this.

The usage of $ is a bit inconsistent: :file:`$SAGE_ROOT/...` vs. :file:`SAGE_BUILD_TMPDIR/...`.

I would add a warning that SAGE_BUILD_TMPDIR must not contain spaces, and should be an absolute path (starting with a slash or whatever). Note that none of this is checked in sage-spkg; also, a broken test might return true for an empty string, so I would also test -n "$SAGE_BUILD_TMPDIR".

Also, if SAGE_BUILD_TMPDIR is set but the directory does not exist, no warning or error message is printed.


I built Sage on various machines [...] and found that [...] I've put in conservative estimates for these in the documentation.

The actual space required or used does hardly depend on the platform, but the file system characteristics, i.e. the block size.

The worst case space usage is theoretically unlimited when taking into account rebuilds and (re)installations of newer packages, as old build dirs are moved to the $BUILD/old/ directory if -s was specified or SAGE_KEEP_BUILT_SPKGS=yes.

(Btw., for some reason the build dirs of the base packages never get deleted. Perhaps that's a side-effect of the "BUILD bug", haven't tracked this down.)

I would mention the relationship to the -s parameter when installing packages with sage; the main reason for the additional environment variable is that there's no other way to achieve what -s does when using make.

comment:18 in reply to: ↑ 17 Changed 8 years ago by jhpalmieri

Replying to leif:

The actual space required or used does hardly depend on the platform, but the file system characteristics, i.e. the block size.

Well, many of the systems on which I tested were on skynet, built in subdirectories of a shared home directory -- all of the skynet machines use the same $HOME. On some of those machines, building eclib took over 1 gigabyte, while on others, it took under 320 megabytes. There are certainly differences between the types of libraries produced: .so files on linux, .dylib files on darwin, etc. I would also guess that the size of the library files might vary depending on the compiler, whether it's 32- or 64-bit, etc.

The worst case space usage is theoretically unlimited when taking into account rebuilds and (re)installations of newer packages, as old build dirs are moved to the $BUILD/old/ directory if -s was specified or SAGE_KEEP_BUILT_SPKGS=yes.

Right, but the documentation as written is accurate ("After a full build of Sage...") and I think is good enough. Anyone who sets this variable should be paying attention to the build directory anyway.

(Btw., for some reason the build dirs of the base packages never get deleted. Perhaps that's a side-effect of the "BUILD bug", haven't tracked this down.)

prereq and bzip are not installed by sage-spkg but by their own install scripts (prereq-0.9-install and bzip2-1.0.5-install), which create subdirectories of build but don't delete them when they're done.

Adding a comment about the relationship to the -s option is a good idea. I'll try to add some tests for SAGE_BUILD_TEMPDIR, too.

comment:19 Changed 8 years ago by jhpalmieri

Here are two new patches. The scripts patch checks for the existence of SAGE_BUILD_TMPDIR, and it also should correctly delete the build subdirectories afterwards -- I had missed this in the previous patch.

Changed 8 years ago by jhpalmieri

scripts repo

comment:20 follow-up: Changed 8 years ago by leif

There's a $ missing in

:file:`$SAGE_ROOT/spkg/build` or :file:`SAGE_BUILD_TMPDIR/build`

I would clarify that SAGE_KEEP_BUILT_SPKGS=yes affects all spkg installations (whether with ./sage [-i|-f] or make, the latter also when rebuilding [parts of] Sage), and that the build directory (within the Sage tree or in $SAGE_BUILD_TMPDIR/) will definitely grow over time, i.e., whenever new packages get installed or already existing / built packages reinstalled, unless one unsets SAGE_KEEP_BUILT_SPKGS at some point (which of course doesn't delete existing subdirectories in the first place).


Your observations regarding the build tree sizes on skynet are interesting; there IMHO shouldn't be such a large difference, at least not when doing "the same thing".

There are differences in object code size between RISC and CISC architectures (on the former usually larger, but at most by a factor of 2 I think) and between 32-bit and 64-bit (mostly on RISC architectures, and also if there's a lot of static data involving e.g. pointers or integers of different size); other differences might be due to debug symbols and how and what we build (e.g. assembly implementations, static or dynamic libraries in addition) on a specific platform.

I would mention the effect of the block size of the file system though (as a note perhaps), since many packages consist of a large number of small files.

Changed 8 years ago by jhpalmieri

sage repo: update installation guide

comment:21 in reply to: ↑ 20 Changed 8 years ago by jhpalmieri

Replying to leif:

I would clarify that SAGE_KEEP_BUILT_SPKGS=yes affects all spkg installations.

Done

and that the build directory will definitely grow over time

I've added some explanation. It doesn't grow quite as fast as it might, since pre-existing subdirectories are moved to SAGE_ROOT/spkg/build/old/, overwriting copies that were already there. So just reinstalling Sage over and over again will just use twice as much as space as doing it once. Upgrading will then take up more space.

Your observations regarding the build tree sizes on skynet are interesting; there IMHO shouldn't be such a large difference, at least not when doing "the same thing".

"eclib" is the usual culprit. There are huge differences in the amount of disk space it uses, so on some systems it is by far the largest, and on others, it isn't. On the skynet machines, "moin" uses a consistent 320 megabytes, whereas eclib ranges from something under that to over 1 gig, depending on the OS and the processor.

I would mention the effect of the block size of the file system though (as a note perhaps), since many packages consist of a large number of small files.

Done.

comment:22 Changed 8 years ago by mderickx

  • Reviewers changed from Mariah Lenox, Leif Leonhardy to Mariah Lenox, Leif Leonhardy, Maarten Derickx
  • Status changed from needs_review to positive_review

I've build sage entirely from scratch after applying the patch and replacing the bootstrap version of sage-spkg in SAGE_ROOT/spkg/base and passing all doctest . Both with and without the environment variables set. So I think this one is ready to get merged.

comment:23 Changed 8 years ago by mderickx

  • Description modified (diff)

comment:24 Changed 8 years ago by was

  • Keywords sd32 added

comment:25 Changed 8 years ago by leif

Then all of the changes of #11021 will have to be rebased on this one...

Perhaps easier the other way around. (Note that I still haven't updated the patches there though.)

comment:26 Changed 8 years ago by leif

  • Description modified (diff)
  • Milestone changed from sage-4.7.2 to sage-pending

comment:27 follow-up: Changed 8 years ago by jhpalmieri

By the way, regarding "After applying, replace the bootstrap version of sage-spkg in $SAGE_ROOT/spkg/base/ with the new version": this is taken care of automatically by the sage-sdist script, if one is making a new source distribution. You just have to make sure that the version in local/bin is up to date.

comment:28 in reply to: ↑ 27 Changed 8 years ago by leif

Replying to jhpalmieri:

By the way, regarding "After applying, replace the bootstrap version of sage-spkg in $SAGE_ROOT/spkg/base/ with the new version": this is taken care of automatically by the sage-sdist script, if one is making a new source distribution. You just have to make sure that the version in local/bin is up to date.

Yep.

Hope you don't mind me temporarily moving this to "sage-pending"; I intend to finish #11021 and rebase the patch(es) here on that, the latter presumably much easier than the other way around.

If I don't find the time, I'll set the milestone back to 4.7.2 of course.

comment:29 Changed 8 years ago by jhpalmieri

See also #329 which touches sage-spkg, although not in a very complicated way.

comment:30 Changed 8 years ago by swenson

Is there any update to this? It has been several months since the last update, and some of us are eagerly anticipating this. :)

comment:31 Changed 8 years ago by jhpalmieri

  • Description modified (diff)
  • Milestone changed from sage-pending to sage-5.0

I've rebased this to Sage 5.0.beta1. It had a positive review already, so I'm leaving it that way.

Changed 8 years ago by jhpalmieri

root repo

comment:32 follow-up: Changed 8 years ago by jdemeyer

Do we *really* want

BUILD=build

?

I think we have too many variables already.

Also, why introduce a new variable $BUILD_DIR instead of using $SAGE_BUILD_TMPDIR?

comment:33 in reply to: ↑ 32 ; follow-ups: Changed 8 years ago by jhpalmieri

Replying to jdemeyer:

Do we *really* want

BUILD=build

?

I don't see a reason for it, it was there before. Should I create a new patch which removes it? I think that would fix one or two of the issues from #11021.

Also, why introduce a new variable $BUILD_DIR instead of using $SAGE_BUILD_TMPDIR?

The code says, roughly

if $SAGE_BUILD_TMPDIR is not empty and points to an existing directory:
    BUILD_DIR=$SAGE_BUILD_TMPDIR
else
    BUILD_DIR=$SAGE_PACKAGES
fi

build Sage in $BUILD_DIR

comment:34 in reply to: ↑ 33 Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

But why not simplify this to:

if $SAGE_BUILD_TMPDIR is not empty:
    sanity check $SAGE_BUILD_TMPDIR but don't change any variables
else
    SAGE_BUILD_TMPDIR=$SAGE_PACKAGES
fi

build Sage in $SAGE_BUILD_TMPDIR

comment:35 in reply to: ↑ 33 Changed 8 years ago by jdemeyer

Concerning $BUILD: it existed before, but was almost not used. If the variable serves no purpose and is not meant to be customized, it is cleaner to delete it.

comment:36 Changed 8 years ago by jdemeyer

Something else: do we really want to build in

$SAGE_BUILD_TMPDIR/build/atlas-3.8.4.p1/

I would find it more natural to build in

$SAGE_BUILD_TMPDIR/atlas-3.8.4.p1/

And therefore, let $SAGE_BUILD_TMPDIR by default be equal to

$SAGE_ROOT/spkg/build/

And perhaps drop the requirement for $SAGE_BUILD_TMPDIR to be an existing directory: just create it if needed.

(This is just an idea: I don't have strong feelings about this)

comment:37 follow-ups: Changed 8 years ago by jhpalmieri

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

If you run "sage -i blah" and "blah" is an optional spkg which needs to be downloaded, would it make more sense to save the spkg in $SAGE_ROOT/spkg/optional/ or in $SAGE_BUILD_TMPDIR/optional? The previous versions of the patch do the second of these, but I think it makes more sense to do the first: downloading spkg files for optional packages is not the same as building, so setting SAGE_BUILD_TMPDIR shouldn't cause the spkg files to end up somewhere nonstandard. Another way to say it: I don't view downloaded spkg files for optional spkgs as temporary, the way build directories are.

Here's a new patch. I've removed the variables "BUILD" and "BUILD_DIR", and I've appended "build" to the setting of "$SAGE_BUILD_TMPDIR". I still check whether $SAGE_BUILD_TMPDIR exists, as a safeguard against typos, for example. Optional spkgs are kept in $SAGE_PACKAGES/optional regardless of the setting of $SAGE_BUILD_TMPDIR.

Changed 8 years ago by jhpalmieri

root repo

comment:38 Changed 8 years ago by ohanar

  • Status changed from needs_review to needs_work

make clean does not clean $SAGE_BUILD_TMPDIR ...

I would also like $SAGE_BUILD_TMPDIR to be created if it does not exist, but I can live without.

comment:39 Changed 8 years ago by jhpalmieri

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

Okay, here are new patches. make clean and make distclean now clean $SAGE_BUILD_TMPDIR, and in sage-spkg, if the directory doesn't exist, it is created. The documentation has been changed to reflect this. (The "delta" patches are the differences between the old versions and the new ones, for reference.)

Changed 8 years ago by jhpalmieri

root repo

Changed 8 years ago by jhpalmieri

root repo: diff between v2 and v3

Changed 8 years ago by jhpalmieri

sage repo: update installation guide

Changed 8 years ago by jhpalmieri

sage repo: diff between original and v2

comment:40 in reply to: ↑ 37 Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

Optional spkgs are kept in $SAGE_PACKAGES/optional regardless of the setting of $SAGE_BUILD_TMPDIR.

I totally agree on this.

comment:41 in reply to: ↑ 37 ; follow-up: Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_info

Replying to jhpalmieri:

I've appended "build" to the setting of "$SAGE_BUILD_TMPDIR".

I ask you again: why???

comment:42 in reply to: ↑ 41 ; follow-ups: Changed 8 years ago by jhpalmieri

Replying to jdemeyer:

Replying to jhpalmieri:

I've appended "build" to the setting of "$SAGE_BUILD_TMPDIR".

I ask you again: why???

It feels cleaner to me: if I set $SAGE_BUILD_TMPDIR=/tmp, then rather than producing many subdirectories of tmp, everything will live in /tmp/build. It's easier to clean up by hand, and make clean is easier to implement this way; otherwise, I suppose it would have to delete /tmp/atlas-3.8.4.p1/, /tmp/blas-..., etc. Or make clean could not modify SAGE_BUILD_TMPDIR at all, but the Make manual suggests that make clean (and make distclean) should "Also delete files in other directories if they are created by this makefile."

comment:43 in reply to: ↑ 42 Changed 8 years ago by jdemeyer

Replying to jhpalmieri:

Replying to jdemeyer:

Replying to jhpalmieri:

I've appended "build" to the setting of "$SAGE_BUILD_TMPDIR".

I ask you again: why???

It feels cleaner to me: if I set $SAGE_BUILD_TMPDIR=/tmp, then rather than producing many subdirectories of tmp, everything will live in /tmp/build.

My opinion is that the user should set SAGE_BUILD_TMPDIR=/tmp/build if that's what he wants.

Or make clean could not modify SAGE_BUILD_TMPDIR at all, but the Make manual suggests that make clean (and make distclean) should "Also delete files in other directories if they are created by this makefile."

I think it does make sense if make clean would always clean $SAGE_ROOT/spkg/build and not SAGE_BUILD_TMPDIR.

Anyway: I don't want to fight over this. I'm happy with whatever the outcome. The last thing I want is that this ticket gets abandoned.

comment:44 Changed 8 years ago by jdemeyer

  • Summary changed from Optionally build spkgs in $SAGE_BUILD_TMPDIR to `

One other small thing: I would prefer the directory build/old to be created only when needed: replace the line

mkdir -p "$SAGE_BUILD_TMPDIR/build/old"

by

mkdir -p "$SAGE_BUILD_TMPDIR/build"

(and change the error message below)

And also replace

    mv -f "$PKG_BASE-"* old/  2>/dev/null

by

    mkdir -p old
    if [ $? -ne 0 ]; then
        echo >&2 "Error creating directory $SAGE_BUILD_TMPDIR/old."
        exit 1
    fi
    mv -f "$PKG_BASE-"* old/

comment:45 Changed 8 years ago by jdemeyer

If you really want to go with the "build" subdirectory, I would prefer appending it immediately:

if [ -n "$SAGE_BUILD_TMPDIR" ]; then
    SAGE_BUILD_TMPDIR="$SAGE_BUILD_TMPDIR/build"
    if [ ! -d "$SAGE_BUILD_TMPDIR" ]; then
        echo "Creating directory \$SAGE_BUILD_TMPDIR (=$SAGE_BUILD_TMPDIR)."
        mkdir -p "$SAGE_BUILD_TMPDIR"
    fi
    echo "Building in $SAGE_BUILD_TMPDIR."
else
    SAGE_BUILD_TMPDIR="$SAGE_PACKAGES/build"
fi

(Actually, the whole if [ ! -d ] ... fi block can be removed).

comment:46 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from ` to Optionally build spkgs in $SAGE_BUILD_TMPDIR

comment:47 in reply to: ↑ 42 ; follow-up: Changed 8 years ago by drkirkby

Replying to jhpalmieri:

Replying to jdemeyer:

I ask you again: why???

It feels cleaner to me: if I set $SAGE_BUILD_TMPDIR=/tmp, then rather than producing many subdirectories of tmp, everything will live in /tmp/build. It's easier to clean up by hand, and make clean is easier to implement this way; otherwise, I suppose it would have to delete /tmp/atlas-3.8.4.p1/, /tmp/blas-..., etc. Or make clean could not modify SAGE_BUILD_TMPDIR at all, but the Make manual suggests that make clean (and make distclean) should "Also delete files in other directories if they are created by this makefile."

John,

how do you avoid a possible race condition if two or more instances of Sage (or some other program), want to use /tmp/build? Perhaps /tmp/$user-sage-$SageVersion?.$$ might be better. Someone can then find their own Sage-related files easily (for example

rm -rf /tmp/drkirkby-sage-4.5.6*

without risk of their being any race condition.

Dave

comment:48 in reply to: ↑ 47 Changed 8 years ago by jhpalmieri

  • Status changed from needs_info to needs_review

Replying to drkirkby:

Replying to jhpalmieri:

Replying to jdemeyer:

I ask you again: why???

It feels cleaner to me: if I set $SAGE_BUILD_TMPDIR=/tmp, then rather than producing many subdirectories of tmp, everything will live in /tmp/build. It's easier to clean up by hand, and make clean is easier to implement this way; otherwise, I suppose it would have to delete /tmp/atlas-3.8.4.p1/, /tmp/blas-..., etc. Or make clean could not modify SAGE_BUILD_TMPDIR at all, but the Make manual suggests that make clean (and make distclean) should "Also delete files in other directories if they are created by this makefile."

John,

how do you avoid a possible race condition if two or more instances of Sage (or some other program), want to use /tmp/build?

I don't. If someone wants to set $SAGE_BUILD_TMPDIR, then I'm making it their responsibility to make sure that the directory is available and in good shape. (If someone does export SAGE_BUILD_TMPDIR=/tmp/sage and then we mangle the file name somehow, I think that will lead to confusion much more often than it will help. We could instead build in SAGE_BUILD_TMPDIR/subdir where we choose subdir to avoid race conditions. But I'm not going to do that.)

Meanwhile, I have new versions of the patches. I'm going to give in on the "build" subdirectory: Sage will now build in SAGE_BUILD_TMPDIR. Oh, and I changed the name to SAGE_BUILD_DIR instead; I think that name makes more sense. I also patched bzip2 and prereq so they build in SAGE_BUILD_DIR as well — easy to do now that the base repo has been merged with the root repo. make clean no longer touches SAGE_BUILD_DIR, nor does make distclean. (Also, make clean no longer recreates spkg/build or spkg/archive — what's that directory for, anyway?).

I updated the documentation accordingly.

Changed 8 years ago by jhpalmieri

installation guide: diff between v2 and v3

Changed 8 years ago by jhpalmieri

sage repo: update installation guide

Changed 8 years ago by jhpalmieri

root repo: diff between v3 and v4

Changed 8 years ago by jhpalmieri

root repo

comment:49 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

comment:50 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Lines 210 and 216 of sage-spkg: you have twice cd "$SAGE_BUILD_DIR". Remove the second and move the check (line 219) up, after the first cd. You probably want to "exit 1" if cd fails.

Line 227 of sage-spkg: replace

if [ -e "$dir" ]; then

by

if [ -d "$dir" ]; then

comment:51 follow-up: Changed 8 years ago by jdemeyer

Line 235: why "mv -f" and not simply "mv"?

comment:52 in reply to: ↑ 51 Changed 8 years ago by jhpalmieri

  • Status changed from needs_work to needs_review

Replying to jdemeyer:

Lines 210 and 216 of sage-spkg: you have twice cd "$SAGE_BUILD_DIR". Remove the second and move the check (line 219) up, after the first cd. You probably want to "exit 1" if cd fails.

Well, the old version had a second 'cd' command, justified by the comment

# Make triply sure that we are in the build directory before doing  
# a scary "rm -rf"

So I left the second one in. You think I should change this? In any case, you're right about the "exit 1".

Line 227 of sage-spkg: replace

if [ -e "$dir" ]; then

by

if [ -d "$dir" ]; then

On the off-chance that there is a file (not a directory) in the build directory with the wrong name, shouldn't we move it, too?

Replying to jdemeyer:

Line 235: why "mv -f" and not simply "mv"?

Left over from the previous version. I can fix that.

Changed 8 years ago by jhpalmieri

Changed 8 years ago by jhpalmieri

comment:53 Changed 8 years ago by jhpalmieri

  • Description modified (diff)

Changed 8 years ago by jdemeyer

comment:54 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Reviewers changed from Mariah Lenox, Leif Leonhardy, Maarten Derickx to Mariah Lenox, Leif Leonhardy, Maarten Derickx, Jeroen Demeyer

comment:55 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Summary changed from Optionally build spkgs in $SAGE_BUILD_TMPDIR to Optionally build spkgs in $SAGE_BUILD_DIR

comment:56 Changed 8 years ago by jhpalmieri

The review patch looks okay to me.

comment:57 Changed 8 years ago by jdemeyer

  • Status changed from needs_review to positive_review

Looks good to me too.

comment:58 Changed 8 years ago by jdemeyer

  • Merged in set to sage-5.0.beta5
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:59 Changed 8 years ago by jhpalmieri

See #12637 for a followup.

Note: See TracTickets for help on using tickets.