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

Downgrade requirements for WASM dependencies #58

Merged
merged 1 commit into from Sep 12, 2022

Conversation

djc
Copy link
Contributor

@djc djc commented Sep 12, 2022

WebAssembly support was added in #38 targeting the current versions versions of wasm-bindgen and js-sys at the time, which is reasonable. Unfortunately the whoami crate, in trying to support older Rust versions, has reduced the upper version of wasm-bindgen that it supports, see ardaku/whoami#43.

This PR thus reduces the minimum version required for wasm-bindgen. I used cargo +nightly update -Z minimal-versions with cargo build --target wasm32-unknown-unknown to find the earliest version that compiles. We could theoretically set up checks in CI to verify this on an ongoing basis, but this might be overkill for now.

cc @Kijewski

WebAssembly support was added in strawlab#38 targeting the current versions
versions of wasm-bindgen and js-sys at the time, which is
reasonable. Unfortunately the whoami crate, in trying to support
older Rust versions, has reduced the upper version of wasm-bindgen
that it supports, see ardaku/whoami#43.

This PR thus reduces the minimum version required for wasm-bindgen.
I used `cargo +nightly update -Z minimal-versions` with
`cargo build --target wasm32-unknown-unknown` to find the earliest
version that compiles. We could theoretically set up checks in
CI to verify this on an ongoing basis, but this might be overkill
for now.
@astraw
Copy link
Member

astraw commented Sep 12, 2022

Rolling back the versions in Cargo.toml seems reasonable to me if you tested things work with those versions. I'm +1. @Kijewski what do you think?

(Adding CI tests to prove that the minimum package versions specified in Cargo.toml do function is a good idea - for all crates - but IMO not necessary at this stage. I haven't seen an implementation of such, nor have I looked.)

@djc
Copy link
Contributor Author

djc commented Sep 12, 2022

Rolling back the versions in Cargo.toml seems reasonable to me if you tested things work with those versions.

Note that I only compile-tested one target (as mentioned in the description), I'm not normally a WASM user and running the tests against a WASM target didn't seem to work.

(Adding CI tests to prove that the minimum package versions specified in Cargo.toml do function is a good idea - for all crates - but IMO not necessary at this stage. I haven't seen an implementation of such, nor have I looked.)

I think a number of crates (like Tokio) have started to exercise cargo update -Z minimal-versions in CI, but there are still a number of pain points to be ironed out (which is why Cargo hasn't stabilized minimal-versions yet).

@Kijewski
Copy link
Collaborator

Thank you! The PR looks good to me, and I'll try it this evening.

It is possible to use -Zminimal-versions in stable, and also quite old versions: ci.yml. The documentation for RUSTC_BOOTSTRAP is a bit hidden, and you are not supposed to use it, but it works perfectly fine for use cases like this.

@Kijewski Kijewski merged commit 2443f01 into strawlab:main Sep 12, 2022
@astraw
Copy link
Member

astraw commented Sep 12, 2022

This is published in release 0.1.48.

@lopopolo lopopolo added the Tier-2 Rust Tier-2 platform label Oct 1, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Tier-2 Rust Tier-2 platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants