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

Check when we build a library, it functions correctly (in particular gmp) #5720

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

ChrisJefferson
Copy link
Contributor

Run make check whenever we build zlib or gmp as an external dependancy.

This will, hopefully, catch if we end up with an out-of-date gmp. This could probably be improved to explain to users why compiling is braking, and suggest they go and suggest they install the approriate library themselves?

Check it works by not installing gmp on mac.

@fingolfin
Copy link
Member

So this is an attempt at "future proofing" us, against a situation where someone tries to install a GAP version with an outdated bundled GMP (as was the case for a time with GAP 4.12.x and older and Apple M1, IIRC, though currently isn't the case with GAP 4.13.0).

But what I have seen much more often recently are problems with people were the system GMP was broken or outdated. So perhaps we should also add a check of the form "if on an ARM mac and the system/homebrew/user specified GMP is older than X, refuse to use it?

@ChrisJefferson
Copy link
Contributor Author

I wonder if (to make life easier in the future) we should just tighten to "require you have at least whatever GMP version we currently distribute" everywhere, while GAP usually works with an older version, it does seem to be a source of occasional bugs (of course that might annoy people unnessarily?)

@ChrisJefferson
Copy link
Contributor Author

The 'old gmp' could just be a warning we print on startup?

@ChrisJefferson
Copy link
Contributor Author

I was wondering if we should do the check for an out-of-date gap at build time or run time? I've made a quick mock where I get the version of our bundled GMP and compare against it at runtime.

Advantages of run time -- someone might build a binary and copy it to a machine with an older GAP, we would miss this if we did it at build time.

Disadvantage of run time -- we need to pass the version of GMP we expect to GAP while building, probably -DGMP_BUNDLED_VERSION="...", while we've been trying to reduce the number of compile-time options we need to set.

Copy link
Member

@fingolfin fingolfin left a comment

Choose a reason for hiding this comment

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

So what this PR solves is the case were we ship the most recent version of GMP when releasing GAP, but then some new OS / compiler / hardware is released with which that GMP release does not work quite right (which happened with GAP 4.12.2 and the release of the M1 macs) -- and a user tries to compile GAP from source without having a working GMP installed -- then we may now catch that by failing during building / checking GMP.

What this does not catch are cases were GAP is built against a user supplied GMP that is broken (as we the case with at least one reason reported issue)

Which is still not bad overall, of course, but perhaps we can do even more some day.

@fingolfin fingolfin merged commit dcf704f into gap-system:master Jun 9, 2024
25 checks passed
@fingolfin fingolfin added topic: build system release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes labels Jun 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes topic: build system
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants