-
Notifications
You must be signed in to change notification settings - Fork 556
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
[jlqml] Bump version to v0.3 #4264
Conversation
I'm getting Log: |
What version of Julia and BinaryBuilderBase.jl are you using locally? Yesterday we upgraded to Julia 1.7 here: #4261. I'm clueless about Pkg internals, which break all the time |
Julia 1.7.1 also, it didn't work with Julia 1.6 (some error about Pkg.Registry not having a certain function). I'll try with Julia 1.6 and an older BinaryBuilder. Edit: BinaryBuilder version as in the Manifest.toml on Yggdrasil CI |
This recipe works with |
I think hacking the
On FreeBSD it's different:
|
That was likely a race condition |
uuid = Base.UUID("a83860b7-747b-57cf-bf1f-3e79990d037f") | ||
delete!(Pkg.Types.get_last_stdlibs(v"1.6.3"), uuid) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't this actually break everything when we try to target Julia v1.6? I don't see LibOSXUnwind_jll
in https://dev.azure.com/JuliaPackaging/Yggdrasil/_build/results?buildId=16459&view=logs&jobId=3e3b6bc7-f97f-5652-f6ca-c52d13eda498&j=a151bd49-d959-5dd9-e79c-4da26ad24f9f&t=fa03432c-08c9-5f99-931b-dc1b3c71632b, which looks wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I understand, LibOSXUnwind_jll
is a dependency of libjulia_jll
and removing it from the stdlibs
makes Pkg choose the one in the registry. It doesn't appear to cause harm, at least on Linux I get a usable JLL with binaries for Julia 1.6, 1.7 and 1.8: https://github.com/barche/jlqml_jll.jl
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still don't see LibOSXUnwind_jll
in https://dev.azure.com/JuliaPackaging/Yggdrasil/_build/results?buildId=16556&view=logs&j=11cc9737-c3b2-5b2e-49c1-3ecd4dd3473d&t=6864626c-b206-5daa-5d51-831805962223. @fingolfin maybe it wasn't necessary after all? If so, I think the best thing to do would be to have a libjulia_jll
for v1.6 which doesn't depend at runtime on LibOSXUnwind_jll
, fix this in the registry so libjulia_jll
never tries to pull in LibOSXUnwind_jll
. That'd solve all of our problems without hacks like this.
Should we keep this hack (I hope we don't), at very least please add a comment, I'm sure someone will be very confused by reading these lines without context in a few months.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That seems like the way to go, but how is this dependency triggered, exactly? I don't see a LibOSXUnwind_jll
in the Project.toml
of libjulia_jll v1.7.0+8
? Is it about removing these:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only in v1.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was never a question about linking to libOSXUnwind.dylib
. The only concern is compiling against julia.h -- some of the headers include libunwind.h
. If one includes one of these headers (possibly indirectly) this can result in a compile time error. If it doesn't, then that means it is picking up a stray libunwind.h from somewhere else, which could be bad -- though I guess my patch shows that it doesn't matter in practice....
Anyway, if this builds fine with your hack, then that's great. But I don't like that now all packages linking against libjulia_jll need such a hack. I think it's much cleaner and easier if we update libjulia_jll, dropping the dep on LibOSXUnwind_jll
in there. That's what #4320 is about (though it may indeed be possible to simplify the patch, and instead e.g. do touch $includedir/libunwind.h
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, so I guess the possibility of corrupt binaries is excluded, at worst we should get a linking error. I have added a comment, I'd propose to merge this and then investigate JuliaLang/Pkg.jl#2942 further, libjulia_jll also doesn't build because of that right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, let's test this, if it works, this is a good short term solution that we can use for the time being also elsewhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, from JuliaLang/Pkg.jl#2963 I also conclude that this hack will in this case actually have the same effect as the (possible) fix. LibOSXUnwind_jll
is here only encountered in the historical dependencies and then triggers a bug in the dependency resolution. So, @giordano this is now good to merge I think :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's do it
No idea why CI fails here, this version works locally for all three platforms. |
I can't find any way to get rid of this error:
Tried on two different machines, locally everything works with the exact same version of BinaryBuilder. |
☝️ |
Yes, I saw that, but I reran it several times since then, not sure what changed now? |
I deleted |
I squashed the commits and activated all platforms for which the dependencies are available, so for me this is good to merge. |
Needed to make QML.jl work on Julia 1.7