-
-
Notifications
You must be signed in to change notification settings - Fork 477
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support linbox 1.7.0 and 1.6.3 at the same time #35612
Conversation
This looks like a good solution, and I agree that it would be important to merge it in 10.0 to make packaging easier for downstream |
Documentation preview for this PR is ready! 🎉 |
@ClementPernet could you please have a look at this and comment? The change here is very minimal, should be very easy to review, and is the way forward to updating linbox in distros and in sage. The idea is to replace the (single) use of an API that changed between 1.6.3 to 1.7.0 with a different one that works for both 1.6.3 and 1.7.0, thus making sagemath source-compatible with both versions. |
@dimpase @mkoeppe @antonio-rojas ping, it'd be really nice to get this merged for 10.0. I think it's pretty safe to push this change without actually updating linbox et al. That ensures sagemath works out of the box with linbox 1.7.0 and will mean it gets used and tested more so the actual update can happen in the next cycle. BTW, I've been testing 10.0.rc1 with this patch and linbox 1.7.0 on x86-64 (glibc / musl) and on i686 (glibc). I'll update everything in void linux once 10.0 is out. See: void-linux/void-packages#43659. |
Conda CI fails for unrelated reasons - conflict of OpenSSL versions... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
The constructor `DenseMatrix(field, entries, m, n)` in 1.6.3 was changed to `DenseMatrix(field, m, n, entries)` in 1.7.0. We replace use of this constructor by a different way to initialize a matrix from entries using part of the API that works both in 1.6.3 and in 1.7.0. Note that the two versions of the library are still ABI-incompatible so changing linbox requires recompiling sagelib. To complicate matters, linbox doesn't change soname so the dynamic linker will let this go and sagemath will eventually crash.
46ec6e1
to
9c8796c
Compare
Rebased to 10.0.rc2 without any change (removes spurious "fix the linter commits") |
📚 Description
The constructor
DenseMatrix(field, entries, m, n)
in 1.6.3 was changed toDenseMatrix(field, m, n, entries)
in 1.7.0.We replace use of this constructor by a different way to initialize a matrix from entries using part of the API that works both in 1.6.3 and in 1.7.0.
NOTE: the two versions of the library are ABI-incompatible so changing linbox requires recompiling sagelib. To complicate matters, linbox doesn't change soname so the dynamic linker will let this go and sagemath will eventually crash.
See: #35148 for the actual update of linbox -- this PR should make that one easier as well as system support of linbox, since both linbox 1.6.3 and 1.7.0 wil be supported.
📝 Checklist