#10903 closed defect (fixed)
Update Singular to 3-1-3-3
Reported by: | malb | Owned by: | tbd |
---|---|---|---|
Priority: | critical | Milestone: | sage-4.7.2 |
Component: | packages: standard | Keywords: | singular SageDays34 sd34 |
Cc: | zimmerma, SimonKing | Merged in: | sage-4.7.2.alpha4 |
Authors: | Burcin Erocal, Martin Albrecht, Volker Braun, Simon King | Reviewers: | Martin Albrecht, Volker Braun, Simon King |
Report Upstream: | N/A | Work issues: | |
Branch: | Commit: | ||
Dependencies: | #11339 | Stopgaps: |
Description (last modified by )
Singular spkg upgrade
Singular 3-1-2 fixes a critical bug, cf. #10902. There are more bugfixes in the newest stable release. We should update as soon as possible.
Sage libsingular upgrade
There are various issues where currRing
is not correctly set. Some of these are triggered by refcounting the rings and freeing them when they are no longer needed, see the dependency ticket #11339.
Installation instructions
- Apply the two dependency patches from #11339
- Apply: 10903_flat.patch
Attachments (8)
Change History (134)
comment:1 Changed 8 years ago by
- Cc zimmerma added
comment:2 Changed 8 years ago by
comment:3 Changed 8 years ago by
comment:4 Changed 8 years ago by
sorry for the double pasting the mouse is vintage.
comment:5 follow-up: ↓ 9 Changed 8 years ago by
Updating singular to at least 3-1-3 (if it's out...) will also help the ARM port, as it's one of the few packages which still give issues with 4.6.2.
comment:6 Changed 8 years ago by
Indeed 3-1-3 is out now. Already 3-1-2 adds a lot of functionality for polynomials over the integers which would be very helpful to have in Sage!
comment:7 Changed 8 years ago by
Trying to compile libsingular with this version and trying to link it to sage is on my TODO list. The singular 3-1-3 executable as called by the pexpect interface breaks a few doctest because normalization has been changed.
comment:8 follow-up: ↓ 10 Changed 8 years ago by
Just FYI, I'm cutting a p10 shortly with a very small fix for Cygwin in it (see #11550), so if someone if making this package, it would be great to base off that.
comment:9 in reply to: ↑ 5 Changed 8 years ago by
Replying to Snark:
Updating singular to at least 3-1-3 (if it's out...) will also help the ARM port, as it's one of the few packages which still give issues with 4.6.2.
Cf. #10810.
If someone touches the Singular spkg, please also add a patch for #11645 in case it should then still be necessary. (I've submitted one upstream, see http://www.singular.uni-kl.de:8002/trac/ticket/352#comment:15).
comment:10 in reply to: ↑ 8 Changed 8 years ago by
- Keywords SageDays34 added
Replying to kcrisman:
Just FYI, I'm cutting a p10 shortly with a very small fix for Cygwin in it (see #11550), so if someone if making this package, it would be great to base off that.
FWIW, the Singular spkg from #11550 will probably become a p12, since #11663 meanwhile provides a p11 (which Karl-Dieter's will have to get rebased on again).
I'll then perhaps provide a p13 based on that fixing #11645. :)
Anyway, I guess upgrading Sage's Singular will be attempted during the Sage/Singular Days in Kaiserslautern at the end of September, so we have some time until then...
comment:11 Changed 8 years ago by
- Keywords sd34 added
comment:12 follow-up: ↓ 14 Changed 8 years ago by
- Description modified (diff)
- Status changed from new to needs_info
I haven't run any tests yet and the changes in the SPKG are not committed (because our patches will probably be accepted upstream before 3-1-4 comes out).
comment:13 Changed 8 years ago by
- Cc SimonKing added
comment:14 in reply to: ↑ 12 Changed 8 years ago by
Replying to malb:
I haven't run any tests yet and the changes in the SPKG are not committed (because our patches will probably be accepted upstream before 3-1-4 comes out).
The spkg name should contain the svn version you are using.
Regarding the patch to module_list.py
:
Please don't randomly add all libraries any extension module using Singular might also use to singular_libs
.
comment:15 follow-up: ↓ 16 Changed 8 years ago by
doctests hang here:
Trying: R = PolynomialRing(ZZ, Integer(5), order='lex', names=('x', 'y', 'a', 'b', 'u',)); (x, y, a, b, u,) = R._first_ngens(5)###line 4461:_sage_ >>> R.<x,y,a,b,u>=PolynomialRing(ZZ, 5, order='lex')
they also crash in other places. Thus, more fun ahead.
comment:16 in reply to: ↑ 15 Changed 8 years ago by
Replying to malb:
doctests hang here:
Trying: R = PolynomialRing(ZZ, Integer(5), order='lex', names=('x', 'y', 'a', 'b', 'u',)); (x, y, a, b, u,) = R._first_ngens(5)###line 4461:_sage_ >>> R.<x,y,a,b,u>=PolynomialRing(ZZ, 5, order='lex')
they also crash in other places. Thus, more fun ahead.
How about starting with 3-1-3 and some important upstream patches?
comment:17 Changed 8 years ago by
I'm in Kaiserslautern right now, so I have direct access to the Singular developers. Hence, actually fixing the issues seems like the order of the day.
comment:18 Changed 8 years ago by
- Dependencies set to #11339
comment:19 Changed 8 years ago by
With
$ hg qap trac_11339_refcount_singular_rings.patch trac_11339_refcount_singular_polynomials.patch 10903_singular-3-1-4.patch
I currently get these doctest failures.
sage -t "devel/sage-main/sage/rings/polynomial/polynomial_element.pyx" sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_ideal.py" sage -t "devel/sage-main/sage/rings/polynomial/toy_d_basis.py" sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_sequence.py" sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_element.py" sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
comment:20 Changed 8 years ago by
With the updated SPKG & patch I get:
sage -t "devel/sage-main/sage/rings/polynomial/polynomial_element.pyx"
Numerical noise, nothing serious
sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_ideal.py"
crash
sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_sequence.py"
crash
sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_element.py"
sage: f=(a*x-1)*((a+1)*y-1); f Expected: -x*y + (-a)*x + (-a - 1)*y + 1 Got: (a^2 + a)*x*y + (-a)*x + (-a - 1)*y + 1
sage -t "devel/sage-main/sage/rings/polynomial/multi_polynomial_libsingular.pyx"
Factorisation over number fields goes horribly wrong
comment:21 Changed 8 years ago by
- Description modified (diff)
comment:22 Changed 8 years ago by
Hans created a new branch of Singular, where the algebraic extensions are reverted to the old implementation. Using this branch, available in SPKG form at:
http://sage.math.washington.edu/home/malb/spkgs/singular-3-1-3.svn-algnumbers.spkg
the crashes seem to go away (at least for everything in sage/rings).
comment:23 Changed 8 years ago by
Output of make ptestlong
with this patch and #11339 applied:
sage -t -long -force_lib devel/sage/sage/misc/sageinspect.py # 1 doctests failed sage -t -long -force_lib devel/sage/sage/tests/cmdline.py # 1 doctests failed sage -t -long -force_lib devel/sage/sage/interfaces/singular.py # 1 doctests failed sage -t -long -force_lib devel/sage/sage/crypto/mq/sr.py # 0 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_generic.py # 0 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/elliptic_curves/ell_curve_isogeny.py # 0 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/plane_conics/con_field.py # 1 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/plane_conics/con_finite_field.py # 0 doctests failed sage -t -long -force_lib devel/sage/sage/schemes/generic/algebraic_scheme.py # 0 doctests failed
i.e., crashes.
comment:24 Changed 8 years ago by
I just updated the spkg such that SAGE_DEBUG="yes"
will build Singular with debugging enabled (internal consistency checks, asserts etc.)
comment:25 Changed 8 years ago by
Alright,
on my computer with
trac_11339_refcount_singular_rings.patch trac_11339_refcount_singular_polynomials.patch 10903_singular-3-1-4.patch
I'm down to these two:
$ sage -t -long -force_lib "devel/sage/sage/schemes/plane_conics/con_finite_field.py" sage: assert all([C.defining_polynomial()(Sequence(C.has_rational_point(point = True)[1])) == 0 for C in c[r::5]]) # long time: 1.6 seconds Exception raised: ... TypeError: Coordinates [4, 4, 1] do not define a point on Projective Conic Curve over Finite Field of size 5 defined by x*y + x*z + y*z + z^2
sage -t -long -force_lib "devel/sage/sage/schemes/generic/algebraic_scheme.py" sage: A.subscheme([x]) + A.subscheme([y^2 - (x^3+1)]) Expected: Closed subscheme of Affine Space of dimension 2 over Rational Field defined by: -x^4 + x*y^2 - x Got: Closed subscheme of Affine Space of dimension 2 over Rational Field defined by: x^4 - x*y^2 + x
The latter doesn't look like a bug but an improvement actually.
comment:26 Changed 8 years ago by
on sage.math
sage -t -long -force_lib devel/sage/sage/crypto/mq/sr.py
also crashes.
comment:27 Changed 8 years ago by
Martin, good, you are making progress!
Paul
comment:28 Changed 8 years ago by
make ptestlong passes on sage.math! However, it's likely that different people will see different crashes/bugs on different machines (I know that Volker has a different one on his machine). The reason is: the garbage collector may randomly change the currRing (because of #11339). The fix is to add rChangeRing() before whichever Singular function is called and crashes. As far as I understand it, there shouldn't be a Python functionc all between rChangeRing() and the function for which it is done, because those could trigger the gc.
comment:29 Changed 8 years ago by
- Description modified (diff)
comment:30 Changed 8 years ago by
SPKG doesn't build on bsd. Any suggestions?
g++ -o libsingular.so \ iparith.o mpsr_Tok.o claptmpl.o \ grammar.o scanner.o attrib.o blackbox.o eigenval_ip.o extra.o fehelp.o feOpt.o ipassign.o ipconv.o ipid.o iplib.o ipprint.o ipshell.o newstruct.o lists.o sdb.o fglm.o interpolation.o silink.o ssiLink.o subexpr.o janet.o wrapper.o libparse.o sing_win.o gms.o pcv.o maps_ip.o walk.o walk_ip.o cntrlc.o misc_ip.o calcSVD.o pipeLink.o Minor.o MinorProcessor.o MinorInterface.o bigintm.o pyobject_setup.o bbcone.o bbfan.o denom_list.o minpoly.o slInit_Static.o mpsr_Put.o mpsr_PutPoly.o mpsr_GetPoly.o mpsr_sl.o mpsr_Get.o mpsr_GetMisc.o mpsr_Error.o ndbm.o sing_dbm.o -lkernel -L../kernel -L../factory -L../libfac -L/Users/malb/sage-4.7.2.alpha3/local/lib -lsingfac -lsingcf -lntl -lreadline -lgmp -lomalloc Undefined symbols: "_main", referenced from: __start in crt1.o ld: symbol(s) not found
comment:31 Changed 8 years ago by
The reason is that
ifeq ($(SINGUNAME),ix86Mac-darwin) ... endif
doesn't apply any more, but x86_64Mac-darwin is now returned by singuname.sh
comment:32 follow-up: ↓ 33 Changed 8 years ago by
Has anyone tried building the SPKG on amd64? Here the build works for x86 but on amd64 I get
make install in Singular make![2]: Entering directory `/storage/sage/sage-4.7.2.alpha2/spkg/build/singular-3-1-3.svn-algnumbers/src/Singular' svnversion >svnver /bin/sh: svnversion: not found make![2]: *** [svnver] Error 127 make![2]: Leaving directory `/storage/sage/sage-4.7.2.alpha2/spkg/build/singular-3-1-3.svn-algnumbers/src/Singular' make![1]: *** [install] Error 1 make![1]: Leaving directory `/storage/sage/sage-4.7.2.alpha2/spkg/build/singular-3-1-3.svn-algnumbers/src' make: *** ![/storage/sage/sage-4.7.2.alpha2/local/bin/Singular-3-1-3] Error 2 Unable to build Singular.
comment:33 in reply to: ↑ 32 Changed 8 years ago by
Replying to strogdon:
Has anyone tried building the SPKG on amd64? Here the build works for x86 but on amd64 I get
Let me first install subversion and then try again!
comment:34 Changed 8 years ago by
- Description modified (diff)
the svnversion business should be fixed by the new SPKG:
http://sage.math.washington.edu/home/malb/spkgs/singular-3-1-3-3.spkg
comment:35 Changed 8 years ago by
Open issues:
- #11339 some doctests fail
- testing on more architectures
- doesn't build on bsd.math
- SPKG has changed which aren't checked in
PS: I won't be able to work on this over the next few days, so happy hacking :)
comment:36 Changed 8 years ago by
comment:37 Changed 8 years ago by
Martin, now I managed to compile the Singular spkg, but when starting Sage I get:
--> 512 from sage.rings.polynomial.multi_polynomial_libsingular import MPolynomialRing_libsingular 513 if m.integral_domain.is_IntegralDomain(base_ring): 514 if n < 1: ImportError: /localdisk/tmp/install/sage/local/lib/python2.6/site-packages/sage/rings/polynomial/multi_polynomial_libsingular.so: undefined symbol: _Z12pTotaldegreeP8spolyrecP9sip_sring Error importing ipy_profile_sage - perhaps you should run %upgrade? WARNING: Loading of ipy_profile_sage failed.
Should I have imported first the two patches?
Paul
comment:38 Changed 8 years ago by
Paul, do you have these patches applied
trac_11339_refcount_singular_rings.patch trac_11339_refcount_singular_polynomials.patch 10903_singular-3-1-4.patch trac_10903_singular-fixes.patch
and sage -b succeeded (after you installed the new SPKG)?
comment:39 Changed 8 years ago by
comment:40 Changed 8 years ago by
You'll have to
- install the SPKG,
- apply the four patches
trac_11339_refcount_singular_rings.patch trac_11339_refcount_singular_polynomials.patch 10903_singular-3-1-4.patch trac_10903_singular-fixes.patch
- run
sage -b
Sorry for the confusion.
comment:41 Changed 8 years ago by
- Description modified (diff)
- Summary changed from Update Singular to 3-1-2 or higher to Update Singular to 3-1-3-3.
I upgraded the spkg with the OSX compile fixes. I only replaced the src/
directory with the new upstream package, I haven't compiled it on OSX. The spkg still has some not-yet-checked-in changes.
comment:42 Changed 8 years ago by
The rules for using/setting currRing
when calling libSingular are:
- The Sage library Python code should never make any assumptions about
currRing
. currRing
can only be assumed to stay defined within pure C/Cython code.
This is particularly important because the garbage collector can run between any two Python instructions, but not within C/Cython!
In order to identify code where we forget to set currRing
, I implemented a function poison_currRing()
in sage.libs.singular.singular
that will set currRing
to NULL, triggering a segfault if we forget to set it back to a meaningful value before calling libSingular. Using the Python debugger hook, this function can be called between each Python command (but not within Cython code).
It can be globally enabled by uncommenting the lines
### Debugging for Singular, see trac #10903 from sage.libs.singular.ring import poison_currRing sys.settrace(poison_currRing)
at the very end of sage/all.py
.
Using this code, I identified and fixed various places in the Sage library of the form
rChangeCurrRing(_ring) self.parent().some_python_method() libSingular_func()
Note that calling some python method in-between exits Cython and allows Python to run the garbage collector etc! Not only do you exit Cython when you return from your Cython function, but also if you call Python in-between.
I also ran into an issue where code uses iteritems() to iterate over locals()
, globals()
, or dictionaries returned by gc.get_referrers()
. This can potentially raise a RuntimeError
if those dictionaries change during the iteration. Hooking into the python debugger loop is a good way of triggering precisely that. This also happens in the PolyBoRi
Python interface, this is now fixed upstream at https://sourceforge.net/tracker/?func=detail&aid=3416946&group_id=210499&atid=1013986
With the attached patch, I can run the whole Sage testsuite without errors while continuosly poisoning currRing
. If you do not patch PolyBoRi
, you get some otherwise harmless errors from it but no segfaults. Note that the patch does not enable the poisoning by default. I am quite sure that the patch fixes all remaining Sage/Singular? segfault issues. Please give it a try!
comment:43 Changed 8 years ago by
- Status changed from needs_info to needs_review
comment:44 Changed 8 years ago by
- Description modified (diff)
comment:45 Changed 8 years ago by
10903_singular-3-1-4.patch still needs work w.r.t. module_list.py
; I don't know whether all changes for Singular version 3-1-4 (svn) equally apply to 3-1-3-3. The name of the patch should change anyway if we upgrade to the stable version, i.e., (currently) 3-1-3-X.
If this spkg is to get into Sage 4.7.2, the linker issue (cf. #11769) also has to be fixed here. (Otherwise I'll provide a Singular 3-1-1-4.p14 spkg fixing just this.)
There are other issues which may already be fixed by the new version, or could be fixed by incorporating upstream patches and / or making changes to the Sage library; see comments above (or search for other Singular-related tickets in case I didn't add references to all of them here... ;-) ).
It would be much less confusing if everybody -- at least those significantly -- touching the spkg (i.e., adding changes, especially after someone else made such) bumped the patch level; since we apply patches to the vanilla upstream sources, there should (at least) be a p0 patch level anyway. (This applies to other spkgs as well by the way. It doesn't make sense to have many different spkgs with the exact same name floating around, even if md5sums are given, which in general isn't bad, independent of the former.)
comment:46 Changed 8 years ago by
I'm against bikeshedding module_list.py
any further. The next version of Singular will split into multiple libraries and any effort trying to minimize the number of linked libraries now is going to be wasted.
As far as naming goes, Martin called the patch 3-1-4 before we knew that the next version would be 3-1-3-3. He should rename the path when he gets back on Monday but thats just a trivial change. I reviewed his patch and hereby give it positive review.
The -ldl
issue #11769 is fixed.
All we need for this ticket is
- review my patch
- check that the spkg builds on OSX and Solaris
comment:47 Changed 8 years ago by
- Status changed from needs_review to needs_work
Spkg doesn't build on Solaris, will investigate...
comment:48 Changed 8 years ago by
Replying to vbraun:
I'm against bikeshedding
module_list.py
any further.
Well, I'd be happy if it just kept the color it had (more precisely, the different modules kept their different colors); what Martin did is just literally painting them all black, or brown, by mixing all the colors at least one of them had.
What would you say if I changed the libraries in all linker commands to just use $SAGE_ROOT/local/lib/*.so
?
This is a useless regression, although it seems the patch has meanwhile changed, and the changes are less poor now.
If you want to "simplify" module_list.py
by factoring out the libraries Singular (i.e. libsingular
) requires, as he probably intended, then put these libraries into singular_libs
, and nothing else.
The next version of Singular will split into multiple libraries and any effort trying to minimize the number of linked libraries now is going to be wasted.
That's totally unrelated.
comment:49 Changed 8 years ago by
The only bug, if you can call that, is that the singular_libs
variable could be called libraries_that_all_cython_code_using_libsingular_needs
. Its fine the way it is.
comment:50 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
Updated spkg, now with checked-in changes and p0! I successfully built it on bsd.math (OSX) and is currently building on Mark (Solaris Sparc). I found one small issue with Solaris that I fixed and reported on the libsingular mailinglist (https://groups.google.com/d/topic/libsingular-devel/Cdz0QpqFqDQ/discussion).
Changed 8 years ago by
comment:51 Changed 8 years ago by
- Description modified (diff)
comment:52 Changed 8 years ago by
Thank you Volker for taking care of the SPKG!
comment:53 Changed 8 years ago by
The new .p0.spkg builds on Mark (solaris sparc). Yay ;-)
comment:54 Changed 8 years ago by
trac_10903_singular-3-1-3-3.patch looks fine except that I don't understand why the misc.misc deprecated function alias changes are part of it.
comment:55 Changed 8 years ago by
The deprecated function stuff uses the garbage collector to find out the name of the variable / method it is assigned to. But it did it quite indiscriminately, which broke when my debugging patch was active. So there were tons of failed doctests because it used the name of some temporary variable instead. Its not strictly necessary to apply the patch to sage.misc.misc and sage.misc.sageinspect to work with Singular, but it is very useful if you want to turn the debugging on for currRing
...
comment:56 Changed 8 years ago by
I'm fine with Martin's changes to the spkg. So if he agrees with my Solaris fix then we have a positive review (except for the dependency ticket).
comment:57 Changed 8 years ago by
- Reviewers set to Martin Albrecht, Volker Braun
- Status changed from needs_review to positive_review
Yes, I agree with your Solaris fix. The only issue I found in the SPKG was a few "*~" files in /patches which I removed in
http://sage.math.washington.edu/home/malb/spkgs/singular-3-1-3-3.p0.spkg
I don't think this should be .p1 (I only removed backup files), so perhaps you could just replace your p0 with this one?
comment:58 follow-up: ↓ 60 Changed 8 years ago by
I replaced the singular-3-1-3-3.p0.spkg at the ticket's URL with Martin's version.
comment:59 Changed 8 years ago by
Did someone check the compiler warnings for the Singular spkg?
Especially "always false" and "may be used uninitialized" are scary:
cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type facBivar.cc:213: warning: ‘evaluation2’ may be used uninitialized in this function facBivar.cc:213: warning: ‘evaluation’ may be used uninitialized in this function facFactorize.cc:322: warning: ‘lev’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘m’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘i’ may be used uninitialized in this function facFqBivarUtil.cc:215: warning: ‘degMipoBeta’ may be used uninitialized in this function facFqFactorize.cc:1495: warning: ‘lev’ may be used uninitialized in this function sm_sparsemod.cc:113: warning: ‘N’ may be used uninitialized in this function cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type facBivar.cc:213: warning: ‘evaluation2’ may be used uninitialized in this function facBivar.cc:213: warning: ‘evaluation’ may be used uninitialized in this function facFactorize.cc:322: warning: ‘lev’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘m’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘i’ may be used uninitialized in this function facFqBivarUtil.cc:215: warning: ‘degMipoBeta’ may be used uninitialized in this function facFqFactorize.cc:1495: warning: ‘lev’ may be used uninitialized in this function sm_sparsemod.cc:113: warning: ‘N’ may be used uninitialized in this function cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type canonicalform.cc:1744: warning: deprecated conversion from string constant to ‘char*’ canonicalform.cc:1750: warning: deprecated conversion from string constant to ‘char*’ cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type facBivar.cc:213: warning: ‘evaluation2’ may be used uninitialized in this function facBivar.cc:213: warning: ‘evaluation’ may be used uninitialized in this function facFactorize.cc:322: warning: ‘lev’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘m’ may be used uninitialized in this function facFqBivar.cc:594: warning: ‘i’ may be used uninitialized in this function facFqBivarUtil.cc:215: warning: ‘degMipoBeta’ may be used uninitialized in this function facFqFactorize.cc:1495: warning: ‘lev’ may be used uninitialized in this function /usr/include/c++/4.2/backward/backward_warning.h:32:2: warning: #warning This file includes at least one deprecated or antiquated header. Please consider using one of the 32 headers found in section 17.4.1.2 of the C++ standard. Examples include substituting the <X> header for the <X.h> header for C++ includes, or <iostream> instead of the deprecated header <iostream.h>. To disable this warning use -Wno-deprecated. sm_sparsemod.cc:113: warning: ‘N’ may be used uninitialized in this function readcf.cc:1452: warning: deprecated conversion from string constant to ‘char*’ readcf.cc:1598: warning: deprecated conversion from string constant to ‘char*’ canonicalform.cc:1744: warning: deprecated conversion from string constant to ‘char*’ canonicalform.cc:1750: warning: deprecated conversion from string constant to ‘char*’ cf_factor.cc:440: warning: comparison is always false due to limited range of data type cf_factor.cc:467: warning: comparison is always false due to limited range of data type /usr/include/c++/4.2/backward/backward_warning.h:32:2: warning: #warning This file includes at least one deprecated or antiquated header. Please consider using one of the 32 headers found in section 17.4.1.2 of the C++ standard. Examples include substituting the <X> header for the <X.h> header for C++ includes, or <iostream> instead of the deprecated header <iostream.h>. To disable this warning use -Wno-deprecated. memutil.c:98: warning: implicit declaration of function ‘memcpy’ memutil.c:98: warning: incompatible implicit declaration of built-in function ‘memcpy’
comment:60 in reply to: ↑ 58 Changed 8 years ago by
Replying to vbraun:
I replaced the singular-3-1-3-3.p0.spkg at the ticket's URL with Martin's version.
You mean you copied Martin's to stp.dias.ie?
Why not change the URL in the ticket's description? I strongly doubt the md5sum is still the same... ;-)
comment:61 Changed 8 years ago by
- Description modified (diff)
Ticket description contains correct spkg.
As for the compiler warnings, upstream recently flipped on more pedantic checks and is working on fixing the compiler warnings. But not in this version.
comment:62 follow-up: ↓ 63 Changed 8 years ago by
- Status changed from positive_review to needs_work
Applying #11339 and #10903 causes the docbuilder to crash and burn:
$ rm -r devel/sage/doc/output $ make doc-html [...] ^[[?1034hsphinx-build -b html -d /usr/local/src/sage-4.7.2.alpha3/devel/sage/doc/output/doctrees/en/reference -A hide_pdf_links=1 /usr/l ^[[?1034hRunning Sphinx v1.0.4 loading pickled environment... not yet created building [html]: targets for 935 source files that are out of date updating environment: 935 added, 0 changed, 0 removed reading sources... [ 0%] algebras reading sources... [ 0%] arithgroup reading sources... [ 0%] calculus reading sources... [ 0%] categories [...] reading sources... [ 96%] sage/structure/sequence reading sources... [ 96%] sage/structure/unique_representation reading sources... [ 96%] sage/symbolic/constants reading sources... [ 96%] sage/symbolic/expression Exception occurred: File "/usr/local/src/sage-4.7.2.alpha3/devel/sage/doc/common/conf.py", line 378, in skip_member if (hasattr(obj, '__name__') and obj.__name__.find('.') != -1 and AttributeError: 'NoneType' object has no attribute 'find' The full traceback has been saved in /tmp/sphinx-err-mqo1bm.log, if you want to report the issue to the developers. Please also report this if it was a user error, so that a better error message can be provided next time. Either send bugs to the mailing list at <http://groups.google.com/group/sphinx-dev/>, or report them in the tracker at <http://bitbucket.org/birkenfeld/sphinx/issues/>. Thanks! Build finished. The built documents can be found in /usr/local/src/sage-4.7.2.alpha3/devel/sage/doc/output/html/en/reference
I have not yet investigated the issue, but it is clear that this needs to be fixed before we can merge this ticket.
comment:63 in reply to: ↑ 62 Changed 8 years ago by
Replying to jdemeyer:
Applying #11339 and #10903 causes the docbuilder to crash and burn:
I found that the problem occurs for some <sage.misc.misc.DeprecatedFunctionAlias object at 0x1e2fc10>
, which in fact wraps <method 'is_constant' of 'sage.symbolic.expression.Expression' objects>
But that's very strange, because I get
sage: x.is_constant.__name__ 'is_constant'
comment:64 Changed 8 years ago by
I changed the name lookup for DeprecatedFunctionAlias? to be stricter and less fragile with respect to the ordering of objects that the garbage collector returns. Maybe I missed something, let me check...
comment:65 Changed 8 years ago by
Aha! Perhaps the problem arose like here:
sage: from sage.misc.misc import deprecated_function_alias sage: print deprecated_function_alias(x.is_constant, "bla").__name__ None
And I think I have located the problem: trac_10903_singular-fixes.patch introduces a change in sage/misc/misc.py
, and apparently it can happen that the lazy attribute __name__
of DeprecatedFunctionAlias
returns None.
Perhaps it would be a solution to return self.func.__name__
, if no name can be determined by using inspect.stack
and gc.get_referrers
?
comment:66 Changed 8 years ago by
I found where the critical change has happened: #10859 deprecates _is_real, _is_positive and so on, and with #10903, we have
sage: print x._is_constant.__name__ None
_is_constant
is a deprecated alias for is_constant
. I think it would be a bad idea to use the name "is_constant"
for _is_constant
(note the underscores), because otherwise the docbuilder might think that the deprecated function has to be documented.
Perhaps the easiest solution is to make the lazy attribute __name__
raise an AttributeError
, rather than returning None
. That would certainly make the docbuilder happy, because it tests if hasattr(obj,'__name__') and obj.__name__.find('.') != -1 and obj.__name__.split('.')[-1] != name): ..."
.
comment:67 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
I have attached a patch that makes the __name__
lazy attribute raise an attribute error when it would return None otherwise. For me, it seems to fix the problem, but I think it should better be verified by someone else.
Apply trac_10903_singular-3-1-3-3.patch trac_10903_singular-fixes.patch trac10903_fix_name.patch
comment:69 follow-up: ↓ 71 Changed 8 years ago by
The bug is that x._is_constant
is a lazy attribute in a Cython class, and I only tested for Python classes. I've patched this issue and will upload the fix as soon as doctests finish (and this time I'm deleting doc/output so the whole documentation will be rebuilt).
The docbuilder currently documents deprecated functions and I think thats a good idea; Say, somebody suddenly gets a deprecation warning and then looks into the developer guide.
Of course x._is_constant
is always excluded from the documentation because of the underscore, before and after deprecation. This lack of documentation is precisely the reason why I renamed it to x.is_constant
and deprecated the underscore method...
comment:70 in reply to: ↑ 68 Changed 8 years ago by
Replying to leif:
Otherwise we get a new contributor...
Just one of the many errors I committed this morning...
comment:71 in reply to: ↑ 69 ; follow-up: ↓ 72 Changed 8 years ago by
Replying to vbraun:
The bug is that
x._is_constant
is a lazy attribute in a Cython class, and I only tested for Python classes. I've patched this issue and will upload the fix as soon as doctests finish (and this time I'm deleting doc/output so the whole documentation will be rebuilt).
Is that needed? For me, both the doc tests and building the references work with the three patches that we have now.
comment:72 in reply to: ↑ 71 ; follow-up: ↓ 73 Changed 8 years ago by
Replying to SimonKing:
Is that needed? For me, both the doc tests and building the references work with the three patches that we have now.
But I take it with your patch the deprecated functions (without underscore) from Cython classes aren't documented any more. For example, see doc/output/html/en/reference/sage/symbolic/expression.html#sage.symbolic.expression.Expression.lgamma
--- this is currently documented and I'd like to keep it that way until we remove it altogether.
comment:73 in reply to: ↑ 72 Changed 8 years ago by
Replying to vbraun:
Replying to SimonKing:
Is that needed? For me, both the doc tests and building the references work with the three patches that we have now.
But I take it with your patch the deprecated functions (without underscore) from Cython classes aren't documented any more. For example, see
doc/output/html/en/reference/sage/symbolic/expression.html#sage.symbolic.expression.Expression.lgamma
--- this is currently documented and I'd like to keep it that way until we remove it altogether.
No, you are mistaken. I've built the documentation, and lgamma is in.
In fact, the documentation is misformatted, it looks like
This method is deprecated, please use the .log_gamma() function instead. Log gamma function evaluated at self. EXAMPLES:: sage: x.lgamma() doctest:...: DeprecationWarning: The lgamma() function is deprecated. Use log_gamma() instead. log_gamma(x)
where actually .log_gamma()
is TeXed?. So, while we are at it, we may fix the docstring of lgamma.
comment:74 Changed 8 years ago by
- Description modified (diff)
The attached new patch fixes the misformatting in the doc of lgamma.
Apply trac_10903_singular-3-1-3-3.patch trac_10903_singular-fixes.patch trac10903_fix_name.patch trac10903_lgamma_doc.patch
comment:75 follow-ups: ↓ 77 ↓ 79 Changed 8 years ago by
So if skip_member()
raises an exception, the method is unconditionally documented? In that case, you would also document _is_constant
which was not documented before and IHMO should not be documented (or, rather, be documented contingent on the value of SAGE_DOC_UNDERSCORE
environment variable).
Can you also fix the formatting of is_constant
while you are at it?
comment:76 Changed 8 years ago by
The trac_10903_docbuild_fix.patch now looks into Python and Cython classes. If all fails, it raises an error as Simon suggested.
comment:77 in reply to: ↑ 75 ; follow-up: ↓ 78 Changed 8 years ago by
Replying to vbraun:
So if
skip_member()
raises an exception, the method is unconditionally documented?
Apparently not.
In that case, you would also document
_is_constant
which was not documented before
It isn't. Only is_constant
without underscore is. So, I don't see why the docbuild_fix patch should be needed.
Can you also fix the formatting of
is_constant
while you are at it?
I just did (updating the lgamma_doc patch).
comment:78 in reply to: ↑ 77 ; follow-up: ↓ 80 Changed 8 years ago by
Replying to SimonKing:
So if
skip_member()
raises an exception, the method is unconditionally documented?Apparently not.
That doesn't make sense if you look into the skip_member()
code. Are you always deleting the cached documentation before testing?
comment:79 in reply to: ↑ 75 Changed 8 years ago by
Replying to vbraun:
So if
skip_member()
raises an exception, the method is unconditionally documented?
To be precise: If skip_member()
raises an exception then building the documentation crashes. That's why it tests hasattr(obj,"__name__")
.
Also I wonder whether the difference between Python and Cython really matters here. Can you give a concrete example (i.e., a new doc test) that demonstrates what your patch fixes?
comment:80 in reply to: ↑ 78 ; follow-up: ↓ 81 Changed 8 years ago by
Replying to vbraun:
Are you always deleting the cached documentation before testing?
The doc of sage.symbolic.expression builds fine after touching sage/symbolic/expression.pyx and sage -b
. Would the documentation be taken from cache in that case?
comment:81 in reply to: ↑ 80 Changed 8 years ago by
Replying to SimonKing:
Replying to vbraun:
Are you always deleting the cached documentation before testing?
The doc of sage.symbolic.expression builds fine after touching sage/symbolic/expression.pyx and
sage -b
. Would the documentation be taken from cache in that case?
PS: And would the additional doc formatting fixes of the lgamma_doc patch be visible if the documentation was taken from cache?
comment:82 follow-up: ↓ 83 Changed 8 years ago by
What my patch does fix is this:
sage: print x._is_constant.__name__ _is_constant
comment:83 in reply to: ↑ 82 Changed 8 years ago by
comment:84 Changed 8 years ago by
The updated patch adds doctest and improves the cython class detection.
comment:85 follow-up: ↓ 86 Changed 8 years ago by
- Description modified (diff)
I'm happy with trac10903_lgamma_doc.patch. Maybe Simon can quickly OK the trac_10903_docbuild_fix.patch, then we would be good to go?
comment:86 in reply to: ↑ 85 Changed 8 years ago by
- Reviewers changed from Martin Albrecht, Volker Braun to Martin Albrecht, Volker Braun, Simon King
- Status changed from needs_review to positive_review
Replying to vbraun:
I'm happy with trac10903_lgamma_doc.patch. Maybe Simon can quickly OK the trac_10903_docbuild_fix.patch, then we would be good to go?
Sorry, I forgot to post before leaving office: Your patch manages to deduce the name of a deprecated Cython method. That certainly fixes the problem with the docs. With the patches applied as you stated it in the modified ticket description, both the doc tests and the references are fine.
Thus, positive review.
comment:87 follow-up: ↓ 96 Changed 8 years ago by
It seems, that kernel mod_raw.cc lacks the following:
#if defined(ppcMac_darwin) #define HAVE_ELF_SYSTEM #endif
I already reported that upstream.
comment:88 Changed 8 years ago by
- Status changed from positive_review to needs_work
- Work issues set to memory usage
Unfortunately, I discovered another serious issue with the new Singular (#11339 + #10903): there is an enormous virtual memory usage regression in the test
sage -t -long devel/sage/sage/crypto/mq/sr.py
It used to need about 1.2GB virtual memory, with the new Singular it needs 3.0GB. I measured this by setting ulimit -v
and checking whether the test succeeds, for example:
### The parentheses start a subshell such that the ulimit only affects ### that one command and not the whole shell. $ ( ulimit -v 2500000; ./sage -t -long devel/sage/sage/crypto/mq/sr.py ) sage -t -long "devel/sage/sage/crypto/mq/sr.py" [***** 0.000s] # doctest internal [***** 0.000s] # doctest internal [00029 0.040s] sr = mq.SR(Integer(1), Integer(1), Integer(1), Integer(4)) [...] [03326 0.000s] from sage.crypto.mq.sr import AllowZeroInversionsContext [03327 0.010s] sr = mq.SR(Integer(1),Integer(2),Integer(2),Integer(4)) [03328 0.000s] with AllowZeroInversionsContext(sr): [03331 0.000s] sr._allow_zero_inversions [***** 0.000s] # doctest internal [***** 0.000s] # doctest internal [***** 0.000s] # doctest internal [03355 0.010s] from sage.crypto.mq.sr import test_consistency Singular error: no more memory System 1604712k:1604712k Appl 1450267k/23501k Malloc 1032k/0k Valloc 1472736k/23501k Pages 368184/0 Regions 2932:2932 halt 14 [114.3 s]
I don't know whether this issue is upstream or caused by some of the Sage library patches, but it is certainly serious enough not to merge this ticket.
As a reference: before this ticket, every test passed with 2.2GB of RAM, with the most memory used by
sage -t -long devel/sage/sage/schemes/elliptic_curves/heegner.py
comment:89 Changed 8 years ago by
- Status changed from needs_work to needs_review
Since the singular update fixes bugs that returned an incorrect result, I think the increased memory usage should be a separate ticket. If its a problem for the build bots, just pick a smaller #long
example.
Or in other words: http://dilbert.com/strips/comic/1995-06-24
comment:90 Changed 8 years ago by
Volker, may I interpret your comment as "I think the increased memory usage is not a big deal and we should merge this ticket anyway"?
I don't know about the buildbot, I have not yet tested.
comment:91 follow-up: ↓ 92 Changed 8 years ago by
Yes, that is what I meant. The increased memory usage should not prevent us from merging the Singular update into sage-4.7.2.
Martin: Do you have any idea about what goes wrong in sr.py?
comment:92 in reply to: ↑ 91 Changed 8 years ago by
- Status changed from needs_review to needs_work
Replying to vbraun:
Yes, that is what I meant. The increased memory usage should not prevent us from merging the Singular update into sage-4.7.2.
I totally understand what you mean but I disagree. It's always hard when one has to make a choice of bugs: this ticket certainly fixes bugs but also introduces a serious bug. If it is a memory leak, this will certainly affect people using Sage, so I don't think we can ignore this so easily.
The increased memory usage is not small either: given that Sage needs about 0.8GB by itself, the extra memory allocated for that test jumped from 0.4GB to 2.2GB.
comment:93 Changed 8 years ago by
Thanks to Martin's suggestion, I tracked the memory usage to this doctest:
sage: from sage.crypto.mq.sr import test_consistency sage: test_consistency(1) # long time (80s on sage.math, 2011)
comment:94 Changed 8 years ago by
Working at #11900, I observe that I get a very massive regression with #10903:
sage: %time for E in cremona_curves([11..100]): S = E.integral_points(both_signs=False) CPU times: user 54.17 s, sys: 0.49 s, total: 54.66 s Wall time: 55.38 s
With sage-4.7.2.alpha2 plus #9138 (which causes some regression in other examples), the example only takes 16.48 s, and with #11900 it reduces to 13.22 s.
I am not totally sure that #11339/#10903 are really to blame for it, but it is at least possible.
comment:95 Changed 8 years ago by
In case someone should touch the spkg again, please also fix the Singular script generation in spkg-install
by changing $*
to "$@"
.
Do we still need the nanny CFLAGS
, overriding a user's settings and forcing -O2
on all platforms except Darwin?
comment:96 in reply to: ↑ 87 Changed 8 years ago by
Replying to AlexanderDreyer:
It seems, that kernel mod_raw.cc lacks the following:
#if defined(ppcMac_darwin) #define HAVE_ELF_SYSTEM #endif
I already reported that upstream.
FWIW, it builds on Linux PPC64 (POWER7, SLES 11, GCC 4.4.6), but it currently seems I get more doctest failures than before, i.e. Sage 4.7.2.alpha3 without #11339, the patches from here and the new spkg.
Have to investigate that further.
comment:97 Changed 8 years ago by
I found the bug, I had accidentally remove one line too many when I removed some debugging code I introduced in #11339. And that line was the p_Delete
in the polynomial destructor...
comment:98 Changed 8 years ago by
- Description modified (diff)
- Status changed from needs_work to needs_review
comment:99 Changed 8 years ago by
- Description modified (diff)
- Work issues memory usage deleted
Testing right now...
comment:100 Changed 8 years ago by
- Status changed from needs_review to positive_review
- Summary changed from Update Singular to 3-1-3-3. to Update Singular to 3-1-3-3
comment:101 Changed 8 years ago by
As far as I can tell, this does not cause a slowdown, see sage-devel.
comment:102 Changed 8 years ago by
- Merged in set to sage-4.7.2.alpha4
- Resolution set to fixed
- Status changed from positive_review to closed
comment:103 Changed 8 years ago by
This fails to build on OS X 10.4 PPC:
Machine: Darwin moufang.ugent.be 8.11.0 Darwin Kernel Version 8.11.0: Wed Oct 10 18:26:00 PDT 2007; root:xnu-792.24.17~1/RELEASE_PPC Power Macintosh powerpc Deleting directories from past builds of previous/current versions of singular-3-1-3-3.p0 Extracting package /Users/jdemeyer/sage-4.7.2.alpha4/spkg/standard/singular-3-1-3-3.p0.spkg ... -rw-r--r-- 1 jdemeyer jdemeyer 8420878 Oct 4 11:09 /Users/jdemeyer/sage-4.7.2.alpha4/spkg/standard/singular-3-1-3-3.p0.spkg Finished extraction **************************************************** Host system uname -a: Darwin moufang.ugent.be 8.11.0 Darwin Kernel Version 8.11.0: Wed Oct 10 18:26:00 PDT 2007; root:xnu-792.24.17~1/RELEASE_PPC Power Macintosh powerpc **************************************************** **************************************************** CC Version gcc -v Using built-in specs. Target: powerpc-apple-darwin8 Configured with: /private/var/tmp/gcc/gcc-5367.obj~1/src/configure --disable-checking -enable-werror --prefix=/usr --mandir=/share/man --enable-languages=c,objc,c++,obj-c++ --program-transform-name=/^[cg][^.-]*$/s/$/-4.0/ --with-gxx-include-dir=/include/c++/4.0.0 --with-slibdir=/usr/lib --build=powerpc-apple-darwin8 --host=powerpc-apple-darwin8 --target=powerpc-apple-darwin8 Thread model: posix gcc version 4.0.1 (Apple Computer, Inc. build 5367) **************************************************** ~/sage-4.7.2.alpha4/spkg/build/singular-3-1-3-3.p0/src ~/sage-4.7.2.alpha4/spkg/build/singular-3-1-3-3.p0 patching file factory/assert.h patching file Singular/configure ~/sage-4.7.2.alpha4/spkg/build/singular-3-1-3-3.p0 make[2]: *** No rule to make target `distclean'. Stop. rm: /Users/jdemeyer/sage-4.7.2.alpha4/local/bin/Singular*: No such file or directory creating cache ./config.cache checking uname for singular... ppcMac-darwin checking for gcc... gcc checking whether the C compiler (gcc -O3 -g -fPIC ) works... yes checking whether the C compiler (gcc -O3 -g -fPIC ) is a cross-compiler... no [...] g++ -O3 -g -fPIC -I.. -I/Users/jdemeyer/sage-4.7.2.alpha4/local -pipe -I. -I.. -I/Users/jdemeyer/sage-4.7.2.alpha4/local -I/Users/jdemeyer/sage-4.7.2.alpha4/local/include -I/Users/jdemeyer/sage-4.7.2.alpha4/local/include -I/Users/jdemeyer/sage-4.7.2.alpha4/local/include -fno-implicit-templates -I.. -I/Users/jdemeyer/sage-4.7.2.alpha4/local -DNDEBUG -DOM_NDEBUG -DppcMac_darwin -DHAVE_CONFIG_H \ -o Singular \ tesths.cc iparith.o mpsr_Tok.o claptmpl.o\ grammar.o scanner.o attrib.o blackbox.o eigenval_ip.o extra.o fehelp.o feOpt.o ipassign.o ipconv.o ipid.o iplib.o ipprint.o ipshell.o newstruct.o lists.o sdb.o fglm.o interpolation.o silink.o ssiLink.o subexpr.o janet.o wrapper.o libparse.o sing_win.o gms.o pcv.o maps_ip.o walk.o walk_ip.o cntrlc.o misc_ip.o calcSVD.o pipeLink.o Minor.o MinorProcessor.o MinorInterface.o bigintm.o pyobject_setup.o bbcone.o bbfan.o denom_list.o minpoly.o slInit_Static.o mpsr_Put.o mpsr_PutPoly.o mpsr_GetPoly.o mpsr_sl.o mpsr_Get.o mpsr_GetMisc.o mpsr_Error.o ndbm.o sing_dbm.o -dynamic -L/Users/jdemeyer/sage-4.7.2.alpha4/local/kernel -L../kernel -lkernel -L/Users/jdemeyer/sage-4.7.2.alpha4/local/lib -L/Users/jdemeyer/sage-4.7.2.alpha4/local/lib -ldl -lm -lsingfac -lsingcf -lntl -lgmp -lreadline -lncurses -lm -lomalloc_ndebug ../kernel/mmalloc.o /usr/bin/ld: warning -L: directory name (/Users/jdemeyer/sage-4.7.2.alpha4/local/kernel) does not exist /usr/bin/ld: Undefined symbols: _dynl_close _dynl_error _dynl_open _dynl_sym collect2: ld returned 1 exit status make[4]: *** [Singular] Error 1 make[3]: *** [install] Error 1 make[2]: *** [/Users/jdemeyer/sage-4.7.2.alpha4/local/bin/Singular-3-1-3] Error 2
comment:104 follow-ups: ↓ 105 ↓ 106 ↓ 109 Changed 8 years ago by
Would that be fixed by Alexander's suggestion in http://trac.sagemath.org/sage_trac/ticket/10903#comment:87
I don't have access to a PPC Mac.
comment:105 in reply to: ↑ 104 Changed 8 years ago by
Replying to vbraun:
Would that be fixed by Alexander's suggestion in http://trac.sagemath.org/sage_trac/ticket/10903#comment:87
I think so. Unfortunately I do not have spare time to provida a proper patches spkg :-(
I don't have access to a PPC Mac.
I could test the spkg.
comment:106 in reply to: ↑ 104 Changed 8 years ago by
- Resolution fixed deleted
- Status changed from closed to new
Replying to vbraun:
Would that be fixed by Alexander's suggestion in http://trac.sagemath.org/sage_trac/ticket/10903#comment:87
Yes. Making a new spkg...
comment:107 Changed 8 years ago by
- Description modified (diff)
comment:108 follow-ups: ↓ 110 ↓ 116 Changed 8 years ago by
- Status changed from new to needs_review
New spkg needs review: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. See singular-3-1-3-3.p0-p1.diff for changes.
comment:109 in reply to: ↑ 104 ; follow-ups: ↓ 111 ↓ 112 Changed 8 years ago by
Replying to vbraun:
Would that be fixed by Alexander's suggestion in http://trac.sagemath.org/sage_trac/ticket/10903#comment:87
Well, smells more as if --without-dynamic-kernel
does no longer work... (which we pass on any OS but Linux)
Regarding the ELF fix, cf. this comment...
(I also thought Alexander's fix was already incorporated into the p0 spkg.)
comment:110 in reply to: ↑ 108 ; follow-up: ↓ 113 Changed 8 years ago by
Replying to jdemeyer:
New spkg needs review: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. See singular-3-1-3-3.p0-p1.diff for changes.
* Exit when `patch` fails. We do not print an error message, patch is verbose enough by itself.
Not true. patch
's error messages do not start with Error
nor Failed
, such that it's hard to find out which spkg actually failed to build / install in parallel builds.
If you don't want to apply the patches in a loop (prepending numbers to [some of] the patches could force the proper order), you should use something like
patch_error() { echo >&2 "Error: Patch failed to apply." exit 1 } ... patch -p1 < ... || patch_error ...
or use the weirdly-named success()
function.
comment:111 in reply to: ↑ 109 Changed 8 years ago by
Replying to leif:
Well, smells more as if
--without-dynamic-kernel
does no longer work... (which we pass on any OS but Linux)
Indeed - intended or not - it's still in the spkg.
Regarding the ELF fix, cf. this comment...
It's not that easy: OS X question does not define ELF, because it's MACH, not an ELF. Singular just needs an ELF-compatible dlopen here. (So maybe HAVE_ELF_SYSTEM
is a misleading macro name.
comment:112 in reply to: ↑ 109 Changed 8 years ago by
Replying to leif:
Well, smells more as if
--without-dynamic-kernel
does no longer work... (which we pass on any OS but Linux)
But --with-dl
is active. Even thought we do not load parts of the kernel dynamically, we can load other dynamic modules (for instance, there is an experimental python interface for Singular).
comment:113 in reply to: ↑ 110 Changed 8 years ago by
- Status changed from needs_review to needs_info
Replying to leif:
Replying to jdemeyer:
New spkg needs review: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. See singular-3-1-3-3.p0-p1.diff for changes.
* Exit when `patch` fails. We do not print an error message, patch is verbose enough by itself.Not true.
patch
's error messages do not start withError
norFailed
, such that it's hard to find out which spkg actually failed to build / install in parallel builds.
What do you mean with this? How is the message
Error: Patch failed to apply.
more clear than something like
1 out of 7 hunks FAILED -- saving rejects to file sage/rings/number_field/number_field_element.pyx.rej
In any case, I don't see what this has to do with parallel builds.
Can anybody tell me how we should proceed?
comment:114 follow-up: ↓ 115 Changed 8 years ago by
Leif wants to be able to grep the install.log for "Error", I think. Thats not particularly high on my agenda, though. Especially since patch will either already fail at the developer's machine or always work (assuming that the hardware is working).
comment:115 in reply to: ↑ 114 ; follow-up: ↓ 117 Changed 8 years ago by
Replying to vbraun:
Leif wants to be able to grep the install.log for "Error", I think. Thats not particularly high on my agenda, though. Especially since patch will either already fail at the developer's machine or always work (assuming that the hardware is working).
BTW: So it's not mandatory anymore to "unroll" patches in the spkg, i.e. to deliver the patched file and use cp
?
Despite of this, you're right from my point of view.
comment:116 in reply to: ↑ 108 Changed 8 years ago by
Replying to jdemeyer:
New spkg needs review: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. See singular-3-1-3-3.p0-p1.diff for changes.
The new spkg builds and installs on SuSE 11 amd64 and OS X 10.5 ppc. The tests are running (fine so far). Somebody should test it on Solaris though.
comment:117 in reply to: ↑ 115 ; follow-up: ↓ 118 Changed 8 years ago by
Replying to AlexanderDreyer:
BTW: So it's not mandatory anymore to "unroll" patches in the spkg, i.e. to deliver the patched file and use
cp
?
Not any more, we have included patch as a standard spkg!
comment:118 in reply to: ↑ 117 Changed 8 years ago by
comment:119 follow-up: ↓ 120 Changed 8 years ago by
Replying to jdemeyer:
Replying to leif:
Replying to jdemeyer:
New spkg needs review: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. See singular-3-1-3-3.p0-p1.diff for changes.
* Exit when `patch` fails. We do not print an error message, patch is verbose enough by itself.
Not true.
patch
's error messages do not start withError
norFailed
, such that it's hard to find out which spkg actually failed to build / install in parallel builds.What do you mean with this? How is the message
Error: Patch failed to apply.
more clear than something like
1 out of 7 hunks FAILED -- saving rejects to file sage/rings/number_field/number_field_element.pyx.rej
It isn't more clear (the "Error: ..." would be printed in addition anyway, although an "ordinary" user would perhaps not know what a hunk is or the message is all about), but you can easily grep for it, so every error message should start with "Failed" or (preferably) "Error".
(Volker, btw., the new ATLAS spkg is an IMHO poor counterexample to that, as it raises unhandled exceptions such that one gets tracebacks instead of meaningful error messages.)
The Sage library, which is built in parallel by default, is also a bad exception to this; it's often hard to see what really failed, not to mention the odd warnings about -Wstrict-prototypes
not being valid for C++.
In any case, I don't see what this has to do with parallel builds.
Well, the screen output (and hence install.log
) is pretty messed up in parallel builds, and make
usually doesn't stop immediately if one spkg fails to build, but continues and waits for other jobs to finish. [Another issue is that stdout
and stderr
frequently get "out of sync".]
To see what went wrong or which spkgs failed to build / didn't pass the (self-)test suite, one can usually do
$ egrep '^(Error|Failed) ' spkg/logs/*
with variations on -i
, -h
, -n
and -A/B/C...
etc.
It wouldn't be bad to add something similar to the top-level Makefile
; since we always append to the logs, we could create a fresh temporary log file upon every build and let sage-spkg
write to it whenever a build or running spkg-check
failed, such that an informative summary can be printed at the end, at least on errors. (We could also log the so far successfully built spkgs somewhere, such that one can tail -f
that file, perhaps even with timings or date/time, "i/N spkgs built".)
Can anybody tell me how we should proceed?
Well, it's a minor issue of course. But we should IMHO avoid such in the future.
I still cannot really tell whether this spkg (along with the patches) causes more doctests to fail on Linux PPC64 (the Skynet machine Silius; SLES 11.1, POWER7), since I've experimented with various things on that machine, and I haven't yet had the time to reliably compare builds which really only differ w.r.t. this ticket (and #11339).
In case this ticket should cause more problems on that platform, we could (hopefully) fix these on a follow-up, since there are other spkgs which apparently are still broken on it (also depending on the compiler version and options), so building Sage there is currently pretty experimental anyway.
comment:120 in reply to: ↑ 119 Changed 8 years ago by
Replying to leif:
To see what went wrong or which spkgs failed to build / didn't pass the (self-)test suite, one can usually do
$ egrep '^(Error|Failed) ' spkg/logs/*with variations on
-i
,-h
,-n
and-A/B/C...
etc.
But every failed spkg also prints the message
sage: An error occurred while installing mpir-2.1.3.p5 Please email sage-devel http://groups.google.com/group/sage-devel explaining the problem and send the relevant part of of /home/buildbot/build/sage/iras-1/iras_full/build/sage-4.7.2.alpha4/install.log. Describe your computer, operating system, etc. If you want to try to fix the problem yourself, *don't* just cd to /home/buildbot/build/sage/iras-1/iras_full/build/sage-4.7.2.alpha4/spkg/build/mpir-2.1.3.p5 and type 'make check' or whatever is appropriate. Instead, the following commands setup all environment variables correctly and load a subshell for you to debug the error: (cd '/home/buildbot/build/sage/iras-1/iras_full/build/sage-4.7.2.alpha4/spkg/build/mpir-2.1.3.p5' && '/home/buildbot/build/sage/iras-1/iras_full/build/sage-4.7.2.alpha4/sage' -sh) When you are done debugging, you can type "exit" to leave the subshell.
One can easily grep
for this to find out which package caused the problem and then look at spkg/logs/$PACKAGENAME.log
.
comment:121 Changed 8 years ago by
- Status changed from needs_info to needs_review
New spkg with error messages for failed patches: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg. Needs review.
comment:122 follow-ups: ↓ 124 ↓ 125 Changed 8 years ago by
- Status changed from needs_review to positive_review
Looks good. I take it from your comment that it works on OSX/PPC. Positive review.
comment:123 Changed 8 years ago by
- Resolution set to fixed
- Status changed from positive_review to closed
comment:124 in reply to: ↑ 122 Changed 8 years ago by
Replying to vbraun:
Looks good. I take it from your comment that it works on OSX/PPC. Positive review.
For the record: I also managed to build, install and succesfully run all tests on SuSE 11.1 SP1 and OSX 10.6 ppc.
comment:125 in reply to: ↑ 122 Changed 8 years ago by
Replying to vbraun:
Looks good.
Looks better.
Btw., I sometimes also grep for "An error"
. Grepping for "^(Error|Failed) "
(perhaps with context, which doesn't make much sense in the former case) immediately shows you what went wrong. Of course the logs of spkgs that are themselves built in parallel are sometimes harder to read.
comment:126 Changed 8 years ago by
trac_10903_singular-fixes.patch seems to remove the functionality of the parameter omit_underscore_names in sageinspect.py's sage_getvariablename(). Was that done on purpose?
Last time I tried libsingular from 3.1.2 didn't play well with sage (sage couldn't build). It is documented somewhere on sage-devel.