Ticket #8357 (closed defect: fixed)
eclib: suppress / delete files left by doctests and check, fix building in parallel
| Reported by: | mpatel | Owned by: | cremona |
|---|---|---|---|
| Priority: | major | Milestone: | sage-4.3.4 |
| Component: | elliptic curves | Keywords: | |
| Cc: | Work issues: | ||
| Report Upstream: | N/A | Reviewers: | Minh Van Nguyen |
| Authors: | John Cremona, Mitesh Patel | Merged in: | sage-4.3.4.alpha1 |
| Dependencies: | Stopgaps: |
Description (last modified by mpatel) (diff)
From John Palmieri:
When I run doctests on the file ell_rational_field.py, I end up with a small file called PRIMES in the current directory. This shouldn't happen: running doctests shouldn't produce files in a non-temporary directory. [...]
This is a follow-up to #7575. Please see comment 24+ for some progress.
A new spkg is available at
It includes the PRIMES, 1, and make check fixes mentioned below and parallel build fixes (cf. #8306).
Note to release manager: Merge only the spkg.
Attachments
Change History
comment:2 Changed 3 years ago by mpatel
- Status changed from new to needs_review
- Milestone set to sage-4.3.4
- Description modified (diff)
- Authors set to John Cremona, Mitesh Patel
comment:3 follow-up: ↓ 4 Changed 3 years ago by mpatel
- Status changed from needs_review to needs_work
There's still a timing(?) problem ( log).
Changed 3 years ago by mpatel
-
attachment
trac_8357-eclib_makefiles.patch
added
Tweak MAKEFILEs for parallel builds. eclib src repo.
comment:5 in reply to: ↑ 4 Changed 3 years ago by cremona
Replying to mpatel:
Replying to mpatel:
There's still a timing(?) problem ( log).
I think the problem is that make sometimes attempts to build a target in progs before it's done building a requisite shared library, e.g., install_so. I've updated the "makefiles" patch and spkg with a fix that I'm testing now.
That sounds likely. We (I) should probably tidy this up; there are several useful executables which are built, but the only one actually used by and accessible from Sage after the build is mwrank. So i should change the targets so that the other progs are only built when doing a make check.
comment:6 in reply to: ↑ 4 Changed 3 years ago by mpatel
Replying to mpatel:
[...] I've updated the "makefiles" patch and spkg with a fix that I'm testing now.
It seems to work and it survives a stress test on sage.math, e.g.,
export MAKE="make -j20" # And NTL_PREFIX, etc. make veryclean && make && make so && make veryclean && make && make so && [lots of reps] && ls
comment:7 follow-up: ↓ 8 Changed 3 years ago by cremona
mpatel: if possible could you make he following additional edits to src/procs/Makefile (in eclib...p10):
diff -r 809e34b4c146 procs/Makefile --- a/procs/Makefile Sat Feb 27 15:42:31 2010 -0800 +++ b/procs/Makefile Sun Feb 28 20:08:44 2010 +0000 @@ -105,7 +105,7 @@ gzip procs.tar check: $(TESTS) - rm -f PRIMES t + rm -f PRIMES t 1 ./vectest1 < vectest.in > t && diff t vectest.out ./vectest2 < vectest.in > t && diff t vectest.out ./mattest1 < mattest.in > t && diff t mattest.out @@ -128,7 +128,7 @@ ./rcubic < rcubic.in > t && diff t rcubic.out ./lcubic < lcubic.in > t && diff t lcubic.out ./tp2points < tp2points.in > t && diff t tp2points.out - rm -f PRIMES t + rm -f PRIMES t 1
It's another temp file which gets left behind, this time after "make check".
If that's too much hassle I'll do it next time I make changes to the eclib spkg, but it would be convenient to do it now.
Changed 3 years ago by mpatel
-
attachment
trac_8357-spkg.patch
added
Diff of spkg-install, SPKG.txt. eclib spkg repo.
Changed 3 years ago by mpatel
-
attachment
trac_8357-suppress_PRIMES.patch
added
Don't write PRIMES. Delete 1 after check. eclib src repo
comment:8 in reply to: ↑ 7 Changed 3 years ago by mpatel
- Status changed from needs_work to needs_review
- Description modified (diff)
- Summary changed from Doctesting ell_rational_field.py leaves a file PRIMES in the current directory to eclib: suppress / delete files left by doctests and check, fix building in parallel
Replying to cremona:
If that's too much hassle I'll do it next time I make changes to the eclib spkg, but it would be convenient to do it now.
Done! I've updated spkg and two patches.
comment:9 Changed 3 years ago by mpatel
I noticed a different problem with make check on t2:
make[3]: Entering directory `/scratch/mpatel/sage-4.3.0.1/spkg/build/eclib-20080310.p10/src/g0n' rm -f PRIMES t ./modtest < modtest.in > t 2>/dev/null && diff t modtest.out ./homtest < homtest.in > t && diff t homtest.out ./hecketest < hecketest.in > t 2>/dev/null && diff t hecketest.out ./mhcount < mhcount.in > t && diff t mhcount.out rm -rf tmp_nf_dir mkdir tmp_nf_dir export NF_DIR=tmp_nf_dir && ./tmanin < tmanin.in > t 2>/dev/null && diff t tmanin.out /bin/sh: NF_DIR=tmp_nf_dir: is not an identifier make[3]: *** [check] Error 1 make[3]: Leaving directory `/scratch/mpatel/sage-4.3.0.1/spkg/build/eclib-20080310.p10/src/g0n'
I think we can fix this with, e.g.,
env NF_DIR=tmp_nf_dir ./tmanin < tmanin.in > t 2>/dev/null && diff t tmanin.out
comment:10 Changed 3 years ago by cremona
I tested the current version of the spkg. (This does nto have the suggested changes to the handling of NF_DIR). All was well.
I did not make new changes (for the NF_DIR thing) since I thought that would be too confusing. But I'll be happy to check the next version of p10 in due course.
comment:11 Changed 3 years ago by cremona
There is a simpler solution to the problem with "export NF_DIR". The code uses a default directory name for this, namely "newforms", which can be over-ridden by the use of the environment variable NF_DIR. For this test therefore we can replace the 2 lines "rm -rf tmp_nf_dir" by "rm -rf newforms" (strictly only the second one is needed) and remove all the starts of lines of the form "export NF_DIR=tmp_nf_dir && ". (This is in src/g0n/Makefile).
Here's why I did not do this in the first place: by own private copy of this code has a newforms directory which contains 3.4G of data which took many many processor-years to compute. I do not want a simple "make check" to delete that so I put in this temporary (and non-default) tmp_nf_dir thing instead. But for the Sage distribution that is not needed.
I can attach a replacement Makefile here if that would be convenient, for yet another version of p10.
comment:12 follow-up: ↓ 13 Changed 3 years ago by mpatel
Sure, that sounds good.
comment:13 in reply to: ↑ 12 Changed 3 years ago by cremona
Replying to mpatel:
Sure, that sounds good.
I have attached the replacement Makefile -- could you update the spkg with it?
comment:14 Changed 3 years ago by mpatel
- Description modified (diff)
Done! The package now builds in parallel and make check now works for me on t2 and sage.math.
Changed 3 years ago by mpatel
-
attachment
trac_8357-newforms_dir.patch
added
Simplify g0n/Makefile. eclib src repo.
comment:15 Changed 3 years ago by mvngu
- Status changed from needs_review to positive_review
- Reviewers set to Minh Van Nguyen
The updated eclib package eclib-20080310.p10.spkg looks good. It solves the unexpected annoyance introduced by #7575.
comment:16 Changed 3 years ago by mhansen
- Status changed from positive_review to closed
- Resolution set to fixed
- Merged in set to sage-4.3.4.alpha1

The quickest quick fix would be just to comment out line 372 in src/procs/marith.cc (in eclib).
Anything else requires thought and testing, which I don't have time for i nthe next several days.