malb commented 13 years ago

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

Install: http://boxen.math.washington.edu/home/jdemeyer/spkg/singular-3-1-3-3.p1.spkg
Apply the two dependency patches from #11339
Apply: attachment: 10903_flat.patch

Depends on #11339

CC: @zimmermann6 @simon-king-jena

Component: packages: standard

Keywords: singular SageDays34 sd34

Author: Burcin Erocal, Martin Albrecht, Volker Braun, Simon King

Reviewer: Martin Albrecht, Volker Braun, Simon King

Merged: sage-4.7.2.alpha4

Issue created by migration from https://trac.sagemath.org/ticket/10903

vbraun commented 12 years ago

comment:46

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

vbraun commented 12 years ago

comment:47

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

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

comment:48

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.

vbraun commented 12 years ago

comment:49

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.

vbraun commented 12 years ago

Description changed:

--- 
+++ 
@@ -9,8 +9,7 @@

 # Installation instructions

-* **Install:** http://www.stp.dias.ie/~vbraun/Sage/spkg/singular-3-1-3-3.spkg (md5sum: 2557ec04e765c971c1ed6ebf3121f8ac)
-
+* **Install:** http://www.stp.dias.ie/~vbraun/Sage/spkg/singular-3-1-3-3.p0.spkg (md5sum: dae730f0ed63e4b3f173ba08ac45ee1f)

 * Apply the two dependency patches from #11339

vbraun commented 12 years ago

comment:50

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

malb commented 12 years ago

Description changed:

--- 
+++ 
@@ -13,4 +13,4 @@

 * Apply the two dependency patches from #11339

-* **Apply:** [attachment: 10903_singular-3-1-4.patch](https://github.com/sagemath/sage/files/ticket10903/10903_singular-3-1-4.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz)
+* **Apply:** [attachment: trac_10903_singular-3-1-3-3.patch](https://github.com/sagemath/sage-prod/files/10652282/trac_10903_singular-3-1-3-3.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz)

malb commented 12 years ago

comment:51

Attachment: trac_10903_singular-3-1-3-3.patch.gz

malb commented 12 years ago

comment:52

Thank you Volker for taking care of the SPKG!

vbraun commented 12 years ago

comment:53

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

malb commented 12 years ago

comment:54

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

vbraun commented 12 years ago

comment:55

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

vbraun commented 12 years ago

comment:56

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

malb commented 12 years ago

Reviewer: Martin Albrecht, Volker Braun

malb commented 12 years ago

comment:57

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?

vbraun commented 12 years ago

comment:58

I replaced the singular-3-1-3-3.p0.spkg at the ticket's URL with Martin's version.

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

comment:59

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’

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

comment:60

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

vbraun commented 12 years ago

comment:61

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.

vbraun commented 12 years ago

Description changed:

--- 
+++ 
@@ -9,7 +9,7 @@

 # Installation instructions

-* **Install:** http://www.stp.dias.ie/~vbraun/Sage/spkg/singular-3-1-3-3.p0.spkg (md5sum: dae730f0ed63e4b3f173ba08ac45ee1f)
+* **Install:** http://www.stp.dias.ie/~vbraun/Sage/spkg/singular-3-1-3-3.p0.spkg

 * Apply the two dependency patches from #11339

jdemeyer commented 12 years ago

comment:62

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.

simon-king-jena commented 12 years ago

comment:63

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'

vbraun commented 12 years ago

comment:64

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

simon-king-jena commented 12 years ago

comment:65

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: attachment: 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?

simon-king-jena commented 12 years ago

comment:66

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

simon-king-jena commented 12 years ago

Prevent __name__ of deprecated function aliases from returning None

simon-king-jena commented 12 years ago

Changed author from Burcin Erocal, Martin Albrecht, Volker Braun to Burcin Erocal, Martin Albrecht, Volker Braun, SImon King

simon-king-jena commented 12 years ago

Description changed:

--- 
+++ 
@@ -13,4 +13,4 @@

 * Apply the two dependency patches from #11339

-* **Apply:** [attachment: trac_10903_singular-3-1-3-3.patch](https://github.com/sagemath/sage-prod/files/10652282/trac_10903_singular-3-1-3-3.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz)
+* **Apply:** [attachment: trac_10903_singular-3-1-3-3.patch](https://github.com/sagemath/sage-prod/files/10652282/trac_10903_singular-3-1-3-3.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz), [attachment: trac10903_fix_name.patch](https://github.com/sagemath/sage-prod/files/10652283/trac10903_fix_name.patch.gz)

simon-king-jena commented 12 years ago

comment:67

Attachment: trac10903_fix_name.patch.gz

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

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

Changed author from Burcin Erocal, Martin Albrecht, Volker Braun, SImon King to Burcin Erocal, Martin Albrecht, Volker Braun, Simon King

83660e46-0051-498b-a8c1-f7a7bd232b5a commented 12 years ago

comment:68

Otherwise we get a new contributor...

vbraun commented 12 years ago

comment:69

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

simon-king-jena commented 12 years ago

comment:70

Replying to @nexttime:

Otherwise we get a new contributor...

Just one of the many errors I committed this morning...

simon-king-jena commented 12 years ago

comment:71

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.

vbraun commented 12 years ago

comment:72

Replying to @simon-king-jena:

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.

simon-king-jena commented 12 years ago

comment:73

Replying to @vbraun:

Replying to @simon-king-jena:

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.

simon-king-jena commented 12 years ago

Description changed:

--- 
+++ 
@@ -13,4 +13,4 @@

 * Apply the two dependency patches from #11339

-* **Apply:** [attachment: trac_10903_singular-3-1-3-3.patch](https://github.com/sagemath/sage-prod/files/10652282/trac_10903_singular-3-1-3-3.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz), [attachment: trac10903_fix_name.patch](https://github.com/sagemath/sage-prod/files/10652283/trac10903_fix_name.patch.gz)
+* **Apply:** [attachment: trac_10903_singular-3-1-3-3.patch](https://github.com/sagemath/sage-prod/files/10652282/trac_10903_singular-3-1-3-3.patch.gz), [attachment: trac_10903_singular-fixes.patch](https://github.com/sagemath/sage-prod/files/10652281/trac_10903_singular-fixes.patch.gz), [attachment: trac10903_fix_name.patch](https://github.com/sagemath/sage-prod/files/10652283/trac10903_fix_name.patch.gz), [attachment: trac10903_lgamma_doc.patch](https://github.com/sagemath/sage-prod/files/10652284/trac10903_lgamma_doc.patch.gz)

simon-king-jena commented 12 years ago

comment:74

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

vbraun commented 12 years ago

comment:75

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?

vbraun commented 12 years ago

comment:76

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

simon-king-jena commented 12 years ago

Attachment: trac10903_lgamma_doc.patch.gz

Fix doc formatting of sage.symbolic.expression

simon-king-jena commented 12 years ago

comment:77

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

vbraun commented 12 years ago

comment:78

Replying to @simon-king-jena:

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?

simon-king-jena commented 12 years ago

comment:79

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?

simon-king-jena commented 12 years ago

comment:80

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?

simon-king-jena commented 12 years ago

comment:81

Replying to @simon-king-jena:

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?

vbraun commented 12 years ago

comment:82

What my patch does fix is this:

sage: print x._is_constant.__name__
_is_constant

simon-king-jena commented 12 years ago

comment:83

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.

vbraun commented 12 years ago

Attachment: trac_10903_docbuild_fix.patch.gz

Updated patch

vbraun commented 12 years ago

comment:84

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

sagemath / sage

Update Singular to 3-1-3-3 #10903

Singular spkg upgrade

Sage libsingular upgrade

Installation instructions