#12883 closed enhancement (fixed)
Update LinBox to most recent upstream release
Reported by:  pcpa  Owned by:  cpernet 

Priority:  major  Milestone:  sage5.4 
Component:  linbox  Keywords:  
Cc:  jpflori  Merged in:  sage5.4.beta1 
Authors:  Paulo César Pereira de Andrade, Martin Albrecht  Reviewers:  Volker Braun, Jeroen Demeyer 
Report Upstream:  Fixed upstream, but not in a stable release.  Work issues:  
Branch:  Commit:  
Dependencies:  #13118, #12840, #12841, #13164  Stopgaps: 
Description (last modified by )
The version of LinBox? used in Sage is 4 years old, we should update.
 Observe: Dependencies
 Install: http://sage.math.washington.edu/home/malb/spkgs/fflas_ffpack1.6.0.spkg
 Install: http://boxen.math.washington.edu/home/jdemeyer/spkg/linbox1.3.2.spkg
 Apply: matrix_modn_dense_no_linbox.patch
 Apply: sagelinbox.2.patch
 Apply: trac_12883_linbox_deps.patch and trac_12883spkginstall.patch to
SAGE_ROOT
repository  Apply: trac_12883_hg_ignore_fflas_config.patch to the scripts (
SAGE_LOCAL/bin
) repository.
SPKG Repositories:
Note for release manager: This ticket must be merged simultaneously with #13164, they depend on each other.
Attachments (11)
Change History (101)
comment:1 Changed 7 years ago by
comment:2 Changed 7 years ago by
Sorry for lack of a better description. I am trying to address and submit reports to involved upstream parties about some patches and version issues of some components, while checking how far I can go building sagemath in Fedora, and using Fedora packages.
For example, the first error:
error: 'linbox_modn_dense_echelonize' was not declared in this scope
in /usr/include/linbox/linboxsage.h
I see only
EXTERN unsigned long int linbox_modn_dense_echelonize_double (double modulus, double*matrix, size_t nrows, size_t ncols); EXTERN unsigned long int linbox_modn_dense_echelonize_float (float modulus, float * matrix,size_t nrows, size_t ncols);
Same happens for linbox_modn_dense_delete_array
that now have prototypes for linbox_modn_dense_delete_array_double
and linbox_modn_dense_delete_array_float
.
Also linbox_integer_dense_matrix_matrix_multiply
changed prototype from
/* ans must be a preallocated and preinitialized array of GMP ints. */ EXTERN int linbox_integer_dense_matrix_matrix_multiply(mpz_t** ans, mpz_t **A, mpz_t **B, size_t A_nr, size_t A_nc, size_t B_nr, size_t B_nc);
in the sagemath linbox1.1.6 spkg, to
/* ans must be a preallocated and preinitialized array of GMP ints. */ int linbox_integer_dense_matrix_matrix_multiply (mpz_t** ans, mpz_t **A, mpz_t **B, size_t m, size_t n, size_t k);
in linbox1.2.2.
I am trying to have upstream aware of these issues, and if not possible to get components working, I can try to get an extra linbox 1.1.6 package in Fedora.
comment:3 Changed 7 years ago by
Upstream linbox 1.2.2 should need this patch
 linbox1.2.2/interfaces/sage/linboxsage.h.orig 20120426 16:25:37.500132509 0300 +++ linbox1.2.2/interfaces/sage/linboxsage.h 20120426 16:26:00.705133519 0300 @@ 48,7 +48,9 @@ EXTERN void linbox_modn_dense_delete_array_double (double *f); EXTERN void linbox_modn_dense_delete_array_float (float *f);  +template <class Element> +unsigned long int linbox_modn_dense_echelonize (Element modulus, Element* matrix, + size_t nrows, size_t ncols); EXTERN unsigned long int linbox_modn_dense_echelonize_double (double modulus, double*matrix, size_t nrows, size_t ncols); EXTERN unsigned long int linbox_modn_dense_echelonize_float (float modulus, float * matrix,size_t nrows, size_t ncols);
But it is not yet enough because the sage matrix interface was changed from "jagged pointers" to a single sequential vector.
I believe I could make a patch for sage/libs/linbox/linbox.{pxd,pyx}
but I do not feel confident about other places where sage interfaces to linbox matrices.
comment:4 Changed 7 years ago by
The attached patches allow continuing experimenting with a new sagemath rpm.
The patches are only tested to know they make sagemath and linbox agree on api/abi, but no testing done.
There were missing prototypes/templates in linboxsage.h as well as wrong ones.
I am not fully confident that some place may do wrong dereferencing of matrix fields due to upstream change from a vector of rows where every row is an allocated pointer to a single memory chunk.
Another part I am not confident is that I did the correct logic in the change from
A_nr, A_nc, B_nr, B_nc to test in sage code if A_nr != B_nc: ...
, and convert the values of m, n, k
as
A_nr, A_nc, B_nr
.
This is not a proposal of a final patch, but a extra call for more feedback. As this was also the first time I actually did look at linbox code, so, just patched the interfaces to make both sides agree.
comment:5 Changed 7 years ago by
 Milestone changed from sage5.0 to sagewishlist
Is it reasonably to define mod_int to C long?
If not, safest approach should be to use uint32_t I believe, otherwise, should require coding 64 bit templates in linbox 1.2.2.
The new attached fflasffpack64bit.patch is required to generate proper 64 bit code, and the updated linboxsagemath.patch should be very close to the final format, but note that in 64 bit arch, linbox 1.2.2 with the attached patches needs also to have D__LINBOX_HAVE_INT64=1 in CFLAGS and CXXFLAGS.
I should update the sage4.8linbox.patch at some point, as the initial patch I attached was only enough to get to another build failure in Fedora. But the next update should be for sage5.0.
comment:6 Changed 7 years ago by
 Summary changed from Please consider updating to linbox 1.2.2 to Update LinBox to most recent upstream release
I cut new SPKGs for LinBox and FFLAS/FFPACK:
http://sage.math.washington.edu/home/malb/spkgs/fflas_ffpack1.5.0.spkg
http://sage.math.washington.edu/home/malb/spkgs/linbox1.3.0.spkg
I am tracking SPKG repositories here:
I will go through your patches now.
comment:7 Changed 7 years ago by
 Description modified (diff)
comment:8 Changed 7 years ago by
 Description modified (diff)
comment:9 Changed 7 years ago by
 Dependencies set to #12840, 12841, #9511
 Description modified (diff)
comment:10 Changed 7 years ago by
 Description modified (diff)
comment:11 Changed 7 years ago by
 Description modified (diff)
comment:12 Changed 7 years ago by
 Dependencies changed from #12840, 12841, #9511 to #12840, #12841, #9511
comment:13 Changed 7 years ago by
 Description modified (diff)
comment:14 Changed 7 years ago by
 Description modified (diff)
comment:15 Changed 7 years ago by
 Description modified (diff)
comment:16 Changed 7 years ago by
 Description modified (diff)
comment:17 Changed 7 years ago by
 Status changed from new to needs_review
All doctests pass on geom.math and my machine with 5.0.1.
comment:18 Changed 7 years ago by
 Status changed from needs_review to needs_work
 Work issues set to Julien's SPKG patch, update for deps
Changed 7 years ago by
comment:19 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to needs_review
 Work issues Julien's SPKG patch, update for deps deleted
Needs review.
comment:20 followup: ↓ 21 Changed 7 years ago by
matrix_modn_dense_no_linbox.patch is full of unnecessary whitespace changes that only serve to break any other patch on trac.
comment:21 in reply to: ↑ 20 Changed 7 years ago by
Replying to vbraun:
matrix_modn_dense_no_linbox.patch is full of unnecessary whitespace changes that only serve to break any other patch on trac.
Fixed
comment:22 followup: ↓ 23 Changed 7 years ago by
What for/where is matrix_modn_dense
used now? I see its commented out in matrix_space.py
.
Minor nitpick: missing space in coefficients ofminpoly as a Python list
.
comment:23 in reply to: ↑ 22 Changed 7 years ago by
Replying to vbraun:
What for/where is
matrix_modn_dense
used now? I see its commented out inmatrix_space.py
.
Currently, none. But William is resurrecting it at #4968. I assume matrix_modn_dense_no_linbox.patch and #4968 will conflict. But I suggest to deal with this when the problem arises and when we know in which order they'll be merged.
Minor nitpick: missing space in
coefficients ofminpoly as a Python list
.
I'll fix that.
Changed 7 years ago by
comment:24 Changed 7 years ago by
Fixed.
comment:25 followup: ↓ 26 Changed 7 years ago by
I believe it is not necessary anymore to add DDISABLE_COMMENTATOR to CPPFLAGS. IT is added automatically in the right places when you configure with the sage interface.
comment:26 in reply to: ↑ 25 Changed 7 years ago by
Replying to fbissey:
I believe it is not necessary anymore to add DDISABLE_COMMENTATOR to CPPFLAGS. IT is added automatically in the right places when you configure with the sage interface.
I am inclined to postpone this change to another ticket, unless you are pretty certain it isn't needed. The reason I am suggesting this is, that if we get SIGSEGVs then on some exotic platform we are not sure whether it's the 1.3.2 update or that we dropped the DDISABLE_COMMENTATOR
. Of course, we could just try to readd it and see what happens, but given how slow and bureaucratic Sage development is these days, this could easily take a week to accomplish, which would be annoying. What do you think?
comment:27 Changed 7 years ago by
Keeping it is "mostly harmless". I just noticed when I created the ebuild for sageongentoo that even so I hadn't added it to the CPPFLAGS it was used in the compilation. Adding it to the CPPFLAGS made it appear twice in those places.
I would have to test with the whole set of new packages and patches to be absolutely certain of course. That could take a few days. And of course that would be on a nonexotic platform first. Make me think I need to put a 5.1beta on power7 to cover my exotic corner.
comment:28 Changed 7 years ago by
 Description modified (diff)
 Milestone changed from sagewishlist to sage5.1
 Reviewers set to Volker Braun
 Status changed from needs_review to positive_review
Looks good to me!
comment:29 Changed 7 years ago by
 Milestone changed from sage5.1 to sage5.2
comment:30 Changed 7 years ago by
 Status changed from positive_review to needs_work
In SPKG.txt
of fflas_ffpack1.6.0.spkg
, replace
=== fflasffpack1.6.0 (Martin Albrecht, 7 June 2012) ===
by
=== fflas_ffpack1.6.0 (Martin Albrecht, 7 June 2012) ===
comment:31 Changed 7 years ago by
 Milestone changed from sage5.2 to sagepending
comment:32 Changed 7 years ago by
Just curious  does this fix #12865? (Won't try myself.)
comment:33 Changed 7 years ago by
Yes, it does fix #12865.
comment:34 Changed 7 years ago by
 Status changed from needs_work to needs_review
SPKG.txt issue is fixed.
comment:35 Changed 7 years ago by
 Status changed from needs_review to positive_review
comment:36 Changed 7 years ago by
 Milestone changed from sagepending to sage5.3
comment:37 Changed 7 years ago by
 Status changed from positive_review to needs_work
There is a weird file .swp
in the linbox spkg.
comment:38 Changed 7 years ago by
 Cc jpflori added
comment:39 Changed 7 years ago by
I've reuploaded the package without that mysterious file (which is not in spkg repo as I've checked) at http://perso.telecomparistech.fr/~flori/sage/linbox1.3.2.spkg
No other changes to the spkg. So someone should put this back to positive review (after checking I did not break everything).
Edit: Swapped 2 and 3...
comment:40 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to positive_review
The new fflas spkg installs a new script in SAGE_LOCAL/bin
, which needs to be added to .hgignore
. I've included a trivial patch to that extent.
Rest looks good.
comment:41 Changed 7 years ago by
 Description modified (diff)
Rebased patch for sage5.3.beta1. Just a trivial rebase in module_list.py.
comment:42 Changed 7 years ago by
Should the root repo patch also touch spkg/install
, adding a line for fflas, like this?

spkg/install
diff git a/spkg/install b/spkg/install
a b ECLIB=`newest_version eclib` 296 296 ECM=`newest_version ecm` 297 297 ELLIPTIC_CURVES=`newest_version elliptic_curves` 298 298 EXTCODE=`newest_version extcode` 299 FFLASFFPACK=`newest_version fflas_ffpack` 299 300 FLINT=`newest_version flint` 300 301 FLINTQS=`newest_version flintqs` 301 302 FPLLL=`newest_version libfplll`
Or is this not needed for some reason?
comment:43 Changed 7 years ago by
Yes, I'm pretty sure thats required. Are you going to post a patch?
Changed 7 years ago by
comment:44 Changed 7 years ago by
 Description modified (diff)
Sure, here's a patch. Do I need to switch it back to "needs review"?
comment:45 Changed 7 years ago by
 Reviewers changed from Volker Braun to Volker Braun, Jeroen Demeyer
 Status changed from positive_review to needs_work
comment:46 Changed 7 years ago by
 Status changed from needs_work to needs_review
comment:47 Changed 7 years ago by
Why the change to the iml dependencies?
comment:48 Changed 7 years ago by
 Status changed from needs_review to needs_work
The linbox spkg still contains the garbage I mentioned before:
? .swp
comment:49 Changed 7 years ago by
 Description modified (diff)
 Status changed from needs_work to positive_review
Fixed. I guess vim poops into cwd when segfaulting (.swp was vim swap file), never heard of /tmp?!?
comment:50 Changed 7 years ago by
PS: The iml testsuite does link against ATLAS, even though libiml doesn't depend on it.
comment:51 Changed 7 years ago by
For what it's worth, building from scratch with these patches works (all long tests passed) on sage.math and on my OS X Lion box.
comment:52 followup: ↓ 53 Changed 7 years ago by
 Dependencies changed from #12840, #12841, #9511 to #13118, #12840, #12841, #9511,
 Status changed from positive_review to needs_work
comment:53 in reply to: ↑ 52 Changed 7 years ago by
comment:54 Changed 7 years ago by
I've added the changelog entries of linbox1.1.6p9  11 to the SPKG.txt and removed the duplicate. None of those changes are needed any more since upstream has long fixed the issues that gcc 4.7 uncovered and Martin cleaned up the spkginstall
.
I've also readded the spkgcheck
as disabledspkgcheck
and deleted the Makefile.am.patch. Right now the testsuite will not compile without commentator. Would be nice to have the testsuite but not on this ticket.
Likewise, it would be nice if the Sage developers could first search trac if there is already a new spkg before opening a ticket.
Since really only the description changed I'll set this ticket back to positive review.
comment:55 Changed 7 years ago by
 Status changed from needs_work to positive_review
comment:56 Changed 7 years ago by
 Report Upstream changed from None of the above  read trac for reasoning. to Reported upstream. No feedback yet.
For the record, I've noted the testsuite issues on the linbox mailinglist at https://groups.google.com/d/topic/linboxuse/z0AlEH8Iyug/discussion
comment:57 Changed 7 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
Testsuite issues are fixed in svn version 4370.
comment:58 followup: ↓ 60 Changed 7 years ago by
Is it intentional that the withgmp
, withgivaro
and withntl
and withblas
configure options were removed?
Secondly, by "rebasing" this way, you've lost the .hgtags
(and probably the whole hg
history) for some earlier versions. Not sure whether that's important, but it's something to be pointed out.
comment:59 Changed 7 years ago by
Also, the .p11 spkg has error messages to stderr, please don't revert this.
comment:60 in reply to: ↑ 58 Changed 7 years ago by
comment:61 Changed 7 years ago by
 Status changed from positive_review to needs_work
I think this really needs to be rebased properly. You've reverted some good changes from the .p9p11 versions. Also, try to keep the Mercurial history.
comment:62 Changed 7 years ago by
Also, there isn't any documentation of the fact and the reason of renaming spkgcheck
to disabledspkgcheck
.
comment:63 Changed 7 years ago by
I've wasted my coffee break in preserving the mercurial history of the bikeshedding that went into linbox1.1.6p(N+1) despite the fact that it is all being ripped out. Hopefully future generations of archaeologists will appreciate the effort.
I've also mentioned in the SPKG.txt that spgkcheck is disabled in case anybody can't figure it out from the filename.
Now that spkginstall sets withdefault
we don't need withfoo
for every foo.
Now errors go to stderr again.
comment:64 Changed 7 years ago by
 Status changed from needs_work to positive_review
comment:65 Changed 7 years ago by
 Dependencies changed from #13118, #12840, #12841, #9511, to #13118, #12840, #12841, #9511
 Milestone changed from sage5.3 to sage5.4
comment:66 followup: ↓ 68 Changed 7 years ago by
 Status changed from positive_review to needs_work
This change is still a regression:
if [ "$SAGE64" = yes ]; then  echo "Building a 64bit version of LinBox."  if [ z "$CFLAG64" ]; then  CFLAG64=m64  fi  CFLAGS="$CFLAGS $CFLAG64"  CXXFLAGS="$CXXFLAGS $CFLAG64"  CPPFLAGS="$CPPFLAGS $CFLAG64"  LDFLAGS="$LDFLAGS $CFLAG64" +if [ "$SAGE64" = "yes" ]; then + echo "64 bit build" + CFLAGS="m64 $CFLAGS "; export CFLAGS + CXXFLAGS="m64 $CXXFLAGS "; export CXXFLAGS + CPPFLAGS="m64 $CPPFLAGS "; export CPPFLAGS + LDFLAGS="m64 $LDFLAGS"; export LDFLAGS fi
comment:67 Changed 7 years ago by
Also this:
if [ z "$SAGE_LOCAL" ]; then  echo >&2 "Error: SAGE_LOCAL undefined  exiting..."  echo >&2 "Maybe run 'sage sh'?"  exit 1 +if [ "$SAGE_LOCAL" = "" ]; then + echo "SAGE_LOCAL undefined ... exiting"; + echo "Maybe run 'sage sh'?" + exit 1 fi
comment:68 in reply to: ↑ 66 ; followup: ↓ 75 Changed 7 years ago by
 Status changed from needs_work to positive_review
I did notice the removal of the undocumented and nonstandard CFLAG64
environment variable and I consider that a definite plus of the new spkginstall
.
The new spkg readds the stderr redirection in the SAGE_LOCAL
check.
comment:69 followup: ↓ 71 Changed 7 years ago by
It would be nice to add an hg tag for the new release.
comment:70 Changed 7 years ago by
I've added the tag. Of course this'll never be done consistently as long as sage pkg
doesn't check that the spkg name matches the tag.
comment:71 in reply to: ↑ 69 Changed 7 years ago by
Replying to jpflori:
It would be nice to add an hg tag for the new release.
This is also done automatically by the merger script when the spkg is merged into Sage.
comment:72 Changed 7 years ago by
Good to know. Was it mentionned anywhere in the developper guide or did I miss it or did I not read it again recently enough?
Additionnal question, what about committing changes or putting them in a queue or not committing anything and letting some script do it? Is there a prefered method to do it?
comment:73 Changed 7 years ago by
Committing changes is always required.
Note: with the latest Mercurial, you can use
hg commit amend
to add changes to the latest commit. So you can effectively emulate a queue with 1 patch like this.
comment:74 Changed 7 years ago by
This is totally offtopic, but versioning systems are most useful if you commit often. Two logically independent changes should be two commits, even if they end up in the same ticket/spkg. Reviewer nitpicks should be another commit. Any other questions on how to use mercurial should go to sagedevel.
comment:75 in reply to: ↑ 68 Changed 7 years ago by
 Status changed from positive_review to needs_work
Replying to vbraun:
I did notice the removal of the undocumented and nonstandard
CFLAG64
environment variable and I consider that a definite plus of the newspkginstall
.
Looks documented to me: http://sagemath.org/doc/installation/source.html#environmentvariables
(I do agree that we should get rid of CFLAG64
and SAGE64
in place of using CC
and CFLAGS
properly, but that's not the point of this ticket)
comment:76 Changed 7 years ago by
Something else I noted: why was the exit code for a failed patch changed from 1
to 1
? Normally, programs should use exit codes in the range 0127.
comment:77 followup: ↓ 80 Changed 7 years ago by
 Status changed from needs_work to positive_review
Oh silly me, I only searched the developer guide
for
CFLAG64
! I also like how the installation guide is dancing around the fact that CFLAG64
is currently useless since it is not used in all spgks, so if you were to use a compiler that doesn't understand its default m64
value breakage will ensue. So I'll concede that it is documented, but it still doesn't sound like a good idea to me.
I've fixed the offending exit code to be +1
.
comment:78 Changed 7 years ago by
comment:79 Changed 7 years ago by
 Dependencies changed from #13118, #12840, #12841, #9511 to #13118, #12840, #12841, #13164
comment:80 in reply to: ↑ 77 Changed 7 years ago by
Replying to vbraun:
I also like how the installation guide is dancing around the fact that
CFLAG64
is currently useless since it is not used in all spgks, so if you were to use a compiler that doesn't understand its defaultm64
value breakage will ensue. So I'll concede that it is documented, but it still doesn't sound like a good idea to me.
This is all very true, but no good excuse for sloppy rebasing...
comment:81 Changed 7 years ago by
 Description modified (diff)
 Status changed from positive_review to needs_work
comment:82 Changed 7 years ago by
 Status changed from needs_work to needs_review
Rebased from scratch, needs review.
comment:83 Changed 7 years ago by
 Description modified (diff)
comment:84 Changed 7 years ago by
 Merged in set to sage5.4.beta1
 Resolution set to fixed
 Status changed from needs_review to closed
comment:85 Changed 7 years ago by
Could this cause the following sporadic doctest error:
sage t long force_lib devel/sage/sage/modular/modform/ambient.py you are running out of primes. 1000 coprime primes found********************************************************************** File "/release/buildbot/sage/sage1/sage_upgrade_5.2/build/sage5.5.beta0/devel/sagemain/sage/modular/modform/ambient.py", line 815: sage: [f[0]%p for p in prime_range(100)] # long time (0s, depends on above) Expected: [0, 0, 0, 0, 1, 9, 2, 7, 0, 0, 0, 0, 1, 12, 9, 16, 37, 0, 21, 11, 70, 22, 0, 58, 76] Got: [1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1] ********************************************************************** File "/release/buildbot/sage/sage1/sage_upgrade_5.2/build/sage5.5.beta0/devel/sagemain/sage/modular/modform/ambient.py", line 817: sage: [f[42]%p for p in prime_range(100)] # long time (0s, depends on above) Expected: [0, 0, 4, 0, 10, 4, 4, 8, 12, 1, 23, 13, 10, 27, 20, 13, 16, 59, 53, 41, 11, 13, 12, 6, 82] Got: [0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0] **********************************************************************
(the string "you are running out of primes" certainly comes from the new LinBox package).
comment:86 Changed 7 years ago by
I've posted this question to the linboxuse mailinglist https://groups.google.com/d/topic/linboxuse/SgsXVYM7u7s/discussion
comment:87 Changed 7 years ago by
FYI: I did 4000 doctest runs and this happened 9 times, so it is quite rare.
Because of the rarity, it doesn't have to be a sage5.4 blocker, but of course I'd like to see it fixed.
comment:88 Changed 7 years ago by
With previous LinBox versions, this file sometimes (equally rarely) timeouts in doctests. So maybe it's the same issue, but with a timeout before, and an error now.
comment:89 Changed 7 years ago by
Unfortunately, no response from upstream concerning the "running out of primes" error.
What's the title supposed to tell us (if you get build errors with that version)?
For GCC 4.7.x / C++11 issues see #12762; for (problems with) updating LinBox see #11718. (I think I've read about complications when trying to upgrade to 1.2.2 elsewhere as well.) See #12865 for yet another issue, which is still present in 1.2.2.