cargo-c support - Githubissues

lu-zero commented 1 year ago

Since @thesamesam asked here a minimal patchset

Improvements over the status-quo:

Produces correct dynamic and static libraries on pretty much all the rust tier-2 platforms.
Correctly embeds the version information in the C header
Supports cross compiling out of box
Distributions have already support for it

It does not require to change the other build systems.

cpu commented 1 year ago

@jsha I think CI might be jammed up on this branch. Could you kick it when you have a chance?

lu-zero commented 1 year ago

I freshened it again.

cpu commented 1 year ago

@jsha @ctz Do you folks have any thoughts on this PR? I've rebased on main because it seems like a worthwhile addition to me and it would be nice to get out of the PR queue one way or the other.

~I think it might be nice to add CI coverage to prevent any regression in support, but naively adding a task that runs cargo install cargo-c adds 6m runtime to CI (example).~

WDYT? ~If there's interest in merging this work should we do it without the CI task? Alternatively we could use a third party action like cargo-binstall to try and cut down on the runtime.~ (Worked out a better approach in later comments).

thesamesam commented 1 year ago

We're still relying on this in Gentoo to build a dynamic lib for Apache and curl to use, and would very much appreciate this being merged. cargo-c makes life a lot easier for stuff like this.

lu-zero commented 1 year ago

@cpu I provide binaries for CI usage, see what we do in rav1e, it shouldn't impact your CI this much. I can try to look into it if you need help :)

ctz commented 1 year ago

I think if downstream users benefit from and value this, we should support it. What I'm not sure about is whether the existing build system has benefits compared to cargo-c (simplicity perhaps?) which means we should keep both supported in parallel?

cpu commented 1 year ago

provide binaries for CI usage, see what we do in rav1e, it shouldn't impact your CI this much. I can try to look into it if you need help :)

Ah thank you, that's helpful :-) That approach looks workable. I've added a commit to use it.

@lu-zero One question for you: it looks like the binary releases don't include the ctest subcommand. Is that intentional?

curl -L https://github.com/lu-zero/cargo-c/releases/download/v0.9.22/cargo-c-x86_64-unknown-linux-musl.tar.gz 2>/dev/null | tar -zf - --list
cargo-capi
cargo-cbuild
cargo-cinstall

lu-zero commented 1 year ago

It is not intentional, thank you for spotting it, ctest apparently is not used by many ^^;

lu-zero commented 1 year ago

The next release will have it, you can use cargo capi test in the mean time. (feedback on its usage very welcome btw)

cpu commented 1 year ago

you can use cargo capi test in the mean time.

Perfect, thank you! I've updated the CI task.

cpu commented 1 year ago

What I'm not sure about is whether the existing build system has benefits compared to cargo-c (simplicity perhaps?) which means we should keep both supported in parallel?

@lu-zero Is this topic something you might have thoughts on as the maintainer of cargo-c?

lu-zero commented 1 year ago

cargo-c is simpler to use and in general should always do the right thing regarding dylibs, if it is not provided pre-built it has the same level of annoyance of building cargo itself.

I wrote cargo-c after trying to integrate cargo with Makefile (and automake) myself, so at least in my experience cargo-c is what works the best, but I have no problems in building cargo and nowadays most platforms I focus on already distribute cargo-c by themselves :)

jsha commented 1 year ago

This looks good to me, and the fact that Gentoo is already finding cargo-c useful in packaging is a strong argument in its favor. Just to note, though: I haven't reviewed cargo-c and don't 100% know how it works.

One other thing to note: So far I intentionally haven't added a dynamic library output to the Makefile, because I felt rustls-ffi and rustls aren't stable enough to offer a dynamic library (which tends to carry the expectation of ABI compatbility over a moderately long period of time). I also wanted to read up more on the nuances of supporting dynamic libraries from Rust. However, it sounds like we've wound up accidentally supporting a dynamic library and it hasn't caused problems in practice, so perhaps we should just jump in with both feet?

Also if our goal is that packagers can rely on the cargo-c integration we should document it in the README.

cpu commented 1 year ago

Also if our goal is that packagers can rely on the cargo-c integration we should document it in the README.

I can take a crack at that in the next few days.

kpcyrd commented 11 months ago

I did some testing of this patch in #345 (but I'm not a cargo-c expert myself). Any way I can help out here to get this merged?

cpu commented 11 months ago

Any way I can help out here to get this merged?

If you wanted to pick up the request for documentation for cargo-c in the README that would be helpful! I haven't had a chance and my "in the next few days" estimate was wildly optimistic :sweat_smile:

thesamesam commented 11 months ago

This looks good to me, and the fact that Gentoo is already finding cargo-c useful in packaging is a strong argument in its favor. Just to note, though: I haven't reviewed cargo-c and don't 100% know how it works.

One other thing to note: So far I intentionally haven't added a dynamic library output to the Makefile, because I felt rustls-ffi and rustls aren't stable enough to offer a dynamic library (which tends to carry the expectation of ABI compatbility over a moderately long period of time). I also wanted to read up more on the nuances of supporting dynamic libraries from Rust. However, it sounds like we've wound up accidentally supporting a dynamic library and it hasn't caused problems in practice, so perhaps we should just jump in with both feet?

FWIW, I've not bound you to any sort of ABI guarantee here, so don't worry about that part unless you want to. That is, we build rustls-ffi with a SONAME equal to the version, and we rebuild all consumers on each new version.

Stable ABI would be nice if/when you want to offer it, but I've not sort of assumed a stable ABI or anything - so you have total freedom still.

Thanks for the support as well here folks!

cpu commented 10 months ago

Hi folks,

I've done some more homework looking into this topic and I'm keen on landing this work. I think it's the right direction and I'd like to commit to figuring out what the remaining concerns/blockers might be and how we can address them. Improving the quality of our packaged library and addressing the dynamic linking use case both feel like important steps towards making rustls and rustls-ffi a viable alternative to openssl longer term.

Pulling out some earlier comments to address directly:

This looks good to me, and the fact that Gentoo is already finding cargo-c useful in packaging is a strong argument in its favor.

Based on https://github.com/rustls/rustls-ffi/issues/345 and @kpcyrd's comments I think we can also say that it would be useful for the ArchLinux folks.

I've also done some poking around in NixPkgs and see a few cargo-c based packages there as well (rav1e, libdovi, gstreamer (for the rust bits), and libimagequant). The rustls-ffi package in NixPkgs is currently using the Makefile and a static lib but I suspect I could convert it over to cargo-c after this lands without much hassle.

In sum: I'm encouraged by the fact that three of the Linux distributions I associate most with high quality packaging find cargo-c a useful tool and appear to be invested in the ecosystem.

What I'm not sure about is whether the existing build system has benefits compared to cargo-c (simplicity perhaps?) which means we should keep both supported in parallel?

I think we should start by maintaining the two in parallel. I don't expect it will be much additional work (cargo-c is practically configuration-free and we already have the Makefile). The downstream availability of cargo-c in popular Linux distributions is also quite promising looking.

I do think there's a path towards removing the existing Makefile based tooling for generating the static library when we're ready. I'm less certain about the cmake CMakeLists.txt bits for Windows - I would appreciate folks with more experience with both CMake and Windows weighing in on that topic.

Just to note, though: I haven't reviewed cargo-c and don't 100% know how it works.

I've spent some time reviewing cargo-c and I think I have an OK sense of its mechanics. I also found this blog post helpful for understanding the motivation. If there are particular points you want to understand better I'd be happy to try and do the work to summarize them.

Above all, I'm very reassured by @lu-zero's participation in this issue, and others upstream (e.g. https://github.com/lu-zero/cargo-c/issues/345). I get the sense we can find help with the upstream project when/if we need it.

One other thing to note: So far I intentionally haven't added a dynamic library output to the Makefile, because I felt rustls-ffi and rustls aren't stable enough to offer a dynamic library (which tends to carry the expectation of ABI compatbility over a moderately long period of time).

This was an important call-out. I think we absolutely need to be clear + direct about ABI compatibility being off the table at the current time.

I also wanted to read up more on the nuances of supporting dynamic libraries from Rust. However, it sounds like we've wound up accidentally supporting a dynamic library and it hasn't caused problems in practice, so perhaps we should just jump in with both feet?

+1 - My feeling here is that it will be hard to build confidence in this area without trying it and learning from our mistakes :-) I think if we make sure the ABI compatibility non-guarantee is well understood, and that we advertise dynamic library support as experimental we can build confidence in this area while also hopefully avoiding giving the impression that this is a battle-tested solution.

Also if our goal is that packagers can rely on the cargo-c integration we should document it in the README.

I've tacked on a commit here that updates the README with some documentation. In particular it:

Describes the current static library approach, mentioning some of the downsides and emphasizing use of the Makefile.
Describes ABI compatibility as an explicit non-goal at this time, and emphasizes the full library version should be used as the SONAME in all cases.
Describes using cargo-c to build the dynamic library, and linking it using pkg-config to do the heavy lifting.

I'm sure the wording I've used in some places could use adjustment, feedback is most welcome. It will also need to be reconciled with https://github.com/rustls/rustls-ffi/pull/355.

I also explicitly avoided describing using cargo-c to build the static library. This works fine as far as I can tell, but while we're getting our feet wet I think its better to keep static library users aimed towards the Makefile and emphasize cargo-c for the dynamic linking use-case.

FWIW, I've not bound you to any sort of ABI guarantee here, so don't worry about that part unless you want to. That is, we build rustls-ffi with a SONAME equal to the version, and we rebuild all consumers on each new version.

Great, :+1: This is what I've captured in the README's advice on ABI guarantees as well.

Thanks everyone. I hope we can get this across the line and in the meantime I do appreciate your patience as we work through the details.

cpu commented 10 months ago

It will also need to be reconciled with https://github.com/rustls/rustls-ffi/pull/355.

Done :ballot_box_with_check:

cpu commented 9 months ago

There's another downstream consumer of rustls-ffi, hs-rustls, that provides Haskell bindings for Rustls. They're also carrying a patch to build a dynamic library for rustls-ffi.

kpcyrd commented 9 months ago

The latest cargo-c now gives you more control over your SONAME and I consider https://github.com/lu-zero/cargo-c/issues/345 as completed (let me know if you need more):

[package.metadata.capi.library]
name = "rustls"
version_suffix_components = 3
rustflags = "-Cmetadata=rustls-ffi"

This embeds the full version (all 3 components) into the shared object SONAME, e.g. librustls.so.0.11.0, so any new version is considered ABI breaking.

With a configuration like this, Arch Linux could remove patchelf --set-soname "librustls.so.${pkgver}" "target/$CARCH-unknown-linux-gnu/release/librustls.so" from the PKGBUILD and instead you get to configure this yourself through the source code you release.

cpu commented 9 months ago

@kpcyrd Thanks! I updated the Cargo.toml metadata to specify version_suffix_components = 3.

kpcyrd commented 9 months ago

Arch Linux is now shipping librustls 0.12.0 with this PKGBUILD:

# Maintainer: kpcyrd <kpcyrd[at]archlinux[dot]org>

pkgname=librustls
pkgver=0.12.0
pkgrel=1
pkgdesc="Use rustls from languages other than Rust"
arch=('x86_64')
license=('Apache-2.0' 'MIT')
url='https://github.com/rustls/rustls-ffi'
makedepends=(
  cargo-c
  rust
)
provides=('librustls.so')
options=(!lto)
source=(https://github.com/rustls/rustls-ffi/archive/v${pkgver}/${pkgname}-${pkgver}.tar.gz
        shared-linking.patch)
sha256sums=('7eaffd02528155f561742bd712f5454e68fb771b3eb55d63bf0520429ab717f1'
            '6f1fd82c456e106c265d9778c4a972e84de7fcfc992202993460edb0bd2fa300')

prepare() {
  cd rustls-ffi-${pkgver}
  cargo fetch --locked --target "$CARCH-unknown-linux-gnu"
  patch -Np1 -i ../shared-linking.patch
}

build() {
  cd rustls-ffi-${pkgver}
  RUSTC_BOOTSTRAP=1 cargo cbuild --release --frozen --prefix=/usr
}

package() {
  cd rustls-ffi-${pkgver}
  cargo cinstall --release --frozen --prefix /usr --destdir "${pkgdir}"
  install -Dm644 -t "${pkgdir}/usr/share/licenses/${pkgname}" LICENSE-*
}

# vim: ts=2 sw=2 et:

It's using the official dependency lockfile that is part of the 0.12.0 release and the applied patch is the latest version of this pull request. The patchelf call/dependency is not necessary anymore and has been removed.

jsha commented 6 months ago

Bypassing the branch protections for the Windows / CMake build (already broken, not by this branch).

cpu commented 6 months ago

Thanks for merging! :tada:

kpcyrd commented 6 months ago

Awesome to see this merged, can we get a new release? :)

cpu commented 6 months ago

can we get a new release? :)

I'm hoping we'll get a new major release shortly with Rustls 0.23 (https://github.com/rustls/rustls-ffi/pull/389) but in the meantime I agree it makes sense to do a point release. Preparing that in https://github.com/rustls/rustls-ffi/pull/395

cpu commented 5 months ago

can we get a new release? :)

@kpcyrd v0.12.1 is here, fresh from the crate factory.

kpcyrd commented 5 months ago

Amazing, thanks! :)

Arch Linux is now distributing librustls using this PKGBUILD (no more patching, just plain tarballs and cargo-c):

# Maintainer: kpcyrd <kpcyrd[at]archlinux[dot]org>

pkgname=librustls
pkgver=0.12.1
pkgrel=1
pkgdesc="Use rustls from languages other than Rust"
arch=('x86_64')
license=('Apache-2.0' 'MIT')
url='https://github.com/rustls/rustls-ffi'
depends=(
  gcc-libs
  glibc
)
makedepends=(
  cargo-c
  rust
)
provides=('librustls.so')
options=(!lto)
source=(https://github.com/rustls/rustls-ffi/archive/v${pkgver}/${pkgname}-${pkgver}.tar.gz)
sha256sums=('3a545127987517faf4a200e746481b7eeab6f3c4058b6bf7dea143584c96947b')
b2sums=('0fddfcb5980811a1b80db2bfb578132d627ad8b47f1abeeaf052a7135f43b2e29888aa6aaa89ccd315299b73b147126cd67be41b104b1911d06c8324dee0b0e9')

prepare() {
  cd rustls-ffi-${pkgver}
  cargo fetch --locked --target "$(rustc -vV | sed -n 's/host: //p')"
}

build() {
  cd rustls-ffi-${pkgver}
  RUSTC_BOOTSTRAP=1 cargo cbuild --release --frozen --prefix=/usr
}

package() {
  cd rustls-ffi-${pkgver}
  cargo cinstall --release --frozen --prefix /usr --destdir "${pkgdir}"
  install -Dm644 -t "${pkgdir}/usr/share/licenses/${pkgname}" LICENSE-*
}

# vim: ts=2 sw=2 et:

It ships the following package content:

librustls /usr/
librustls /usr/include/
librustls /usr/include/rustls.h
librustls /usr/lib/
librustls /usr/lib/librustls.so
librustls /usr/lib/librustls.so.0.12.1
librustls /usr/lib/pkgconfig/
librustls /usr/lib/pkgconfig/rustls.pc
librustls /usr/share/
librustls /usr/share/licenses/
librustls /usr/share/licenses/librustls/
librustls /usr/share/licenses/librustls/LICENSE-APACHE
librustls /usr/share/licenses/librustls/LICENSE-ISC
librustls /usr/share/licenses/librustls/LICENSE-MIT

Let me know if anything needs to be changed at some point in the future. :)

rustls / rustls-ffi

cargo-c support #274