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

Set /std:c11 ccflag for Windows in Rust bindings #354

Merged
merged 1 commit into from
Aug 31, 2023

Conversation

jtraglia
Copy link
Member

@jtraglia jtraglia commented Aug 31, 2023

This PR fixes our Windows issues. This is an alternative to #353.

See #353 for details about the problem. To be brief, if __STDC_VERSION__ isn't defined as c99 or greater, blst will typedef a bool as an int (this check is here btw). There is a bug in bindgen associated with this. MSVC doesn't define __STDC_VERSION__ by default, but we can use the /std flag to set it to c11, which is the earliest version it supports.

Fixes #318.
Fixes #331.

Big thanks to @pawanjay176 & @michaelsproul for identifying this.

@asn-d6
Copy link
Contributor

asn-d6 commented Aug 31, 2023

Thanks!

@asn-d6 asn-d6 merged commit d35b0f3 into ethereum:main Aug 31, 2023
32 checks passed
@jtraglia jtraglia deleted the windows-cstd branch August 31, 2023 19:13
@jtraglia jtraglia mentioned this pull request Sep 11, 2023
@ghost
Copy link

ghost commented Dec 14, 2023

@jtraglia @asn-d6 I think this is not a correct fix and causing issues when the toolchain is set to GNU

https://stackoverflow.com/questions/58226545/how-to-switch-between-rust-toolchains

GCC compiler is complaining that /std:c11 is a wrong flag, so I think the flag should be only added when the toolchain is MSVC

https://rust-lang.github.io/rustup/installation/windows.html

@ghost ghost mentioned this pull request Dec 14, 2023
1 task
@jtraglia
Copy link
Member Author

Hey @kaliubuntu0206 👋 I'm okay with only setting it for MSVC, sounds like a mistake on my part. Would you like to make a PR for this? Also worth noting, blst appears to have fixed this problem (supranational/blst@9c87d4a). I don't think we should update blst right now though, since we would prefer to use a stable release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stack overflow in Rust benchmarks on Windows Broken CI in rust-windows because of rust-1.7.0
2 participants