Closed cpu closed 6 months ago
@Kangie I'd be interested to hear how this branch works for you. It seems to do the right thing in my local testing :crossed_fingers:
@kpcyrd If you wouldn't mind testing I'm hoping this branch lets you drop using RUSTC_BOOTSTRAP
in your Arch package.
I will try and test with this in the near future, but will be busy over this weekend :(
Pinging @thesamesam as the Gentoo rustls-ffi maintainer who might get a chance to look first!
rustls-ffi / Clippy nightly (optional) (pull_request) Failing after 18s
Thanks! This PR works for us in Gentoo. I'll pull it in.
Thanks! This PR works for us in Gentoo. I'll pull it in.
I lied! It looks like rustls.h
may not be installed when this is applied?
It also builds without RUSTC_BOOTSTRAP=1
for me but the header file is not in the package anymore:
==> Running checkpkg
-> Checking packages
usr/include/ <
usr/include/rustls.h <
==> No soname differences for librustls.
Maybe @lu-zero has thoughts on this too? :)
Ah, thank you both for reporting back. I did my initial testing with config like the following, and observed the header being included:
[package.metadata.capi.install.include]
asset = [{from = "src/rustls.h", to = "" }]
Right before pushing this branch I thought the to = ""
was strange and tried an alternative that I pushed:
[package.metadata.capi.install]
asset = [{from = "src/rustls.h", to = "include" }]
I think when I re-tested this change I must have made a mistake because I see the same results you folks reported: the header file was missing. (It's always the "one last tweak before bed" that gets you...)
I've reverted that change and now see the correct behaviour again:
[daniel@blanc:~/Code/Rust/rustls-ffi]$ git rev-parse HEAD
dd0a1131b50a92479bdb9e18e1a56dd28ad09de1
[daniel@blanc:~/Code/Rust/rustls-ffi]$ cargo cinstall --release --prefix=/tmp/out
Compiling rustls-ffi v0.12.1 (/home/daniel/Code/Rust/rustls-ffi)
Finished release [optimized] target(s) in 0.99s
Building pkg-config files
Populating uninstalled header directory
Installing pkg-config file
Installing header file
Installing static library
Installing shared library
[daniel@blanc:~/Code/Rust/rustls-ffi]$ tree /tmp/out
/tmp/out
├── include
│ └── rustls.h
└── lib
├── librustls.a
├── librustls.so -> librustls.so.0.12.1
├── librustls.so.0 -> librustls.so.0.12.1
├── librustls.so.0.12.1
└── pkgconfig
└── rustls.pc
4 directories, 6 files
Could you give this branch another spin and see if it resolves the issue for you folks as well?
I tried again with the new patch and it seems fine for me :+1:
(Looking at your output, you may want to update your cargo-c binary) :)
I tried again with the new patch and it seems fine for me 👍
Excellent, thank you!
(Looking at your output, you may want to update your cargo-c binary) :)
I'm using cargo-c 0.9.24+cargo-0.73.0 but it looks like nixpkgs unstable updated to 0.9.29 since I last rebuilt. I don't think anyone has started packaging .30 or .31 yet. I'll follow up on that (Edit: https://github.com/NixOS/nixpkgs/pull/299789).
I'm going to merge this so I can include it in the 0.13 release. If it turns out there's still an issue with Gentoo builds we can tweak futher. I'll also backport the fix to a 0.12.2 point release (edit: https://github.com/rustls/rustls-ffi/pull/401).
Maybe @lu-zero has thoughts on this too? :)
I think the default to
may be changed since to=""
probably is more common. I'll think more about it and in case change it and document the default to
. Thank you for pointing out to=""
is a bit annoying.
Maybe @lu-zero has thoughts on this too? :)
I think the default
to
may be changed sinceto=""
probably is more common. I'll think more about it and in case change it and document the defaultto
. Thank you for pointing outto=""
is a bit annoying.
I just checked and behaves as intended. I would suggest to keep the include subdir since tends to be more future proof.
[package.metadata.capi.install.include]
-asset = [{from = "src/rustls.h", to = "" }]
+asset = [{from = "src/rustls.h" }]
@lu-zero Ah! Interesting to know we could omit the to
. I think I agree with you that we might as well keep it as-is for now. It's working as intended and I doubt we'll have reason to revisit soon.
Presently we pre-generate the rustls.h header file using
cbindgen
, commit the result tosrc/rustls.h
, and check that the generated result matches the checked-in result in CI. However, the new experimental cargo-c build regenerates its own header file usingcbindgen
by default unless told to do otherwise. We'd prefer it didn't do this because we're using acbindgen
feature that requires nightly rust.This commit updates the
Cargo.toml
capi metadata to tell cargo-c to skip generation of its own header file. We then configure the pre-generated checked-in header file as an asset to be copied into the install include directory.This better matches how the
Makefile
build allowed building the static lib without needing nightly rust orcbindgen
and resolves https://github.com/rustls/rustls-ffi/issues/397.