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

Migrate libcurl/libgit2/libssh2 to OpenSSL #8377

Merged
merged 3 commits into from
Mar 30, 2024

Conversation

fxcoudert
Copy link
Contributor

Thinking about JuliaLang/julia#48799 and trying to see how to migrate things in here first.

Copy link
Member

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.

@giordano
Copy link
Member

For libgit2 and libssh2 we need

# Necessary for cmake to find openssl on Windows
if [[ ${target} == x86_64-*-mingw* ]]; then
export OPENSSL_ROOT_DIR=${prefix}/lib64
fi
because openssl puts the 64-bit windows library under lib64/

Copy link
Member

@giordano giordano left a 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.

@fxcoudert
Copy link
Contributor Author

fxcoudert commented Mar 29, 2024

Done now:

  • Fixed typos
  • Removed LibCURL@7
  • Added OPENSSL_ROOT_DIR on x86_64-*-mingw*

Open questions:

  • Should I regenerate .ci/Manifest.toml? How is it done?
  • I don't understand why the indirect dependencies need to be included. Maybe @samtkaplan can comment, he authored many of these commits.

@fxcoudert
Copy link
Contributor Author

fxcoudert commented Mar 29, 2024

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:

[07:51:19] -- Found OpenSSL: /opt/i686-linux-gnu/i686-linux-gnu/sys-root/usr/local/lib/libcrypto.so (found version "3.0.12")
[07:51:19] -- Found mbedTLS:
[07:51:19] --   version 2.28.6
[07:51:19] --   TLS: /opt/i686-linux-gnu/i686-linux-gnu/sys-root/usr/local/lib/libmbedtls.so
[07:51:19] --   X509: /opt/i686-linux-gnu/i686-linux-gnu/sys-root/usr/local/lib/libmbedx509.so
[07:51:19] --   Crypto: /opt/i686-linux-gnu/i686-linux-gnu/sys-root/usr/local/lib/libmbedcrypto.so

OK we'll need to force mbedTLS off, because it's picking it up from MbedTLS_jll v2.28.6+0. I am wondering if this is installed from Julia itself, or from the .ci/Manifest.toml (which would mean I have to regenerate that file, but I don't know how).

LibSSH2 does not seem to pick up mbedTLS, however.

@fxcoudert
Copy link
Contributor Author

OK I'm trying something: removing all the independent MbedTLS_jll dependencies.

    Aria2: remove MbedTLS_jll dependency
    AzStorage: remove MbedTLS_jll dependency
    IOAPI: remove MbedTLS_jll dependency
    NetCDFF: remove MbedTLS_jll dependency
    samtools: remove MbedTLS_jll dependency
    TempestModel: remove MbedTLS_jll dependency
    VMEC: remove MbedTLS_jll dependency

They shouldn't be necessary, and maybe the issue that made them required does not occur with OpenSSL.

@fxcoudert
Copy link
Contributor Author

Joy.

ERROR: LoadError: ArgumentError: The archive automatically generated by GitHub
  https://github.com/Unidata/netcdf-fortran/archive/refs/tags/v4.6.0.tar.gz
may not have a stable checksum in the future, thus cannot be used as a reliable source, see
<https://github.blog/2023-02-21-update-on-the-future-stability-of-source-code-archives-and-hashes/>.
Use a different source, for example a `GitSource`, or an official release artifact uploaded
by the maintainers of the package (*not* the automatic archive produced by GitHub).

Updating to 4.6.1 and switching source URL.

@fxcoudert
Copy link
Contributor Author

State of the build:

  • TempestModel fails on 6/9 targets. The error is: /workspace/destdir/lib/libiomp5.so: undefined reference to 'memcpy@GLIBC_2.14' and I don't know where it comes from. There are, however, warnings before that, related to netcdf.
[10:43:21] /opt/x86_64-linux-gnu/bin/../lib/gcc/x86_64-linux-gnu/4.8.5/../../../../x86_64-linux-gnu/bin/ld: warning: libmbedtls.so.12, needed by /workspace/destdir/lib/libnetcdf.so, not found (try using -rpath or -rpath-link)
[10:43:21] /opt/x86_64-linux-gnu/bin/../lib/gcc/x86_64-linux-gnu/4.8.5/../../../../x86_64-linux-gnu/bin/ld: warning: libmbedx509.so.0, needed by /workspace/destdir/lib/libnetcdf.so, not found (try using -rpath or -rpath-link)
[10:43:21] /opt/x86_64-linux-gnu/bin/../lib/gcc/x86_64-linux-gnu/4.8.5/../../../../x86_64-linux-gnu/bin/ld: warning: libmbedcrypto.so.3, needed by /workspace/destdir/lib/libnetcdf.so, not found (try using -rpath or -rpath-link)

Those might mean that NetCDF should be rebuilt to be based on the new CURL? If so, are there other packages depending on LibCURL that we should rebuild? How do we audit that?

  • NetCDFF has a single failure on x86_64-linux-gnu-libgfortran3. It says configure: error: Could not link to netcdf C library. Please set LDFLAGS; for static builds set LIBS to the results of nc-config --libs… but why does it work on other targets?
  • IOAPI has a single failure on x86_64-linux-gnu-libgfortran4, seeming to pick up mbedtls through NetCDF (as above).
ERROR: could not load library "/cache/build/yggy-amdci7-15/julialang/yggdrasil/I/IOAPI/build/x86_64-linux-gnu-libgfortran4/qRO2jGHy/x86_64-linux-gnu-libgfortran4-cxx11/destdir/lib/libioapi.so"
libmbedtls.so.12: cannot open shared object file: No such file or directory
  • samtools has two failures, on x86_64-linux-gnu and x86_64-linux-musl. First one is:
configure: error: HTSlib development files not found

and second one is:

[10:48:07] configure: error: C compiler cannot create executables

Both might be fixed with a htslib rebuild with the new LibCURL.

@giordano
Copy link
Member

Should I regenerate .ci/Manifest.toml? How is it done?

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.

@fxcoudert
Copy link
Contributor Author

OK, based on my understand above, I'm adding triggers for three new builds:

    HDF5: new build
    htslib: new build
    NetCDF: new build

@giordano
Copy link
Member

Unfortunately I'm not sure just removing MbedTLS_jll from the deps of the extra packages is sufficient, for example I downloaded the build of Aria2 for x86_64-linux-gnu-cxx11:

% 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 😞

@giordano
Copy link
Member

That's probably because libssh2 forces that: https://buildkite.com/julialang/yggdrasil/builds/9225#018e89b6-012c-4518-bcb5-0207dc268aed/657-2813

[10:19:25] Libssh2:        yes (CFLAGS='-I/workspace/destdir/include ' LIBS='-L/workspace/destdir/lib -lssh2 -lmbedcrypto ')

Sigh.

@fxcoudert
Copy link
Contributor Author

Capture d’écran 2024-03-29 à 12 10 37

I think I know what's happening: it's not rebuilding LibCURL actually! Probably because I made no change to build_tarballs.jl? That's a guess about how the builder works, but I'm going to try to add a trigger line there.

@fxcoudert
Copy link
Contributor Author

fxcoudert commented Mar 29, 2024

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.

@giordano
Copy link
Member

Ok, I think we need to do something like

# Note: we explicitly set `LIBSSH2_LIBS` to avoid pulling in mbedtls when
# linking to older builds of libssh2.
./configure \
--prefix=${prefix} --build=${MACHTYPE} --host=${target} \
--with-pic --enable-shared --enable-libaria2 \
--with-openssl --with-libxml2 --with-libz --with-libssh2 \
LIBSSH2_LIBS="-L${libdir} -lssh2 "
to override info from pkg-config files and force packages not to link directly to mbedtls

@fxcoudert
Copy link
Contributor Author

fxcoudert commented Mar 29, 2024

to override info from pkg-config files and force packages not to link directly to mbedtls

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.

@giordano
Copy link
Member

They won't link to mbedtls if the new version of libssh2 is picked.

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).

I just can't figure out how to make the builder aware of the correct dependency chain. Help on that would be appreciated.

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.

@giordano
Copy link
Member

I moved to #8386 dropping the mbedtls dependency on some packages, so as to keep here only switching to OpenSSL for libcurl/libgit2/libssh2.

@fxcoudert
Copy link
Contributor Author

Yeah that won't work. The build artefacts for libgit2 still have:

/Users/fx/Downloads/LibGit2.v1.8.0.x86_64-apple-darwin/lib/libgit2.1.8.0.dylib:
	@rpath/libgit2.1.8.dylib (compatibility version 1.8.0, current version 1.8.0)
	@rpath/libssl.3.dylib (compatibility version 3.0.0, current version 3.0.0)
	@rpath/libcrypto.3.dylib (compatibility version 3.0.0, current version 3.0.0)
	@rpath/libssh2.1.dylib (compatibility version 1.0.0, current version 1.0.1)
	@rpath/libmbedcrypto.7.dylib (compatibility version 7.0.0, current version 2.28.6)
	/usr/lib/libiconv.2.dylib (compatibility version 7.0.0, current version 7.0.0)
	/usr/lib/libSystem.B.dylib (compatibility version 1.0.0, current version 1238.0.0)

because of the libssh2 dependency on a previous version. I suppose they'll need to be done one after another.

fxcoudert and others added 2 commits March 29, 2024 16:59
Also, disable linking to mbedTLS

Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>
@giordano
Copy link
Member

This should do the trick:

# Make sure we don't link to mbedTLS:
# <https://github.com/JuliaPackaging/Yggdrasil/pull/8377#issuecomment-2027370830>.
# TODO: this hack can be removed when we'll link to a newer version of libssh2 which
# doesn't link to mbedTLS.
-DLIBSSH2_LDFLAGS="-L${libdir};-lssh2"
-DLIBSSH2_LIBRARIES="ssh2"

@giordano giordano changed the title Try some migration to OpenSSL Migrate libcurl/libgit2/libssh2 to OpenSSL Mar 29, 2024
@giordano giordano marked this pull request as draft March 30, 2024 15:13
@giordano giordano marked this pull request as ready for review March 30, 2024 15:13
Copy link
Member

@giordano giordano left a 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!

@giordano giordano merged commit dbee0b6 into JuliaPackaging:master Mar 30, 2024
54 checks passed
@fxcoudert fxcoudert deleted the openssl branch March 31, 2024 09:22
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.

2 participants