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.

Sign up for GitHub

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

Jump to bottom

Add config files to ease contribution #34

Closed
wants to merge 1 commit into from
Closed

Add config files to ease contribution #34

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .cargo/config.toml
View file
Open in desktop
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
@@ -1,2 +1,10 @@
[target.wasm32-unknown-unknown]
runner = "wasm-bindgen-test-runner"
rustflags = [
"-C",
"target-feature=+atomics,+bulk-memory",
]

# Only kicks in if actually compiled with unstable compiler.
[unstable]
build-std = ["std,panic_abort"]
2 changes: 1 addition & 1 deletion .github/workflows/audit.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ jobs:
with:
tool: cargo-audit
- name: Run Audit
run: cargo audit -D warnings
run: cargo +stable audit -D warnings
Copy link
Owner

Choose a reason for hiding this comment

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

The Rust stable channel is already the default.

15 changes: 9 additions & 6 deletions .github/workflows/coverage-documentation.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,14 +24,17 @@ jobs:
strategy:
matrix:
mt:
- { id: 0 }
- {
id: 0,
description: without Atomics,
envs: "CARGO_TARGET_wasm32_unknown_unknown_RUSTDOCFLAGS= cargo +stable --verbose test",
toolchain: "stable",
}
- {
id: 1,
description: with Atomics,
component: --component rust-src,
cflags: -matomics -mbulk-memory,
flags: "-Ctarget-feature=+atomics,+bulk-memory",
args: "-Zbuild-std=panic_abort,std",
toolchain: "nightly",
}

steps:
Expand All @@ -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 }}
Copy link
Owner

Choose a reason for hiding this comment

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

Test coverage only works on nightly right now.
This is a limitation by Rust and wasm-bindgen.

- name: Test
env:
CHROMEDRIVER: chromedriver
Expand All @@ -58,7 +61,7 @@ jobs:
WASM_BINDGEN_UNSTABLE_TEST_PROFRAW_OUT: coverage-output
run: |
mkdir coverage-output
cargo test --all-features --target wasm32-unknown-unknown -Ztarget-applies-to-host -Zhost-config ${{ matrix.mt.args }} --tests
${{ matrix.mt.envs }} cargo +${{ matrix.mt.toolchain }} test --all-features --target wasm32-unknown-unknown -Ztarget-applies-to-host -Zhost-config ${{ matrix.mt.args }} --tests
- name: Prepare Object Files
env:
CFLAGS_wasm32_unknown_unknown: ${{ matrix.mt.cflags }}
Expand Down
2 changes: 2 additions & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@
/Cargo.lock
/coverage-input
/coverage-output
/.vscode/*
!.vscode/settings.json
26 changes: 4 additions & 22 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,39 +11,21 @@ This crate has some code paths that depend on Wasm Atomics, which has some prere
- Cargo's [`build-std`].
- The `atomics` and `bulk-memory` target features.

Example usage:

```sh
# Installing Rust nightly and necessary components:
rustup toolchain install nightly --target wasm32-unknown-unknown --component rust-src
# 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).
Copy link
Owner

Choose a reason for hiding this comment

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

Same as above, a rust-toolchain.toml would change the default, which is undesirable.


### Rust Analyzer

To get proper diagnostics for Rust Atomics it can be helpful to configure Rust Analyzer to support
that.

Here is an example configuration for Visual Studio Code:

```json
"rust-analyzer.cargo.target": "wasm32-unknown-unknown",
"rust-analyzer.cargo.extraArgs": [
"-Zbuild-std=panic_abort,std"
],
"rust-analyzer.cargo.extraEnv": {
"RUSTUP_TOOLCHAIN": "nightly",
"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).
Copy link
Owner

Choose a reason for hiding this comment

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

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:
Copy link
Owner

Choose a reason for hiding this comment

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

They do not, tests are also run on native.


```sh
RUSTFLAGS=-Ctarget-feature=+atomics,+bulk-memory cargo +nightly test -Zbuild-std=panic_abort,std --target wasm32-unknown-unknown
cargo test --target wasm32-unknown-unknown
```

Additionally, keep in mind that usage of [`#[should_panic]`](`should_panic`) is known to cause
Expand Down
Loading