Opened 8 years ago
Closed 8 years ago
#11969 closed defect (fixed)
Clean up top-level Makefile
Reported by: | jdemeyer | Owned by: | GeorgSWeber |
---|---|---|---|
Priority: | major | Milestone: | sage-4.8 |
Component: | build | Keywords: | Makefile pipestatus |
Cc: | Merged in: | sage-4.8.alpha2 | |
Authors: | Jeroen Demeyer | Reviewers: | John Palmieri, Leif Leonhardy |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11926, #11972, #11959 | Stopgaps: |
Description (last modified by )
Using &&
inside commands for pipestatus does not work. Therefore, in the top-level Makefile, we should move $(TESTPRELIMS)
out of $(PIPE)
.
For example, if sage-starts
fails, one would get
$ make ptestlong [...] spkg/pipestatus ". local/bin/sage-env && sage-starts && ./sage -tp 0 -sagenb -long devel/sage/doc/common devel/sage/doc/de devel/sage/doc/en devel/sage/doc/fr devel/sage/doc/ru devel/sage/sage 2>&1" "tee -a ptestlong.log" Testing that Sage starts... Traceback (most recent call last): File "/usr/local/src/sage-4.7.2.alpha4/local/bin/sage-eval", line 4, in <module> from sage.all import * File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/all.py", line 68, in <module> from sage.misc.all import * # takes a while File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/misc/all.py", line 81, in <module> from functional import (additive_order, File "/usr/local/src/sage-4.7.2.alpha4/local/lib/python2.6/site-packages/sage/misc/functional.py", line 36, in <module> from sage.rings.complex_double import CDF File "integer.pxd", line 9, in init sage.rings.complex_double (sage/rings/complex_double.c:14716) ImportError: libgmp.so.8: cannot open shared object file: No such file or directory Sage failed to start up. Please email sage-devel (http://groups.google.com/group/sage-devel) explaining the problem and send the log file /usr/local/src/sage-4.7.2.alpha4/start.log Describe your computer, operating system, etc. spkg/pipestatus: line 44: [: -ne: unary operator expected make: *** [ptestlong] Error 1
Note the penultimate line.
The attached patch also:
- removes
sage-starts
fromsage -tp
, because runningmake ptest
ormake ptestlong
already runssage-starts
. - runs
./sage -b
in themake build
rule. This ensures that the Sage library is up-to-date when testing or building documentation. - adds
make testalllong
andmake ptestalllong
for completeness. - generally cleans up the
Makefile
.
Apply:
- 11969_testprelims.patch to SAGE_ROOT
- 11969_scripts.patch to scripts
Attachments (2)
Change History (29)
comment:1 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_review
comment:2 Changed 8 years ago by
- Description modified (diff)
comment:3 Changed 8 years ago by
- Keywords Makefile pipestatus added
comment:4 Changed 8 years ago by
comment:5 Changed 8 years ago by
Looks okay to me, too. I think it also makes sense to move sage-env out of TESTPRELIMS
and only run it explicitly where needed. I think that logging the output from sage-starts
might be nice, but it's pretty minor.
Does sage -t
accept --optional
as meaning the same as -optional
? It accepts --sagenb
and --long
. Do you want to change those while you're editing those lines?
comment:6 follow-up: ↓ 14 Changed 8 years ago by
- Status changed from needs_review to needs_work
I noticed that make ptestlong
actually runs sage-starts
twice: once from the Makefile
and once from sage-sage
.
I propose not to run sage-starts
when the user explicitly runs
$ ./sage -tp [...]
but to run sage-starts
only when the user does
$ make ptestlong
The rationale is that sage -tp
is often done with one or few files and then the overhead of sage-starts
might be significant. However, for a make ptestlong
run, running sage-starts
once does not really impact the testing time.
Changed 8 years ago by
comment:7 Changed 8 years ago by
- Description modified (diff)
comment:8 Changed 8 years ago by
- Description modified (diff)
comment:9 Changed 8 years ago by
- Status changed from needs_work to needs_review
I made some additional changes to the top-level Makefile
, needs_review
comment:10 Changed 8 years ago by
- Description modified (diff)
comment:11 follow-up: ↓ 12 Changed 8 years ago by
I'm wary of not running sage-starts as part of "sage -tp". It was introduced in #7103 to fix a problem when someone ran "sage -tp" before actually running Sage. I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.
comment:12 in reply to: ↑ 11 ; follow-up: ↓ 13 Changed 8 years ago by
Replying to jhpalmieri:
I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.
Then all those tests would fail. Is that a problem?
In all seriousness: I can see some arguments supporting sage-starts
before ./sage -tp [...]
. But then we should also do that for ./sage -t [...]
. I value consistency and predictability very high, I prefer either "always" or "never" to anything in between.
comment:13 in reply to: ↑ 12 ; follow-up: ↓ 16 Changed 8 years ago by
Replying to jdemeyer:
Replying to jhpalmieri:
I can see someone doing "make build" and then "./sage -tp ..." in order to test whether a new Sage build fixes problems in a particular directory.
Then all those tests would fail. Is that a problem?
No, the problem is that you might get this:
A mysterious error (perhaps a memory error?) occurred, which may have crashed doctest.
And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage. So it doesn't occur when you do "sage -t", only "sage -tp".
comment:14 in reply to: ↑ 6 Changed 8 years ago by
Replying to jdemeyer:
I noticed that
make ptestlong
actually runssage-starts
twice: once from theMakefile
and once fromsage-sage
.I propose not to run
sage-starts
when the user explicitly runs
$ ./sage -tp [...]
but to run
sage-starts
only when the user does
$ make ptestlong
The rationale is that
sage -tp
is often done with one or few files and then the overhead ofsage-starts
might be significant. However, for amake ptestlong
run, runningsage-starts
once does not really impact the testing time.
Yes, definitely, i.e. remove it from sage-sage
.
comment:16 in reply to: ↑ 13 ; follow-up: ↓ 18 Changed 8 years ago by
Replying to jhpalmieri:
And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage.
How is this solved by running sage-starts
? I would assume that a user doing ./sage -tp
has already ran Sage at least once. And it also seems that #11926 should solve precisely this problem (except in the rare case where the changed code has to do with creating directories in the Sage startup).
Also, such a race condition is a bug in Sage. If you have more information about when it occurs and for which directories, we can look into this. (I find it very annoying that Python doesn't seem to have an equivalent of mkdir -p
, making a directory but not complaining if it exists).
comment:17 Changed 8 years ago by
Sorry, hadn't read John's comments before my last post.
But IMHO, if there are race conditions in creating directories (in $DOT_SAGE/
), the race conditions should get fixed instead of using dumb work-arounds that are hardly ever necessary.
Note that e.g. only sage-ptest
supports SAGE_TEST_ITER
.
comment:18 in reply to: ↑ 16 Changed 8 years ago by
Replying to jdemeyer:
Replying to jhpalmieri:
And you get it from a race condition, where multiple processes are simultaneously trying to create the same subdirectories of .sage.
I would assume that a user doing
./sage -tp
has already ran Sage at least once. And it also seems that #11926 should solve precisely this problem (except in the rare case where the changed code has to do with creating directories in the Sage startup).
This doesn't help if $DOT_SAGE
differs, e.g. because the user changed the variable's setting, or it's just a different user that runs Sage [for the first time maybe, or deleted his previous .sage
, or there are new directories to create].
comment:19 Changed 8 years ago by
If you have more information about when it occurs and for which directories, we can look into this.
You could look at #7103, as I mentioned above, and to #7079, to which #7103 refers.
I find it very annoying that Python doesn't seem to have an equivalent of mkdir -p, making a directory but not complaining if it exists
See #11972. I think the patch there should also take care of the race conditions when creating DOT_SAGE and its various subdirectories, which seemed to be the problem with #7103. One way to test: remove the sage-starts code from the doctesting routines and set DOT_SAGE to a non-existing directory and run "sage -tp ...". Repeat many times and see if you ever get "mysterious errors". Then apply the patch at #11972 and see if that fixes it.
If this fixes it, then I'm willing to make #11972 a prerequisite for this ticket and remove the execution of sage-starts from sage-sage. But I'm not willing to remove sage-starts unless we're pretty sure that we haven't reintroduced a possible bug in doctesting.
comment:20 Changed 8 years ago by
- Dependencies changed from #11926 to #11926, #11972
Changed 8 years ago by
comment:21 Changed 8 years ago by
- Dependencies changed from #11926, #11972 to #11926, #11972, #11959
comment:22 Changed 8 years ago by
- Description modified (diff)
comment:23 Changed 8 years ago by
- Summary changed from Move $(TESTPRELIMS) out of pipestatus to Clean up top-level Makefile
comment:25 follow-up: ↓ 26 Changed 8 years ago by
- Milestone set to sage-4.8
- Reviewers set to John Palmieri, Leif Leonhardy
- Status changed from needs_review to positive_review
This all looks good to me, and I'm happy with the patch to the scripts repo now that #11972 is merged.
comment:26 in reply to: ↑ 25 Changed 8 years ago by
Replying to jhpalmieri:
This all looks good to me, and I'm happy with the patch to the scripts repo now that #11972 is merged.
Thanks for the review!
comment:27 Changed 8 years ago by
- Merged in set to sage-4.8.alpha2
- Resolution set to fixed
- Status changed from positive_review to closed
Looks reasonable, although you could just use parentheses in the parameter to
pipestatus
. (That way, the output ofsage-starts
would still get logged, too.)