-
Notifications
You must be signed in to change notification settings - Fork 549
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
Migrate libcurl/libgit2/libssh2 to OpenSSL #8377
Conversation
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.
We aren't going to touch this old version of libcurl, I think we can exclude it from this.
For libgit2 and libssh2 we need Yggdrasil/L/libssh/build_tarballs.jl Lines 15 to 18 in fa8242b
|
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.
This PR is dealing with building the packages which directly depend on mbedtls, which is great! The good news is that they're all stdlibs, so we'll just need to update all of them together in Julia and that's it.
However there are a bunch of packages which link to mbedtls just because it's an indirect dependency through other packages, like libcurl or libgit2: https://github.com/search?q=repo%3AJuliaPackaging%2FYggdrasil%20mbedtls_jll&type=code. Before merging this PR we need to figure out what to do with them. In an ideal world we wouldn't need to link them to mbedtls/openssl at all if that's only an indirect dependency, but the build systems of those packages didn't seem to agree.
Done now:
Open questions:
|
Edit: it's finding it but not linking to it, so that's fine. I'd like to confirm by downloading one set of artefacts and trying them out, but I don't know how. For LibGit2:
OK we'll need to force mbedTLS off, because it's picking it up from LibSSH2 does not seem to pick up mbedTLS, however. |
OK I'm trying something: removing all the independent
They shouldn't be necessary, and maybe the issue that made them required does not occur with OpenSSL. |
Joy.
Updating to 4.6.1 and switching source URL. |
State of the build:
Those might mean that NetCDF should be rebuilt to be based on the new CURL? If so, are there other packages depending on
and second one is:
Both might be fixed with a htslib rebuild with the new LibCURL. |
We can do that with the Update Manifest GitHub action (workflow file), but that's the environment for starting up BinaryBuilder, it doesn't affect what's present inside the build environment, so that's not a concern for this PR. |
OK, based on my understand above, I'm adding triggers for three new builds:
|
Unfortunately I'm not sure just removing % readelf -d lib/libaria2.so
Dynamic section at offset 0x51f5000 contains 34 entries:
Tag Type Name/Value
0x000000000000001d (RUNPATH) Library runpath: [$ORIGIN:$ORIGIN/../lib64]
0x0000000000000001 (NEEDED) Shared library: [libz.so.1]
0x0000000000000001 (NEEDED) Shared library: [libxml2.so.2]
0x0000000000000001 (NEEDED) Shared library: [libssl.so.3]
0x0000000000000001 (NEEDED) Shared library: [libcrypto.so.3]
0x0000000000000001 (NEEDED) Shared library: [libssh2.so.1]
0x0000000000000001 (NEEDED) Shared library: [libmbedcrypto.so.7]
0x0000000000000001 (NEEDED) Shared library: [libcares.so.2]
0x0000000000000001 (NEEDED) Shared library: [libstdc++.so.6]
0x0000000000000001 (NEEDED) Shared library: [libm.so.6]
0x0000000000000001 (NEEDED) Shared library: [libc.so.6]
0x0000000000000001 (NEEDED) Shared library: [libgcc_s.so.1]
0x000000000000000e (SONAME) Library soname: [libaria2.so.0] It's still linking to mbedtls 😞 |
That's probably because libssh2 forces that: https://buildkite.com/julialang/yggdrasil/builds/9225#018e89b6-012c-4518-bcb5-0207dc268aed/657-2813
Sigh. |
Hum, I definitely don't understand how the builder works. In the latest run, it is rebuilding LibCURL, but it built LibGit2 before LibSSH2, even though that's a dependency. Therefore, the newly built LibGit2 links to mbedtls through the old LibSSH2. Does it actually ignore dependencies altogether? Or have I not figured out how to tell it correctly? Technically, I could open one PR per package, in the right order, and migrate stuff. But that's horrible from a testing point of view, because we might have an issue and find out only at the end. |
Ok, I think we need to do something like Yggdrasil/A/Aria2/build_tarballs.jl Lines 18 to 24 in 9b72607
|
They won't link to mbedtls if the new version of libssh2 is picked. In fact, they might need to link to openssl, actually. I just can't figure out how to make the builder aware of the correct dependency chain. Help on that would be appreciated. |
Sure, but it'd be good to solve the issue without linking to the new builds of libcurl/libgit2/libssh2, so that these packages will be compatible with all versions of Julia, not just the newer ones (since libcurl/libgit2/libssh2 are stdlibs, and so bound to a specific version of Julia).
Yeah, that's not possible at the moment, we use information in the General registry, which is where new version are recorded after a PR is merged here. |
547b11a
to
ad52ceb
Compare
I moved to #8386 dropping the mbedtls dependency on some packages, so as to keep here only switching to OpenSSL for libcurl/libgit2/libssh2. |
Yeah that won't work. The build artefacts for libgit2 still have:
because of the libssh2 dependency on a previous version. I suppose they'll need to be done one after another. |
Also, disable linking to mbedTLS Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
This should do the trick: Yggdrasil/L/LibGit2/build_tarballs.jl Lines 26 to 31 in ab29645
|
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.
With #8386 merged, my concerns are addressed. Thank you so much for your work!
Thinking about JuliaLang/julia#48799 and trying to see how to migrate things in here first.