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.

Sign up for GitHub

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

Jump to bottom

GMP 6.2.0 #1159

Merged
merged 1 commit into from
Jun 16, 2020
Merged

GMP 6.2.0 #1159

merged 1 commit into from
Jun 16, 2020

Conversation

fingolfin
Copy link
Member

@fingolfin fingolfin commented Jun 16, 2020 •
edited
Loading

Also remove bits of the build script that are no longer relevant

Caveat: Julia itself still uses 6.1.2, so most likely this shouldn't be merged before a similar PR for Julia is ready.

UPDATE: this is the Julia PR: JuliaLang/julia#36309

@fingolfin
GMP 6.2.0
683b1c4 
Also remove bits of the build script that are no longer relevant
fingolfin
fingolfin commented Jun 16, 2020
View reviewed changes
G/GMP/build_tarballs.jl
DirectorySource("./bundled"),
]

# Bash recipe for building across all platforms
script = raw"""
cd $WORKSPACE/srcdir/gmp-*

# Update config.status
update_configure_scripts
Copy link
Member Author

Choose a reason for hiding this comment

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

Despite the comment, I think this actually updates config.guess and config.sub ; both are fairly recent in GMP 6.2.0, so this shouldn't be necessary anymore

@giordano giordano marked this pull request as draft June 16, 2020 13:01
fingolfin
fingolfin commented Jun 16, 2020
View reviewed changes
G/GMP/build_tarballs.jl
# Include Julia-carried patches
atomic_patch -p1 ${WORKSPACE}/srcdir/patches/gmp_alloc_overflow_func.patch
atomic_patch -p1 ${WORKSPACE}/srcdir/patches/gmp-exception.patch
Copy link
Member Author

Choose a reason for hiding this comment

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

GMP 6.2.0 already includes this change.

fingolfin
fingolfin commented Jun 16, 2020
View reviewed changes
G/GMP/build_tarballs.jl
@@ -35,10 +28,6 @@ fi

./configure --prefix=$prefix --build=${MACHTYPE} --host=${target} ${flags[@]}

# Something is broken in the libtool that gets generated on macOS; I can't
# figure out why, but `hardcode_action` is set to blank for CXX files. /shrug
sed -i -e 's&hardcode_action=$&hardcode_action=immediate&g' libtool
Copy link
Member Author

Choose a reason for hiding this comment

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

This was added by @staticfloat in d2b4c53 but once again I don't know how exactly the issues it works around manifested; GMP 6.2.0 builds fine without it. But perhaps the issue was not about a build issue but rather about an issue with using the resulting libraries?

Copy link
Member

Choose a reason for hiding this comment

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

I believe this was an issue while building, so if it builds without doing this, great!

@fingolfin fingolfin mentioned this pull request Jun 16, 2020
Merged
@fingolfin fingolfin changed the title GMP 6.2.0 (DO NOT MERGE) GMP 6.2.0 Jun 16, 2020
@fingolfin fingolfin marked this pull request as ready for review June 16, 2020 13:37
@fingolfin
Copy link
Member Author

Everything seems to work fine. Until this PR here is merged, though, JuliaLang/julia#36309 will fail. I am not sure what the best way to go forward is (that's something the Julia team will have to decide, I guess). But from my side, this PR ought to be complete...

@giordano
Copy link
Member

We need to merge first this here, and then you'll need to generate the hashes of GMP in the Julia pull request with contrib/refresh_bb_tarballs.sh and delete the old ones.

Since this is a dependency of Julia, I'd like a review from @staticfloat

staticfloat
staticfloat approved these changes Jun 16, 2020
View reviewed changes
G/GMP/build_tarballs.jl
@@ -35,10 +28,6 @@ fi

./configure --prefix=$prefix --build=${MACHTYPE} --host=${target} ${flags[@]}

# Something is broken in the libtool that gets generated on macOS; I can't
# figure out why, but `hardcode_action` is set to blank for CXX files. /shrug
sed -i -e 's&hardcode_action=$&hardcode_action=immediate&g' libtool
Copy link
Member

Choose a reason for hiding this comment

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

I believe this was an issue while building, so if it builds without doing this, great!

@giordano giordano merged commit 415ead0 into JuliaPackaging:master Jun 16, 2020
@fingolfin fingolfin deleted the mh/gmp-6.2.0 branch June 16, 2020 16:52
@giordano
Copy link
Member

This breaks all libraries depending on GMP on macOS: https://github.com/Wikunia/ConstraintSolver.jl/pull/169/checks?check_run_id=779855667#step:5:154

ERROR: LoadError: LoadError: InitError: could not load library "/Users/runner/.julia/artifacts/3677b4b693751024a0923d9e7c6d56da89f15b8e/lib/libgmpxx.4.dylib"
dlopen(/Users/runner/.julia/artifacts/3677b4b693751024a0923d9e7c6d56da89f15b8e/lib/libgmpxx.4.dylib, 1): Library not loaded: @rpath/libgmp.10.dylib
  Referenced from: /Users/runner/.julia/artifacts/3677b4b693751024a0923d9e7c6d56da89f15b8e/lib/libgmpxx.4.dylib
  Reason: Incompatible library version: libgmpxx.4.dylib requires version 15.0.0 or later, but libgmp.dylib provides version 14.0.0

@fingolfin
Copy link
Member Author

I'd need some more time to analyze this properly. My first guess: libgmp.dylib is already loaded by Julia, so instead of loading the fresh (but binary incompatible, due to increased library version) copy of libgmp.dylib provied by GMP_jll, it runs into this error.

If that's the cause, then I'd expect the test to work when using Julia nightly / master branch. If somebody can confirm this, that'd be helpful (I can't right now, but could try to find time for it tomorrow).

If so, then a solution might be to change GMP_jll 6.2.0 to require Julia >= 1.6; then everybody using Julia <= 1.5 will get GMP_jll 6.1.2 (which in turn then should perhaps get a dependency on Julia 1.0-1.5 or so). Probably easiest to achieve by keeping both versions of the GMP builder in Yggdrasil for the time being.

Being able to specify a constraint on the Julia version is something I'd like to have anyway (see the discussion in JuliaPackaging/BinaryBuilder.jl#511 with @staticfloat about this). I started work on such a feature back then but it got stalled.

Of course all of this is moot if my guess about the underlying issue and how it can be resolved is wrong, so the first step should be to test if e.g. ConstraintSolver.jl tests pass when using a Julia nightly which already contains the GMP 6.2.0 patch.

@giordano
Copy link
Member

Yes, the problem is with libraries loaded by default by Julia. Do you mind opening a PR for the registry to require v1.6 of Julia for GMP_jll v6.2?

@fingolfin
Copy link
Member Author

@giordano I don't mind but I am not sure what I'd have to change where? I guess I need to edit a file in https://github.com/JuliaRegistries/General/tree/master/G/GMP_jll?

@giordano
Copy link
Member

The file is https://github.com/JuliaRegistries/General/blob/master/G/GMP_jll/Compat.toml. We want v6.1.2 to be compatible with julia 1-1.5, 6.2 with 1.6 upwards

fingolfin added a commit to fingolfin/General that referenced this pull request Jun 17, 2020
@fingolfin
Limit GMP 6.2 to Julia 1.6
2ce55ad 
See JuliaPackaging/Yggdrasil#1159
@fingolfin fingolfin mentioned this pull request Jun 17, 2020
Merged
@fingolfin
Copy link
Member Author

Done at JuliaRegistries/General#16517

fingolfin added a commit to fingolfin/General that referenced this pull request Jun 18, 2020
@fingolfin
Limit GMP 6.2 to Julia 1.6
72ff10e 
See JuliaPackaging/Yggdrasil#1159
@fingolfin fingolfin mentioned this pull request Jun 18, 2020
Merged
fredrikekre pushed a commit to JuliaRegistries/General that referenced this pull request Jun 18, 2020
@fingolfin
Limit GMP 6.2 to Julia 1.6 (#16517)
a784413 
See JuliaPackaging/Yggdrasil#1159
@MikaelSlevinsky MikaelSlevinsky mentioned this pull request Jun 18, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@giordano giordano giordano left review comments

@staticfloat staticfloat staticfloat approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fingolfin @giordano @staticfloat