Skip to content
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

fix bindgen for BoringSSL on Windows #2089

Closed
wants to merge 1 commit into from
Closed

Conversation

HMaker
Copy link

@HMaker HMaker commented Nov 5, 2023

Comment on lines 176 to 177
.arg("--allowlist-file=.*[/\\\\]openssl/[^/]+\\.h")
.arg("--no-recursive-allowlist")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing whitespace

@@ -761,7 +761,7 @@ impl X509 {
ffi::PEM_read_bio_X509(bio.as_ptr(), ptr::null_mut(), None, ptr::null_mut());
if r.is_null() {
let err = ffi::ERR_peek_last_error();
if ffi::ERR_GET_LIB(err) as X509LenTy == ffi::ERR_LIB_PEM
if ffi::ERR_GET_LIB(err) as X509LenTy == ffi::ERR_LIB_PEM.try_into().unwrap()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this change for?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a build error and cargo suggested me to make that change, it had a mismatch between i32 and u32. I don't know what caused that, maybe the boringssl version? I built boringssl from commit 5d58c559ace6a24ea6613e412b26bd4c50668ab3.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not needed for either the version we're using in CI, nor for boringssl HEAD as far as I can tell, so it shouldn't be part of this PR

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed that and now got the same error

error[E0308]: mismatched types
   --> openssl\src\x509\mod.rs:764:62
    |
764 |                     if ffi::ERR_GET_LIB(err) as X509LenTy == ffi::ERR_LIB_PEM
    |                        ----------------------------------    ^^^^^^^^^^^^^^^^ expected `u32`, found `i32`
    |                        |
    |                        expected because this is `u32`
    |
help: you can convert an `i32` to a `u32` and panic if the converted value doesn't fit
    |
764 |                     if ffi::ERR_GET_LIB(err) as X509LenTy == ffi::ERR_LIB_PEM.try_into().unwrap()
    |                                                                              ++++++++++++++++++++

That BoringSSL version is used by Python's cryptography 0.42

@alex
Copy link
Collaborator

alex commented Nov 5, 2023

If this relies on a new version of bindgen, you'll need to change openssl-sys/Cargo.toml as well

@HMaker
Copy link
Author

HMaker commented Nov 5, 2023

Looks like the linux build of boringssl now fails, probably that -include flag is still needed there?

@alex
Copy link
Collaborator

alex commented Nov 5, 2023

Some debugging will be required. Testing locally, it seems to work without -include.

@HMaker HMaker changed the title fix bindgen for BoringSSL fix bindgen for BoringSSL on Windows Nov 5, 2023
- remove the -include flag (works for bindgen 0.68+)
- fix allowlist-file regex to match Windows' path backslash
@HMaker
Copy link
Author

HMaker commented Nov 5, 2023

Yes it's the -include flag, I guess the CI is using old version of bindgen?

@HMaker HMaker requested a review from alex November 5, 2023 04:06
alex added a commit to alex/rust-openssl that referenced this pull request Nov 26, 2023
also make path seperator windows friendly

fixes sfackler#2087, sfackler#2086
closes sfackler#2089
alex added a commit to alex/rust-openssl that referenced this pull request Nov 26, 2023
also make path seperator windows friendly

fixes sfackler#2087
fixes sfackler#2086
closes sfackler#2089
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants