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

Autotools build updates for RHEL 8 #3461

Merged
merged 37 commits into from
Oct 19, 2024

Conversation

d-torrance
Copy link
Member

Otherwise, it may not look in usr-host for dependencies that we've built. For example, the version of gmp in RHEL 8 is too old for flint, so we need to build it and link against that instead.

Here an example of a failed build prior to this patch (https://github.com/d-torrance/M2-workflows/actions/runs/10733556117/job/29767118034), where it was only looking at the RHEL gmp package:

checking for library containing __gmpz_init... -lgmp
configure: error: "`mpn_gcd_11' was not found in the GMP library. It is needed to enable GMP
internals."
checking for library containing __gmpn_gcd_11... no

After this patch (from a local build in an AlmaLinux 8 docker image):

checking for library containing __gmpz_init... -lgmp
checking for library containing __gmpn_gcd_11... none required

This is a followup to #3350 and #3367 in my quest to get the autotools build working with RHEL.

Skipping the GitHub builds since we use the Ubuntu/Homebrew flint packages and this patch won't be tested.

@d-torrance
Copy link
Member Author

I ended up simplifying this a bit more. The underlying issue was that flint didn't used to respect LDFLAGS. Instead, it used a variable called LIB_DIRS for a similar purpose, and we needed to change the call to configure pretty significantly to get it to work.

However, now that it does respect LDFLAGS, we can mostly use the defaults from Makefile.library with just a couple tweaks.

@d-torrance d-torrance changed the title Pass LDFLAGS to flint's configure script (autotools) Autotools build updates for RHEL 8 Sep 26, 2024
@d-torrance d-torrance marked this pull request as draft September 26, 2024 14:15
@d-torrance
Copy link
Member Author

I'm broadening the scope of this PR -- the goal is to get the autotools build to work in RHEL 8.

@d-torrance
Copy link
Member Author

d-torrance commented Sep 26, 2024

This now compiles on a Rocky Linux 8 docker image! (With the --disable-tbb configure option.)

It still fails at link time due to FFLAS-FFPACK/Givaro BLAS shenanigans.

TODO:

  • Add the ability to build TBB
  • Figure out the BLAS stuff

@d-torrance
Copy link
Member Author

d-torrance commented Sep 27, 2024

Got it working!

Macaulay2, version version-1.14.0.1-9300-gb87f02b5f4 (rhel-8)
with packages: ConwayPolynomials, Elimination, IntegralClosure, InverseSystems, Isomorphism, LLLBases, MinimalPrimes, OnlineLookup, Polyhedra,
               PrimaryDecomposition, ReesAlgebra, Saturation, TangentCone, Truncations, Varieties

i1 : version#"issue"

o1 = rocky-8.9

I think this closes #475. Rather than rolling our own BLAS check, we use AX_BLAS from the autoconf archive and also pass the result to fflas-ffpack.

Still a draft for now -- I'd like to do some more experimenting.

@d-torrance d-torrance force-pushed the flint-autotools branch 3 times, most recently from c45295d to 51b65ec Compare September 27, 2024 21:09
@d-torrance
Copy link
Member Author

Assuming the builds all pass, I think this PR is ready to go. I successfully compiled and linked M2 in Rocky Linux 8 using both the openblas-devel package from EPEL and building LAPACK ourselves. I haven't checked building the examples or running the tests yet.

An overview of the changes:

  • Replace the autotools linear algebra library checks with AX_BLAS and AX_LAPACK from the Autoconf Archive. This simplifies things significantly, and in particular, allows linking with OpenBLAS out of the box, closing #475. We also switch the OpenBLAS for the GitHub Ubuntu builds.
  • Add a "blas implementation" key/value pair to the version hash table so we can see which BLAS we're linking against.
  • Simplify the checks for givaro and fflas-ffpack.
  • Remove the deprecated givaro source files from the e directory.
  • Add a patch for fflas-ffpack so that it properly detects LAPACK when we build it ourselves. Some of these changes were already submitted upstream, but I also submitted a PR with additional changes.
  • Improve the TBB check so that it detects when we're using a TBB older than 2020 (the earliest one we support). We also build TBB if needed.
  • Fix some some bugs with the build when --without-tbb is passed.
  • Add a version check for MPFR and build it if the detected version is less than 4 (which we need for several functions).
  • Build eigen if it's not found.
  • Simplify the flint build, and particular, allow it to link against the gmp we just built.
  • Build msolve as a static library since we build flint as a static library.

@d-torrance d-torrance marked this pull request as ready for review September 27, 2024 21:35
@mahrud
Copy link
Member

mahrud commented Sep 27, 2024

Should the givaro changes be in the same PR? I think @mikestillman wanted to remove them for some time, but I don't know what was the status.

M2/Macaulay2/d/version.dd Outdated Show resolved Hide resolved
@d-torrance
Copy link
Member Author

Should the givaro changes be in the same PR?

Maybe it's outside the scope of the PR, but this was my thought process:

  • I wanted to simplify the fflas-ffpack and givaro checks since they were muddying up the build flags with a bunch of redundancies with their calls to pkg-config. In particular, I wanted to make sure that any BLAS/LAPACK flags were coming from AX_BLAS and AX_LAPACK and nothing else.
  • Part of this was investigating whether we needed to still do the isunit/isUnit check. We haven't compiled the file that called this since 1b3c31b, so I figured I'd remove it.

@mahrud
Copy link
Member

mahrud commented Sep 27, 2024

  • Part of this was investigating whether we needed to still do the isunit/isUnit check.

Maybe remove this from the cmake build as well then.

@mahrud mahrud linked an issue Sep 29, 2024 that may be closed by this pull request
@mahrud
Copy link
Member

mahrud commented Sep 29, 2024

I think I'm happy with this, but of course I haven't tested it.
@trevorkarn could you test this PR? (I'm requesting a review on github, but you don't need to formally review the changes as long as your build succeeds)

@d-torrance does this close #2507 as well?

@mahrud
Copy link
Member

mahrud commented Sep 29, 2024

Oh also, @d-torrance could you add your RHEL Dockerfile under BUILD/docker?

@trevorkarn
Copy link

I think I'm happy with this, but of course I haven't tested it. @trevorkarn could you test this PR? (I'm requesting a review on github, but you don't need to formally review the changes as long as your build succeeds)

@d-torrance does this close #2507 as well?

Sad to say the build did not succeed.

@d-torrance
Copy link
Member Author

does this close #2507 as well?

No, -lgfortran is in FCLIBS, which we still link against. I think we need it if we link against any Fortran implementation of BLAS, not just one we build, right? And I'm not sure how to detect that, especially if the user can hot-swap with flexiblas or Debian's alternatives system.

@d-torrance
Copy link
Member Author

There were several errors building the examples for ForeignFunctions. These were assuming that some of the libraries that M2 links against were linked dynamically and not statically. I pushed a few commits changing these to use things from the C standard library instead. (One of these will close #3374.)

@d-torrance
Copy link
Member Author

After the most recent commits, the build finished successfully for me!

It's not directly related to runProgram.  Add a bit more detail about
how "get" might be used (by adding a leading "!"), and also link to
the docs for the various "open*" functions that also take a string
with a leading "!" to interact with a program.

The primary reason we're dropping this is example is that it called
gfan, which may not be on the PATH, especially if we've just built it.
It will only work if we've dynamically linked against mpfr, which may
not be the case.
It will only work if mpfr is available as a dynamic library and we've
linked against it, which may not be the case.

Replace it with "cos" from the C standard math library
It's in the C standard library, so we know that it will be available.

Closes: Macaulay2#3374
M2/autogen.sh Outdated Show resolved Hide resolved
Older versions of autoconf don't install them automatically

[ci skip]
Also use a wildcard for the Makefile's in the subdirectories of
libraries for simplicity and ease of maintenance.

[ci skip]
@d-torrance
Copy link
Member Author

I'm going to go ahead and merge this -- building against this branch works for me using docker images for both Rocky Linux and AlmaLinux 8:

(They both fail during make check because the time utility is missing, but that's an easy fix...)

The builds on RHEL 9 are still failing due to #2398.

@d-torrance d-torrance merged commit 64265a4 into Macaulay2:development Oct 19, 2024
5 checks passed
@mahrud
Copy link
Member

mahrud commented Oct 19, 2024

building against this branch works for me using docker images for both Rocky Linux and AlmaLinux 8

At some point I wanted to create GitHub Actions workflows that build an image based on the contents of the BUILD/docker directory, and setup a workflow_dispatch (like the package review workflow) that could be triggered in a browser. This would make testing on different distributions way easier.

@mikestillman
Copy link
Member

We should also create docker images (for using M2, and for compiling M2) for each release. We should put the docker images in an "official" location, e.g. under macaulay2 on docker hub, although I'm not sure docker hub is as good as it used to be...

@mahrud
Copy link
Member

mahrud commented Oct 19, 2024

We have one already: https://github.com/orgs/Macaulay2/packages/container/package/testbot
Based on BUILD/testbot/Dockerfile.

@trevorkarn
Copy link

@d-torrance So sorry for my slow reply! I was able to build a working binary off of commit 4e49717e08fe3166b1115a53778089fb9b40a702 but I never successfully built the docs. Having the binary was enough for the work I wanted to do, so I didn't try, but let me know if you need me to.

@d-torrance
Copy link
Member Author

@trevorkarn -- No worries!

I'm pretty sure the docs should build now. This branch has now been merged into the development branch. If you have some time to build the development branch to confirm that the docs build, then that would be much appreciated!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

allow the use of openblas
4 participants