Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#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 jdemeyer)

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

Attachments (8)

trac_10903_singular-fixes.patch (23.5 KB) - added by vbraun 8 years ago.
Updated patch
trac_10903_singular-3-1-3-3.patch (22.6 KB) - added by malb 8 years ago.
trac10903_fix_name.patch (1.2 KB) - added by SimonKing 8 years ago.
Prevent __name__ of deprecated function aliases from returning None
trac10903_lgamma_doc.patch (2.5 KB) - added by SimonKing 8 years ago.
Fix doc formatting of sage.symbolic.expression
trac_10903_docbuild_fix.patch (1.9 KB) - added by vbraun 8 years ago.
Updated patch
trac_10903_memleak_fix.patch (781 bytes) - added by vbraun 8 years ago.
one-
10903_flat.patch (45.4 KB) - added by jdemeyer 8 years ago.
All patches together in one patch
singular-3-1-3-3.p0-p1.diff (224.7 KB) - added by jdemeyer 8 years ago.
diff for the new Singular spkg, for review only

Download all attachments as: .zip

Change History (134)

comment:1 Changed 9 years ago by zimmerma

  • Cc zimmerma added

comment:2 Changed 9 years ago by fbissey

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.

comment:4 Changed 9 years ago by fbissey

sorry for the double pasting the mouse is vintage.

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

comment:6 Changed 8 years ago by kedlaya

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 fbissey

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: Changed 8 years ago by 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.

comment:9 in reply to: ↑ 5 Changed 8 years ago by leif

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 leif

  • 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 leif

  • Keywords sd34 added

comment:12 follow-up: Changed 8 years ago by malb

  • Authors set to Burcin Erocal, Martin Albrecht
  • 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 SimonKing

  • Cc SimonKing added

comment:14 in reply to: ↑ 12 Changed 8 years ago by leif

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: Changed 8 years ago by 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.

comment:16 in reply to: ↑ 15 Changed 8 years ago by leif

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 malb

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 malb

  • Dependencies set to #11339

comment:19 Changed 8 years ago by malb

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 malb

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 malb

  • Description modified (diff)

comment:22 Changed 8 years ago by malb

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 malb

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 malb

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 malb

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 malb

on sage.math

sage -t  -long -force_lib devel/sage/sage/crypto/mq/sr.py

also crashes.

comment:27 Changed 8 years ago by zimmerma

Martin, good, you are making progress!

Paul

comment:28 Changed 8 years ago by malb

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 vbraun

  • Description modified (diff)

comment:30 Changed 8 years ago by malb

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 malb

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: Changed 8 years ago by strogdon

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 strogdon

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 malb

  • 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 malb

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:37 Changed 8 years ago by zimmerma

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 malb

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 zimmerma

no, I only applied the first two patches (from #11339) since the description of that ticket (#10903) says to first install the spkg, then to apply the patches. Should I first apply all patches?

Paul

comment:40 Changed 8 years ago by malb

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 vbraun

  • Authors changed from Burcin Erocal, Martin Albrecht to Burcin Erocal, Martin Albrecht, Volker Braun
  • 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 vbraun

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!

Changed 8 years ago by vbraun

Updated patch

comment:43 Changed 8 years ago by vbraun

  • Status changed from needs_info to needs_review

comment:44 Changed 8 years ago by vbraun

  • Description modified (diff)

comment:45 Changed 8 years ago by leif

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 vbraun

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 vbraun

  • Status changed from needs_review to needs_work

Spkg doesn't build on Solaris, will investigate...

comment:48 Changed 8 years ago by leif

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 vbraun

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 vbraun

  • 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 malb

comment:51 Changed 8 years ago by malb

  • Description modified (diff)

comment:52 Changed 8 years ago by malb

Thank you Volker for taking care of the SPKG!

comment:53 Changed 8 years ago by vbraun

The new .p0.spkg builds on Mark (solaris sparc). Yay ;-)

comment:54 Changed 8 years ago by malb

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 vbraun

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 vbraun

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 malb

  • 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: Changed 8 years ago by vbraun

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 leif

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 leif

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 vbraun

  • 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: Changed 8 years ago by jdemeyer

  • 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 SimonKing

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 vbraun

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 SimonKing

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 SimonKing

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): ...".

Changed 8 years ago by SimonKing

Prevent __name__ of deprecated function aliases from returning None

comment:67 Changed 8 years ago by SimonKing

  • Authors changed from Burcin Erocal, Martin Albrecht, Volker Braun to Burcin Erocal, Martin Albrecht, Volker Braun, SImon King
  • 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:68 follow-up: Changed 8 years ago by leif

  • Authors changed from Burcin Erocal, Martin Albrecht, Volker Braun, SImon King to Burcin Erocal, Martin Albrecht, Volker Braun, Simon King

Otherwise we get a new contributor...

comment:69 follow-up: Changed 8 years ago by 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).

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 SimonKing

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: Changed 8 years ago by SimonKing

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: Changed 8 years ago by 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.

comment:73 in reply to: ↑ 72 Changed 8 years ago by SimonKing

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 SimonKing

  • 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: Changed 8 years ago by vbraun

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 vbraun

The trac_10903_docbuild_fix.patch now looks into Python and Cython classes. If all fails, it raises an error as Simon suggested.

Changed 8 years ago by SimonKing

Fix doc formatting of sage.symbolic.expression

comment:77 in reply to: ↑ 75 ; follow-up: Changed 8 years ago by SimonKing

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: Changed 8 years ago by vbraun

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 SimonKing

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: Changed 8 years ago by 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?

comment:81 in reply to: ↑ 80 Changed 8 years ago by SimonKing

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: Changed 8 years ago by vbraun

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 SimonKing

Replying to vbraun:

What my patch does fix is this:

Could you add that example as a doc test? Then probably it is better to use your patch, not mine.

Perhaps your trick could actually help me with something I could not solve at #11115.

Changed 8 years ago by vbraun

Updated patch

comment:84 Changed 8 years ago by vbraun

The updated patch adds doctest and improves the cython class detection.

comment:85 follow-up: Changed 8 years ago by vbraun

  • 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 SimonKing

  • 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: Changed 8 years ago by 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.

comment:88 Changed 8 years ago by jdemeyer

  • 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 vbraun

  • 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 jdemeyer

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: Changed 8 years ago by 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.

Martin: Do you have any idea about what goes wrong in sr.py?

comment:92 in reply to: ↑ 91 Changed 8 years ago by jdemeyer

  • 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 vbraun

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 SimonKing

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 leif

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 leif

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 vbraun

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...

Changed 8 years ago by vbraun

one-

comment:98 Changed 8 years ago by vbraun

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

comment:99 Changed 8 years ago by jdemeyer

  • Description modified (diff)
  • Work issues memory usage deleted

Testing right now...

Changed 8 years ago by jdemeyer

All patches together in one patch

comment:100 Changed 8 years ago by jdemeyer

  • 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

The memory leak seems to be fixed, so positive_review. Thanks Volker!

I will do some timing tests, also regarding #9138 and #11900. So if I can confirm Simon King's observations, this ticket will need work.

comment:101 Changed 8 years ago by jdemeyer

As far as I can tell, this does not cause a slowdown, see sage-devel.

comment:102 Changed 8 years ago by jdemeyer

  • 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 jdemeyer

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: Changed 8 years ago by vbraun

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 AlexanderDreyer

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 jdemeyer

  • 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 jdemeyer

  • Description modified (diff)

comment:108 follow-ups: Changed 8 years ago by jdemeyer

  • Status changed from new to needs_review

comment:109 in reply to: ↑ 104 ; follow-ups: Changed 8 years ago by leif

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: Changed 8 years ago by 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 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 AlexanderDreyer

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 AlexanderDreyer

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 jdemeyer

  • 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 with Error nor Failed, 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: Changed 8 years ago by 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).

comment:115 in reply to: ↑ 114 ; follow-up: Changed 8 years ago by AlexanderDreyer

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 AlexanderDreyer

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: Changed 8 years ago by vbraun

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 AlexanderDreyer

Replying to vbraun:

Not any more, we have included patch as a standard spkg!

Nice!

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

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 with Error nor Failed, 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 jdemeyer

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.

Changed 8 years ago by jdemeyer

diff for the new Singular spkg, for review only

comment:121 Changed 8 years ago by jdemeyer

  • 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: Changed 8 years ago by vbraun

  • 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 jdemeyer

  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:124 in reply to: ↑ 122 Changed 8 years ago by AlexanderDreyer

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 leif

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 saraedum

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?

Note: See TracTickets for help on using tickets.