Improve WASM build #661
Closed
Improve WASM build #661
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
In `lib.rs` unit tests we are getting build warnings because of how we are importing things, just import with `super::*` unconditionally and be done with it.
We currently use the `cc::Build::flag_if_supported` function to pass in `-Wno-unused-function` however when building for wasm the flag is ignored. Using plain old `flag` seems to work, found by trial and error, I did no further investigation as to why it works and the other does not. Resolves: rust-bitcoin#434
`libc v0.2.149` for the `wasm-unknown-unknown` target does not include the struct `max_align_t`. Feature gate the test that uses it so it doesn't get built in for WASM builds.
Kixunil
reviewed
Nov 1, 2023
| @@ -19,7 +19,7 @@ fn main() { | |||
| base_config.include("depend/secp256k1/") | |||
| .include("depend/secp256k1/include") | |||
| .include("depend/secp256k1/src") | |||
| .flag_if_supported("-Wno-unused-function") // some ecmult stuff is defined but not used upstream | |||
| .flag("-Wno-unused-function") // some ecmult stuff is defined but not used upstream | |||
There was a problem hiding this comment.
Looks like this needs bug report against cc (but we should check with the newest version first).
There was a problem hiding this comment.
Agreed. This change seems pretty sketchy.
| use std::mem; | ||
| use std::os::raw; | ||
| use crate::{types, AlignedType}; | ||
| #[cfg(not(target_arch = "wasm32"))] | ||
| use crate::AlignedType; |
There was a problem hiding this comment.
Shouldn't we redefine the type to be something else on wasm?
This was referenced Nov 2, 2023
apoelstra
added a commit
that referenced
this pull request
Nov 3, 2023
dd6bf7c Fix unit test import statements (Tobin C. Harding) Pull request description: In `lib.rs` unit tests we are getting build warnings because of how we are importing things, just import with `super::*` unconditionally and be done with it. This patch is the only good one out of #661. ACKs for top commit: apoelstra: ACK dd6bf7c Kixunil: ACK dd6bf7c Tree-SHA512: 3970f4c1374ec6de4798bfb52b561e9ac4611ec3a3885edc79639566f777e1fbb502cb36fa7abd015f3fd4a9ca4b6a4931b4ecb2e629e967b4e49391db97a97f
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently we are getting a bunch of warnings when building for WASM, this fixes a few of them leaving just the ones described in #660.
I can't figure out #660 so far.
Fix: #434