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 WASM build and add a job in CI #195

Merged
merged 2 commits into from
Feb 21, 2025

Conversation

RCasatta
Copy link
Collaborator

@RCasatta RCasatta commented Jan 23, 2024

close #194

@RCasatta RCasatta marked this pull request as ready for review January 23, 2024 15:58
@RCasatta
Copy link
Collaborator Author

Note downstream you can use this without the dev-dep obviously, this is just needed to enforce wasm check in CI so that we are not breaking it in the future

- name: Checkout Toolchain
uses: dtolnay/rust-toolchain@stable
- run: rustup target add wasm32-unknown-unknown
- run: cargo check --target wasm32-unknown-unknown
Copy link
Member

Choose a reason for hiding this comment

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

In d6c5892:

trailing whitespace

Copy link
Member

Choose a reason for hiding this comment

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

@RCasatta ping -- if you fix the whitespace I can ack and merge this.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed

@apoelstra
Copy link
Member

LGTM, but would this check have actually caught the getrandom thing? My understanding is that if you compile without the js feature you'll get a runtime panic, so just running cargo check won't have noticed it.

Regardless, this CI job is an improvement over having nothing.

@RCasatta
Copy link
Collaborator Author

It seems it's a compile time check and without the getrandom thing the job would fail https://github.com/RCasatta/rust-elements/actions/runs/10578325188/job/29308193852?pr=2

@delta1 delta1 merged commit 7debedd into ElementsProject:master Feb 21, 2025
6 checks passed
@apoelstra
Copy link
Member

Heads up that on my system I had to add another cargo update -p line for bumpalo after this. cargo does the right thing if you use 1.56.1 consistently but if you use a recent cargo and then downgrade to 1.56.1 it's a bit of work.

But I want to update the MSRV here to 1.63 and start committing lockfiles anyway so I won't worry about it for now.

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.

Make it work on WASM
3 participants