Add config files to ease contribution #34
Conversation
There was a problem hiding this comment.
What exactly is the goal here?
I've put a fair amount of thought into the current setup and I'm happy to share any explanations why anything in particular was done in a specific way, but it would help if you declare exactly what issue you are having and what you are trying to improve.
There was a problem hiding this comment.
This is changing the default target to use the atomics target feature, which is undesirable.
The atomics target feature is experimental in Rust, so this should require opt-in, not opt-out, even for developers.
| @@ -26,4 +26,4 @@ jobs: | |||
| with: | |||
| tool: cargo-audit | |||
| - name: Run Audit | |||
| run: cargo audit -D warnings | |||
| run: cargo +stable audit -D warnings | |||
There was a problem hiding this comment.
The Rust stable channel is already the default.
| @@ -46,7 +49,7 @@ jobs: | |||
| - name: Install Rust nightly | |||
| run: | | |||
| rustup toolchain install nightly --profile minimal --target wasm32-unknown-unknown ${{ matrix.mt.component }} | |||
| rustup default nightly | |||
| rustup default ${{ matrix.mt.toolchain }} | |||
There was a problem hiding this comment.
Test coverage only works on nightly right now.
This is a limitation by Rust and wasm-bindgen.
| # Example `cargo build` usage: | ||
| RUSTFLAGS=-Ctarget-feature=+atomics,+bulk-memory cargo +nightly build -Zbuild-std=panic_abort,std --target wasm32-unknown-unknown | ||
| ``` | ||
| These are set using [`rust-toolchain.toml`](./rust-toolchain.toml). |
There was a problem hiding this comment.
Same as above, a rust-toolchain.toml would change the default, which is undesirable.
| "RUSTFLAGS": "-Ctarget-feature=+atomics,+bulk-memory" | ||
| }, | ||
| ``` | ||
| It takes the settings from `rust-toolchain.toml`, but we also need to specify a target, as seen for vscode in [.vscode/settings.json](./.vscode/settings.json). |
There was a problem hiding this comment.
I don't want to assume that everybody uses VS Code, which is why I didn't include it in the repo.
My assumption is that it should be good enough to include this in the CONTRIBUTING.md, do you believe it is not?
|
|
||
| ## Testing | ||
|
|
||
| Tests are run as usual, but tests that require Wasm Atomics can be run like this: | ||
| Tests are run as usual, but also rewuire an explicit target: |
There was a problem hiding this comment.
They do not, tests are also run on native.
|
Gonna go ahead and close this as per the last review. |
The pipeline isnt complete, this was just an experiment to check if it was possible to override the toolchain file, and I'll happily put in the work if you will actually accept this.