cargo-c support #274

lu-zero · 2022-12-10T13:58:22Z

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 · 2023-03-03T19:57:52Z

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

lu-zero · 2023-03-10T13:48:01Z

I freshened it again.

cpu · 2023-08-02T13:13:36Z

@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 · 2023-08-02T13:19:38Z

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 · 2023-08-02T13:37:33Z

@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 · 2023-08-02T13:54:42Z

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 · 2023-08-02T13:58:05Z

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 · 2023-08-02T15:57:33Z

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

lu-zero · 2023-08-03T09:01:33Z

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

cpu · 2023-08-03T13:36:41Z

you can use cargo capi test in the mean time.

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

cpu · 2023-08-08T15:22:38Z

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 · 2023-08-09T08:51:39Z

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

cpu

I think this should be merged since it has value to downstream consumers and I don't expect the maintenance burden to be large. We can revisit whether to remove existing parts of the build system separately.

@ctz @jsha Would one of you be willing to +1 or suggest changes? I've contributed code to this branch so I think it would be nice to have an independent approval.

jsha · 2023-08-18T15:18:46Z

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 · 2023-08-18T16:52:13Z

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 · 2023-09-28T22:34:59Z

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 · 2023-09-29T18:07:43Z

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 😅

thesamesam · 2023-09-30T01:48:42Z

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 · 2023-10-16T20:46:38Z

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 #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. lu-zero/cargo-c#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 #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, 👍 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 · 2023-10-17T20:04:06Z

It will also need to be reconciled with #355.

Done ☑️

cpu · 2023-11-29T15:14:01Z

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 · 2023-11-29T16:10:44Z

The latest cargo-c now gives you more control over your SONAME and I consider lu-zero/cargo-c#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.

This will prevent bitrot with the cargo-c support.

This commit details our current thinking w.r.t static vs dynamic linking for rustls-ffi. It also adds initial documentation of using `cargo-c` to build a dynamic library. In particular we emphasize that: a) we make **no** ABI stability guarantees b) the dynamic library/cargo-c support is considered experimental

cpu · 2023-11-29T20:55:51Z

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

kpcyrd · 2023-12-07T12:23:28Z

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

Looks good, let's land it!

jsha · 2024-03-08T19:50:49Z

README.md

+
+## Static Library
+
+In its current for rustls-ffi's `Makefile` infrastructure will generate a static


Suggested change

In its current for rustls-ffi's `Makefile` infrastructure will generate a static

In its current form rustls-ffi's `Makefile` infrastructure will generate a static

jsha · 2024-03-08T20:04:12Z

README.md

+    pkg-config --libs rustls
+    pkg-config --cflags rustls


Nit: this seems to suggest that these pkg-config commands directly manipulate $LDLIBS and $CFLAGS.

Suggested change

pkg-config --libs rustls

pkg-config --cflags rustls

LDLIBS="$(pkg-config --libs rustls)"

CFLAGS="$(pkg-config --cflags rustls)"

jsha · 2024-03-08T20:58:15Z

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

cpu · 2024-03-11T13:56:12Z

Thanks for merging! 🎉

kpcyrd · 2024-03-11T14:54:08Z

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

cpu · 2024-03-11T16:09:15Z

can we get a new release? :)

I'm hoping we'll get a new major release shortly with Rustls 0.23 (#389) but in the meantime I agree it makes sense to do a point release. Preparing that in #395

cpu · 2024-03-21T18:06:03Z

can we get a new release? :)

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

kpcyrd · 2024-03-21T20:45:57Z

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

lu-zero force-pushed the cargo-c branch from 5d3e109 to 76542bb Compare March 10, 2023 13:47

cpu force-pushed the cargo-c branch from 76542bb to 30fec23 Compare August 2, 2023 12:50

cpu self-assigned this Aug 2, 2023

cpu force-pushed the cargo-c branch from 30fec23 to cd2544a Compare August 2, 2023 13:37

cpu force-pushed the cargo-c branch from 7588ebf to cd254d9 Compare August 2, 2023 14:02

cpu force-pushed the cargo-c branch from cd254d9 to 65fc2c4 Compare August 3, 2023 13:36

cpu approved these changes Aug 17, 2023

View reviewed changes

cpu mentioned this pull request Sep 26, 2023

Building as shared library #345

Closed

kpcyrd mentioned this pull request Sep 30, 2023

More granular SONAME versioning lu-zero/cargo-c#345

Closed

cpu mentioned this pull request Oct 16, 2023

docs: various small fixes to the README documentation #355

Merged

cpu force-pushed the cargo-c branch from 65fc2c4 to fa06a4c Compare October 16, 2023 19:45

Move the language configuration to cbindgen.toml

0a6c7d9

cpu force-pushed the cargo-c branch from f1888c2 to 0c8e4c1 Compare October 17, 2023 20:01

kpcyrd approved these changes Oct 17, 2023

View reviewed changes

kpcyrd mentioned this pull request Nov 11, 2023

Add version_suffix_components setting lu-zero/cargo-c#352

Merged

lu-zero and others added 3 commits November 29, 2023 15:53

Initial cargo-c support

713973d

ci: add task for cargo-c.

58dd02a

This will prevent bitrot with the cargo-c support.

cpu force-pushed the cargo-c branch from 0c8e4c1 to 75b6882 Compare November 29, 2023 20:53

kpcyrd mentioned this pull request Feb 25, 2024

Port to librustls 0.12.0 curl/curl#12989

Closed

cpu mentioned this pull request Feb 28, 2024

Simplify build systems, consider removing cmake #390

Open

jsha approved these changes Mar 8, 2024

View reviewed changes

README nits

761c544

jsha merged commit a2adbd1 into rustls:main Mar 8, 2024
19 of 22 checks passed

lu-zero deleted the cargo-c branch March 22, 2024 06:39

kpcyrd mentioned this pull request Mar 27, 2024

Regression in rustls pkg-config detection curl/curl#13200

Closed

cpu mentioned this pull request Apr 4, 2024

ci: add Linux pkg-config/.so test coverage #412

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo-c support #274

cargo-c support #274

lu-zero commented Dec 10, 2022

cpu commented Mar 3, 2023

lu-zero commented Mar 10, 2023

cpu commented Aug 2, 2023 •

edited

thesamesam commented Aug 2, 2023

lu-zero commented Aug 2, 2023

ctz commented Aug 2, 2023

cpu commented Aug 2, 2023 •

edited

lu-zero commented Aug 2, 2023

lu-zero commented Aug 3, 2023

cpu commented Aug 3, 2023

cpu commented Aug 8, 2023

lu-zero commented Aug 9, 2023

cpu left a comment

jsha commented Aug 18, 2023

cpu commented Aug 18, 2023

kpcyrd commented Sep 28, 2023

cpu commented Sep 29, 2023

thesamesam commented Sep 30, 2023

cpu commented Oct 16, 2023

cpu commented Oct 17, 2023

cpu commented Nov 29, 2023

kpcyrd commented Nov 29, 2023

cpu commented Nov 29, 2023

kpcyrd commented Dec 7, 2023

jsha left a comment

jsha Mar 8, 2024

jsha Mar 8, 2024

jsha commented Mar 8, 2024

cpu commented Mar 11, 2024

kpcyrd commented Mar 11, 2024

cpu commented Mar 11, 2024

cpu commented Mar 21, 2024

kpcyrd commented Mar 21, 2024 •

edited


		## Static Library

		In its current for rustls-ffi's `Makefile` infrastructure will generate a static

	In its current for rustls-ffi's `Makefile` infrastructure will generate a static
	In its current form rustls-ffi's `Makefile` infrastructure will generate a static

cargo-c support #274

cargo-c support #274

Conversation

lu-zero commented Dec 10, 2022

cpu commented Mar 3, 2023

lu-zero commented Mar 10, 2023

cpu commented Aug 2, 2023 • edited

thesamesam commented Aug 2, 2023

lu-zero commented Aug 2, 2023

ctz commented Aug 2, 2023

cpu commented Aug 2, 2023 • edited

lu-zero commented Aug 2, 2023

lu-zero commented Aug 3, 2023

cpu commented Aug 3, 2023

cpu commented Aug 8, 2023

lu-zero commented Aug 9, 2023

cpu left a comment

Choose a reason for hiding this comment

jsha commented Aug 18, 2023

cpu commented Aug 18, 2023

kpcyrd commented Sep 28, 2023

cpu commented Sep 29, 2023

thesamesam commented Sep 30, 2023

cpu commented Oct 16, 2023

cpu commented Oct 17, 2023

cpu commented Nov 29, 2023

kpcyrd commented Nov 29, 2023

cpu commented Nov 29, 2023

kpcyrd commented Dec 7, 2023

jsha left a comment

Choose a reason for hiding this comment

jsha Mar 8, 2024

Choose a reason for hiding this comment

jsha Mar 8, 2024

Choose a reason for hiding this comment

jsha commented Mar 8, 2024

cpu commented Mar 11, 2024

kpcyrd commented Mar 11, 2024

cpu commented Mar 11, 2024

cpu commented Mar 21, 2024

kpcyrd commented Mar 21, 2024 • edited

cpu commented Aug 2, 2023 •

edited

cpu commented Aug 2, 2023 •

edited

kpcyrd commented Mar 21, 2024 •

edited