Skip to content

Conversation

@Joel-Dahne
Copy link
Contributor

This bumps the GMP version to 6.2.1 since this is now the minimum supported version by FLINT. It also bumps the MPFR version to 4.2.0 since the previous one was not compatible with GMP 6.2.1.

It also uses the new repository at https://github.com/flintlib/flint.git rather than the old one at https://github.com/flintlib/flint2.git.

@thofma, @benlorenz and @fingolfin I know you have been involved in updating this package before. Do you have any comments on this update?

@benlorenz
Copy link
Contributor

Bumping the MPFR version to 4.2.0 is problematic as I think this will make it incompatible with all julia versions before 1.10. GMP 6.2.1 is available since 1.6.7, so that should be fine.

What kind of incompatibility is there between mpfr 4.1.1 and gmp 6.2.1? The most recent MPFR_jll 4.1.1 was built with that GMP version: https://github.com/JuliaPackaging/Yggdrasil/pull/5904/files#diff-f62dea2b4cf76ca6d7ed4b810e53476d7cfd237f5fd9cc221a9c7c251f968335L44

The x86-64 apple build complains about the use of aligned_alloc:

[21:03:14] src/generic_files/memory_manager.c:153:12: error: call to undeclared library function 'aligned_alloc' with type 'void *(unsigned long, unsigned long)'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
[21:03:14]     return aligned_alloc(alignment, size);
[21:03:14]            ^
[21:03:14] src/generic_files/memory_manager.c:153:12: note: include the header <stdlib.h> or explicitly provide a declaration for 'aligned_alloc'

Not sure if aligned_alloc is even available on the macos version (10.12) we are targeting by default for x86_64.

@Joel-Dahne
Copy link
Contributor Author

I was under the impression that the GMP and MPFR versions for the jll packages were not directly tied to the version Julia uses? But I could definitely be wrong on this. I tried to run the tests for Arblib.jl with the updated FLINT_jll under Julia 1.6 and it seems to work fine. I don't know if things could be broken in some unexpected way though?

Looking at the history of MPFR_jll I agree that it seems like 4.1.1 should be built with GMP 6.2.1. But if I change the MPFR version to 4.1.1 in the build script I get the following error

ERROR: LoadError: Unsatisfiable requirements detected for package GMP_jll [781609d7]:
 GMP_jll [781609d7] log:
 ├─possible versions are: 6.1.2-6.3.0 or uninstalled
 ├─restricted to versions 6.2.1 by an explicit requirement, leaving only versions: 6.2.1
 └─restricted by compatibility requirements with MPFR_jll [3a97d323] to versions: 6.2.0 — no versions left
   └─MPFR_jll [3a97d323] log:
     ├─possible versions are: 4.0.2-4.2.0 or uninstalled
     └─restricted to versions 4.1.1 by an explicit requirement, leaving only versions: 4.1.1

I'm not sure why this is the case

@fingolfin
Copy link
Member

You have to carefully distinguish between compiletime and later runtime: when using a JLL, then GMP will already be loaded, because Julia loaded it; so we end up using the same GMP as Julia (or else we go bust). So we must be compatible with that.

The way binary interfaces work, that means the JLL must be built and linked against a version of GMP which is not newer than the version used by Julia -- though it can be older (I am simplifying a bit, it can't be "too old" of course, but that's not relevant for the versions we are talking about).

The same rule holds for MPFR.

So in general the safest thing is to build against the oldest possible version of both GMP and MPFR, then things are compatible with all the newer versions, too.


Regarding the "Unsatisfiable requirements" error, we can help debug that, but the first thing I'd suggest to try is to just leave the GMP and MPFR dependencies exactly as they were -- unless you have an explicit reason why they should be changed?

If that also gives an error, we can look deeper.

@Joel-Dahne
Copy link
Contributor Author

Since this commit Flint has GMP 6.2.1 as minimal dependency. That commit also sets the minimal MPFR version to 4.2.0, but that was later downgraded to 4.1.0 in this commit.

The GMP requirement has been moved around a bit in the code but is still 6.2.1, see this line

@benlorenz
Copy link
Contributor

I do think that the compat entry in the registry is wrong:
https://github.com/JuliaRegistries/General/blob/75e504a1d681b6b79a681fc4c712e8c936ea26f5/jll/M/MPFR_jll/Compat.toml#L13

There is only one compat entry for MPFR 4.1.1 which restricts GMP to exactly 6.2.0. I think this might have been caused by some old BinaryBuilder version setting an overly restrictive compat entry when passing a specific build version?
Versions 4.1.0 and 4.2.0 don't restrict the GMP version at all.

I think we should either remove that line or change it to 6.2.0-6 ?

@Joel-Dahne
Copy link
Contributor Author

Joel-Dahne commented Mar 6, 2024

Regarding the x86_64-apple build I realize I never tried to build that one myself. The Flint repository build for that system seems to work though, see for example the build log here.

For that build the configuration check checking if system can use FLINT's assembly... is answered by yes, whereas for our build it is answered by no. I'm not sure why we would get that difference though.

@albinahlback
Copy link

albinahlback commented Mar 7, 2024

We do require C11, but it does seem like I have to add a check for aligned_alloc/_aligned_malloc. If neither is available during configuration, we should just push the replacement function based on malloc.

I will push a fix for this in a new version 3.1.1. Thanks for making me aware of this!

@albinahlback
Copy link

@Joel-Dahne Can you try with the newest release v3.1.1?

@Joel-Dahne
Copy link
Contributor Author

Seemed to work fine when building locally for apple, lets see if all the other cases as happy as well!

@Joel-Dahne
Copy link
Contributor Author

The failures are due to some instances using a cached version of the Flint repository that doesn't contain the new commit. I'm not sure why they are not fetching the latest version of the repository. It is possibly related to that the new commit is not on any branch.

It seems like everything should work as long as we get them to not used the cached version thought! Thank you for the quick patch Albin!

@Joel-Dahne
Copy link
Contributor Author

The builders seem to all be happy now! That leaves the issue with the GMP compatibility for MPFR. Looking at the history of Project.toml in MPFR_jll we see that the 6.2.0 requirement was introduced in 4.1.1+0 and removed in 4.1.1+2. The compat in the General repository does however not reflect this, but keeps the same compat for all 4.1.1 versions.

I noticed that Flint only depends on MPFR 4.1.0, and this version of MPFR_jll doesn't have the 6.2.0 compat. I tried building against 4.1.0 locally for linux and that seemed to work. However the aarch64-apple-darwin build fails, which I guess means that 4.1.0 is not compatible with that system.

The only solution that I see is to get the compat for 4.1.1 updated to not require GMP 6.2.0. But I don't know how one would proceed with that. Do any of you have any idea?

@fingolfin
Copy link
Member

Yes this requires making a pull request to the registry, with an explanation of what's going on https://github.com/JuliaRegistries/General

This bumps the GMP version to 6.2.1 since this is now the minimum
supported version by FLINT. It also bumps the MPFR version to 4.2.0
since the previous one was not compatible with GMP 6.2.1.

It also uses the new repository at
https://github.com/flintlib/flint.git rather than the old one at
https://github.com/flintlib/flint2.git.
@Joel-Dahne
Copy link
Contributor Author

It works locally for me now, thank you @benlorenz! I think the buildbots are failing because they didn't get the absolutely latest version of the registry. Hopefully it should be fine if one restarts them in a little while.

@Joel-Dahne
Copy link
Contributor Author

Everything now builds successfully! I did however notice a warning from the audit stage. For the x86_64-linux-gnu build it gives the warning

┌ Warning: Minimum instruction set detected for lib/libflint.so.19.0.0 is avx2, not x86_64 as desired.
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/Auditor.jl:325

I'm not sure if this could potentially be an issue.

@Joel-Dahne
Copy link
Contributor Author

The warning seems to be present also for the 3.0.0 release, so is presumably not an issue.

As far as I can tell this is then ready for merging!

@thofma
Copy link
Contributor

thofma commented Mar 11, 2024

I don't think we should require avx2 instructions.

@albinahlback
Copy link

Everything now builds successfully! I did however notice a warning from the audit stage. For the x86_64-linux-gnu build it gives the warning

┌ Warning: Minimum instruction set detected for lib/libflint.so.19.0.0 is avx2, not x86_64 as desired.
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/L8tBh/src/Auditor.jl:325

I'm not sure if this could potentially be an issue.

Unless I have done something wrong in the build system of FLINT, it seems to be a false positive. For instance, it checks for AVX2, but cannot find it, and so the module fft_small is never used.

@benlorenz
Copy link
Contributor

Unless I have done something wrong in the build system of FLINT, it seems to be a false positive.

I agree, older builds, even for FLINT 2.9, contain the same warning and we have never seen any issues with it:

┌ Warning: Minimum instruction set detected for lib/libflint.so.17.0.0 is avx, not x86_64 as desired.
└ @ BinaryBuilder.Auditor /cache/julia-buildkite-plugin/depots/e2fd9734-29d8-45cd-b0eb-59f7104f3131/packages/BinaryBuilder/MoPsh/src/Auditor.jl:323

from: https://buildkite.com/julialang/yggdrasil/builds/7832#018d1def-8625-4083-a52f-edf1bc363adb/629-4831

@giordano
Copy link
Member

giordano commented Mar 11, 2024

Unfortunately the ISA check isn't bullet-proof, for example JuliaPackaging/BinaryBuilder.jl#833

@albinahlback
Copy link

Unfortunately the ISA check isn't bullet-proof, for example JuliaPackaging/BinaryBuilder.jl#833

But AVX(2) opcodes are pretty distinct from base instructions, no? Even AVX should not be used for this build.

@benlorenz
Copy link
Contributor

The disassembly seems to contain quite a few tzcnt instructions which are from BMI1 (Haswell and newer), but this instruction seems to be backwards compatible in the sense that older processors will ignore the prefix and use the much older bsf instead.

@giordano giordano merged commit 6b5d555 into JuliaPackaging:master Mar 11, 2024
@Joel-Dahne Joel-Dahne deleted the FLINT-3.1.0 branch March 12, 2024 10:44
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.

6 participants