wasm-bindgen-rayon icon indicating copy to clipboard operation
wasm-bindgen-rayon copied to clipboard

Using clippy got harder with the explicit target_features `atomics` check.

Open rustonaut opened this issue 3 years ago • 6 comments

https://github.com/GoogleChromeLabs/wasm-bindgen-rayon/blob/4cd0666d2089886d6e8731de2371e7210f848c5d/src/lib.rs#L14-L15

This line causes clippy to fail if you didn't set the appropriate compiler flags and used the right --target arguments.

Commenting out the lines prevents this failure as it then triggers in a part of the compiler clippy doesn't care about.

The problem with this is that it makes thinks like clippy check --all-features --all-targets not usable (without some hacks), and in turn complicates dev and CI workflows of projects/workspaces for which wasm is just one of multiple targets.

I'm not sure what the best way to fix this is, as the error message without this "early fail" isn't the most intuitive one.

Once the other hand this only checks for atomics but not for the other two required options :thinking:, so there is a good chance people will run into this error anyway.

(What I do for now is to explicitly exclude the wasm ffi crate and run a separate clippy check on it.)

rustonaut avatar Aug 19 '21 16:08 rustonaut

The problem with this is that it makes thinks like clippy check --all-features --all-targets not usable (without some hacks), and in turn complicates dev and CI workflows of projects/workspaces for which wasm is just one of multiple targets.

If wasm is just one of multiple targets, chances are you're already using cfg(target_arch = "wasm32") in some places to exclude Wasm-specific dependencies. When you do that, Clippy issues should go away naturally.

OTOH, if it is a Wasm-specific code path, then it's best to add .cargo/config as per README, and run Clippy with --target wasm32-unknown-unknown, which should work correctly.

RReverser avatar Aug 19 '21 19:08 RReverser

Thanks for the response.

exclude Wasm-specific dependencies

The problem is not that the wrong files are included (we made sure they are not) but that clippy checking everything including wasm fails.

The options to make clippy not fail for wasm will make it fail for other targets.

.cargo/config seems like a way to set the options only for the right target, but in practice this doesn't work due to two reasons:

  • ~How clippy handles the --all-targets flag conflicts with build-std leading to following error: error: -Zbuild-std requires --target (I will look into that and potentially open a issue with clippy if neccessary).~ (EDIT: I mixed up build targets and platform targets, sorry for that)
    • There is also the thing that the build-std option is not scoped to WASM which is a problem.
    • EDIT: you always need to supply the --target parameter manually which is sub-optimal
  • I can't really use .cargo/config in my case, as we target both wasm32 with atomics and wasm32 without.

So what I did was:

In the workspace root set: resolver="2"

And then in the ffi-wasm crate I included:

[target.'cfg(all(target_arch="wasm32", target_feature = "atomics"))'.dependencies]
wasm-bindgen-rayon = {  version = "1.0.3",  features = ["no-bundler"] }
non-target-specific-core = { path = "../non-target-specific-core", features=["parallel"] }

[target.'cfg(not(all(target_arch="wasm32", target_feature = "atomics")))'.dependencies]
non-target-specific-core = { path = "../non-target-specific-core" }

As well as some small changes in ffi-wasm.

Due to how we split the code base into non-target-specific core parts and targets specific FFI bindings this works rather well.

rustonaut avatar Aug 20 '21 14:08 rustonaut

Is it ok if I make a doc-PR adding a section which hints at target.'cfg' + resolver="2" being useful?|

I also would close the issue with that. Given that people (should) search closed issues too when they have a problem, there would be little reason to keep it open once the documentation mentioned the less often used [target.'cfg'] and resolver="2" options (which are not specific to this project but are something everyone who wants to e.g. optionally support parallelism likely needs).

rustonaut avatar Aug 23 '21 01:08 rustonaut

I guess a doc PR would be good, but OTOH I'm a bit worried that docs are already quite overwhelming and this seems like one of more niche use-cases...

I'd say we can keep this issue open for now and see if anyone else runs into the same problem; if not, perhaps we'll just be able to close it and over time I'm hoping resolver="2" will become the default for new projects on Cargo side.

RReverser avatar Sep 07 '21 15:09 RReverser

resolver="2" is the new default in the new edition.

maxammann avatar Mar 22 '22 13:03 maxammann

you could set rustflags to use with clippy in .cargo/config (we do this so that a certain set of clippy rules is used):

[target.'cfg(feature = "cargo-clippy")']
rustflags = [
  "-C", "target-feature=+atomics,+bulk-memory,+mutable-globals"
  "-Aclippy::all", .. your favourite clippy include / exclude list here ... 
]

gilescope avatar Apr 18 '22 16:04 gilescope

I guess I'll close this since resolver=2 has been the default for some time now and, in my understanding, avoids the related issue.

RReverser avatar May 31 '23 14:05 RReverser