-
Notifications
You must be signed in to change notification settings - Fork 558
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
[hyper] Actually compile the C API #4678
Conversation
@giordano Please have a look. |
Seems that v0.14.18 does not emit .so/.dylib files by default. |
H/hyper/build_tarballs.jl
Outdated
] | ||
|
||
# Bash recipe for building across all platforms | ||
script = raw""" | ||
cd $WORKSPACE/srcdir/hyper*/ | ||
cargo build --release | ||
sed '21 a [lib]\ncrate-type=["cdylib"]\n' -i Cargo.toml |
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.
Isn't there a way to enable building of the library without having to edit the file manually? Also, this sed script doesn't seem very sustainable as it hardcodes a line number, probability it'll bit-rot at some point is very high.
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.
hyperium/hyper@1c66370 seems to suggest you can use --crate-type
?
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.
That would require a nightly version of rust. Do we have that available in the environment?
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.
No 🤦 Why do they do this? 😩
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.
For now, we can switch back to v0.14.17 if the sed script appears ugly.
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.
I think they made the decision because the C API is unstable, so it is supposed to be used with nightly instead of stable rust.
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.
Yeah, let's rollback to 0.14.7 and update once we have a new Rust toolchain with this capability
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.
Done!
Thanks! |
As a note: once the rust version within BinaryBuilder is updated, we can use the following command to build the dynamic library:
|
Does that actually work for you? #4694 (comment) |
Yes. You would need nightly rust newer than 1.61.0-nightly (2022-03-02) to make the command work. |
We don't use nightly 🙂 |
Yes, so we need to wait until 1.61.0 becomes stable. |
With Rust 1.61.0 I get sandbox:${WORKSPACE}/srcdir/hyper-0.14.19 # RUSTFLAGS="--cfg hyper_unstable_ffi" cargo rustc --release --crate-type cdylib -Z unstable-options --features client,http1,http2,ffi
error: the `-Z` flag is only accepted on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
sandbox:${WORKSPACE}/srcdir/hyper-0.14.19 # RUSTFLAGS="--cfg hyper_unstable_ffi" cargo rustc --release --crate-type cdylib --features client,http1,http2,ffi
error: the `crate-type` flag is unstable, and only available on the nightly channel of Cargo, but this is the `stable` channel
See https://doc.rust-lang.org/book/appendix-07-nightly-rust.html for more information about Rust release channels.
See https://github.com/rust-lang/cargo/issues/10083 for more information about the `crate-type` flag. Sounds like |
Unfortunately, this API has not been stabilized yet... |
For the time being, I'm just reverting hyperium/hyper@1c66370: #5051 |
According to https://docs.rs/hyper/latest/hyper/ffi/index.html, the C API of hyper is not compiled by default. Extra options and environment variables need to be set to make the C API work.