Make boringssl-src optional #537
Conversation
No, it will be downloaded but won't be compiled. The optimal way is depend on boringssl-src if openssl feature is not enabled. But there seems to be no way to do so in Cargo. |
|
Seems like on Mac OpenSSL is needed for grpc compilation even if |
|
hug-dev
force-pushed
the
boringssl-optional
branch
5 times, most recently
from
fb1c858 to
6bb19f2
Compare
|
Thanks for the help! After many force pushes, all tests are passing 🌟 |
.github/workflows/ci.yml
Outdated
| @@ -43,7 +43,8 @@ jobs: | |||
| - run: cargo build --no-default-features --features prost-codec | |||
| - run: cd proto && cargo build --no-default-features --features prost-codec | |||
| - run: cargo build | |||
| - run: cargo test --all | |||
| # Some of the bindgen tests generate warnings, see https://github.com/rust-lang/rust-bindgen/issues/1651 | |||
| - run: RUSTFLAGS="" cargo test --all | |||
There was a problem hiding this comment.
How about adding -A deref_nullptr to RUSTFLAGS? So that other warnings can still be checked.
There was a problem hiding this comment.
Ah yes good point, will do that 👌
src/channel.rs
Outdated
| @@ -404,7 +405,7 @@ impl ChannelBuilder { | |||
| // On most modern compiler and architect, c_int is the same as i32, | |||
| // panic directly to simplify signature. | |||
| assert!( | |||
| val <= i32::from(libc::INT_MAX) && val >= i32::from(libc::INT_MIN), | |||
| val <= libc::INT_MAX && val >= libc::INT_MIN, | |||
There was a problem hiding this comment.
I think it's intended to convert as explained at L405.
There was a problem hiding this comment.
I see. On some platforms they might be different and that will error out. Will just allow the clippy lint there then.
hug-dev
force-pushed
the
boringssl-optional
branch
2 times, most recently
from
7badcde to
db78845
Compare
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Signed-off-by: Hugues de Valon <[email protected]>
hug-dev
force-pushed
the
boringssl-optional
branch
from
db78845 to
4c4420a
Compare
| @@ -100,6 +101,7 @@ jobs: | |||
| runs-on: windows-latest | |||
| env: | |||
| LIBCLANG_PATH: 'C:\Program Files\LLVM\bin' | |||
| RUSTFLAGS: "" | |||
There was a problem hiding this comment.
Was about to comment about that :D With nothing there, there was an error that -A deref-nullptr was not recognised by rustc, with only --deny=warnings the errors were still showing. Not sure why
There was a problem hiding this comment.
I guess maybe l102 overwrites L11.
There was a problem hiding this comment.
Yes, I thought that as well, that putting an empty string here would empty the RUSTFLAGS defined before. It still failed but that seems to be for another reason?
|
@hunterlxt PTAL |
| if cfg!(feature = "secure") { | ||
| config.define("gRPC_SSL_PROVIDER", "package"); | ||
| } |
There was a problem hiding this comment.
why not just define the cmake config regardless of the secure feature
There was a problem hiding this comment.
@hug-dev can you add a comment here?
There was a problem hiding this comment.
I did that to fix a failing test on Mac OS, after your recommendation.
Would the following comment be ok?
// OpenSSL is needed for gRPC compilation when the secure feature is on.
There was a problem hiding this comment.
How about
// `package` should only be set for secure feature, otherwise cmake will always search for ssl library.
Signed-off-by: Hugues de Valon <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Fix tikv#536 Signed-off-by: Hugues de Valon <[email protected]> Signed-off-by: Christian Oudard <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Fix tikv#536 Signed-off-by: Hugues de Valon <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Fix tikv#536 Signed-off-by: Hugues de Valon <[email protected]> Signed-off-by: Christian Oudard <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Fix tikv#536 Signed-off-by: Hugues de Valon <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate. Fix tikv#536 Signed-off-by: Hugues de Valon <[email protected]>
If the secure feature is disabled, it's not needed to compile the boringssl-src crate.
This assume that the
securefeature is usingboringssl-srcby default which I think is the current behaviour.A bad aspect of this is that
boringssl-srcwill be compiled even ifopensslis chosen but I think that is currently the case anyway?Fix #536