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

Build and test wasm on MacOS in CI #24

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

rishflab
Copy link
Contributor

@rishflab rishflab commented Jul 15, 2021

The default clang installation provided in MacOS does not work. We fixed
this locally by installing clang via llvm through brew and setting the
CC and AR env variables to point at the new clang installation.The CI
MacOS image seems to already have llvm installed so we did not need to
install llvm on the CI.

See rust-bitcoin/rust-secp256k1#254 for more
details

.github/workflows/rust.yml Outdated Show resolved Hide resolved
@rishflab rishflab force-pushed the mac-os-build branch 2 times, most recently from 8e9f7f7 to 05e62c4 Compare July 16, 2021 00:40
.github/workflows/rust.yml Outdated Show resolved Hide resolved
Cargo.toml Show resolved Hide resolved
contrib/test.sh Outdated Show resolved Hide resolved
.github/workflows/rust.yml Outdated Show resolved Hide resolved
The default clang installation provided in MacOS does not work. We fixed
 this locally by installing clang via llvm through brew and setting the
 CC and AR env variables to point at the new clang installation.The CI
 MacOS image seems to already have llvm installed so we did not need to
 install llvm on the CI.

See rust-bitcoin/rust-secp256k1#254 for more
details
Copy link
Contributor

@thomaseizinger thomaseizinger left a comment

Choose a reason for hiding this comment

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

Looks good from my side! Thanks for sticking with this @rishflab :)

@jonasnick / @sanket1729 Do you mind having a look at this?

Copy link
Collaborator

@jonasnick jonasnick left a comment

Choose a reason for hiding this comment

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

I don't know anything about WASM, but this PR is fine from my POV since the other tests aren't affected. Why do we remove the build_wasm task?

@rishflab
Copy link
Contributor Author

I don't know anything about WASM, but this PR is fine from my POV since the other tests aren't affected. Why do we remove the build_wasm task?

The contrib/test.sh script does the build anyway (through wasm-pack) so this step is kind of redundant?

@rishflab rishflab requested a review from jonasnick July 21, 2021 04:39
@jonasnick
Copy link
Collaborator

Sorry, missed your answer. Ok.
ACK 149d686

@jonasnick
Copy link
Collaborator

Thank you for the PR.

@jonasnick jonasnick merged commit f610090 into BlockstreamResearch:master Aug 3, 2021
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.

4 participants