Skip to content
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

Update fplll (and lots of other programs/libraries) #3683

Open
wants to merge 21 commits into
base: development
Choose a base branch
from

Conversation

d-torrance
Copy link
Member

I'd never noticed this before, but we have the option of linking against fplll to use for one of the LLL strategies in the LLLBases package. However, we were forced to build it ourselves rather than using the library already available on the system, and the version we were building was over 10 years old!

We update the autotools build so that it also checks the system for the library, and so that when we do build it, we build the latest version.

This is a draft for now to make sure that the tests still pass when building fplll ourselves. If that all works, then I'll remove that commit and just use the Ubuntu/brew fplll packages.

@d-torrance
Copy link
Member Author

The tests all passed building fplll (see https://github.com/Macaulay2/M2/actions/runs/13697326897). I'll remove the last commit and make sure that the tests pass when using the Ubuntu/brew packages.

@mikestillman
Copy link
Member

@d-torrance Do we have any other libraries/packages that we sometimes build that are out of date?

@d-torrance
Copy link
Member Author

Quite possibly! I discovered not too long ago that when we built GMP it was like a 10 year old version. It would definitely be good to go through everything and check.

@d-torrance
Copy link
Member Author

d-torrance commented Mar 6, 2025

I audited what we're building (at least on the autotools side of things). These are the things that are out of date:

library M2 version latest verson notes handled in this PR
cdd+ 77a 77 are we using this?
factory 4.4.0 4.4.1 4.4.0 seems to be the highest version with its own tarball outside of singular ❌ (#3698)
flint 3.0.0 3.1.3 we're currently using our fork w/ some cmake patches -- is this still necessary? ❌ (#3698)
gc 8.2.6 8.2.8
gdbm 1.9.1 1.24
gfan 0.6.2 0.7 new 128-bit integer support is a pain to get working on macOS -- will wait for another PR
gftables 4.0.1 4.4.1 why isn't this just included w/ factory? ❌ (#3698)
glpk 4.5.9 5.0
gtest 1.11.0 1.16.0
lapack 3.9.0 3.12.1
libtool 2.4.2 2.5.4 remove? this should be available already, plus I think I removed the Makefile that built it
linbox 1.7.0 1.7.0 remove? we aren't using this
lrslib 071a 073
msolve 0.7.3 0.7.5
nauty 2.8.8 2.8.9
pari 2.11.2 2.17.1 remove -- we stopped using this
polymake 2.9.8 4.12 Polymake package should be updated to handle latest versions
tbb 2021.13.0 2022.0.0
topcom 0.17.8 1.1.2

@d-torrance d-torrance force-pushed the fplll branch 3 times, most recently from e6ccd91 to b72e095 Compare March 7, 2025 16:56
@d-torrance d-torrance changed the title Update fplll Update fplll (and lots of other programs/libraries) Mar 7, 2025
@d-torrance
Copy link
Member Author

@mahrud: I couldn't figure out how to force cmake to build nauty using -DBUILD_LIBRARIES or -DBUILD_PROGRAMS. Is it possible?

@d-torrance d-torrance force-pushed the fplll branch 4 times, most recently from 896ba36 to f09a9d5 Compare March 8, 2025 02:34
@d-torrance
Copy link
Member Author

All the tests are finally passing when we build all these libraries and programs! https://github.com/Macaulay2/M2/actions/runs/13733104922

I'll remove the final commit now and go back to Ubuntu/Homebrew packages

@d-torrance d-torrance marked this pull request as ready for review March 8, 2025 12:43
@@ -180,7 +180,7 @@ ExternalProject_Add(build-bdwgc
CMAKE_ARGS -DCMAKE_INSTALL_PREFIX=${M2_HOST_PREFIX}
-DCMAKE_SYSTEM_PREFIX_PATH=${M2_HOST_PREFIX}
-DCMAKE_BUILD_TYPE=${CMAKE_BUILD_TYPE}
-DBUILD_SHARED_LIBS=${BUILD_SHARED_LIBS}
-DBUILD_SHARED_LIBS=ON # for ForeignFunctions package
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this forced on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ForeignFunctions has a getMemory function for allocating memory, which is very useful when your foreign function has a pointer as one of its arguments. Under the hood, it calls either GC_malloc or GC_malloc_atomic. Currently, this is implemented as a foreign function at top level, which means that we need libgc to be dynamically linked so we can use dlsym.

It could be implemented differently to allow for static linking by moving some of that code to ffi.d. Would that be preferable?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think so? The ability to have a completely static build is always good (though there are currently a few obstacles using the CMake build that I don't understand).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated!

Something is weird when building bdwgc and linking statically in the cmake build on my system, though. M2 is sluggish, we eventually get lots of GC finalization warnings, and the ForeignFunctions tests just segfault. It's not a problem with the autotools build -- I'm not sure what's different.

Copy link
Member Author

@d-torrance d-torrance Mar 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, I checked and I have the same problems statically linking bdwgc in the cmake build on the development branch, so it's not an issue with the newer version of bdwgc or anything.

@@ -178,7 +178,6 @@ endif()
###############################################################################
## TODO: Do we still want these libraries?
# fplll Lattice algorithms using floating-point arithmetic (uses gmp and mpfr)
# linbox Exact computational linear algebra (needs fflas and givaro)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding was that Mike was still interested in experimenting with Linbox, even though it isn't actively in use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mikestillman - What's the status of using linbox?

Also:
  - make sure that it's always built as a shared library in
    cmake (needed for ForeignFunctions package)
  - make sure that gc util program can find gc in macOS
@mahrud
Copy link
Member

mahrud commented Mar 11, 2025

This is definitely not in the scope here, but if you're looking at LLL package anyway, would be great if you could:

  1. figure out what't the deal with the LLL strategy of (minimalPresentation, Module) which is disabled by return null?
  2. document these nodes, even if just a stub:
gramm
gramm(Matrix)
LLL(...,Limit=>...)
gcdLLL(...,Strategy=>...)
kernelLLL
kernelLLL(...,Limit=>...)
kernelLLL(Matrix)
hermite
hermite(Matrix)
hermite(...,Strategy=>...)
hermite(...,ChangeMatrix=>...)
hermite(...,Limit=>...)

@d-torrance d-torrance added waiting for review build issue platform specific issues involving compiling M2, generating examples, or running tests dependencies Pull requests that update a dependency file labels Mar 14, 2025
For some reason, autoreconf isn't overwriting the existing aclocal.m4,
so we manually remove it first.
Otherwise, the build fails when LAPACK is a static library (e.g., when
built by autotools).

Also bump ForeignFunctions to version 0.5
Also:
* Remove the requirement that we build a shared bdwgc library in
  autotools
* Update the check for GC_get_full_gc_total_time.  ifndef was the
  wrong thing to use since it's not a macro.  We were *always* declaring
  it, which wasn't a problem unless we were statically linking bdwgc.
  Then we got redefinition errors.
@d-torrance
Copy link
Member Author

Msolve 0.7.5 was just released, so I've updated this PR accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build issue platform specific issues involving compiling M2, generating examples, or running tests dependencies Pull requests that update a dependency file waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants