search

rust-secure-code / cargo-auditable

Make production Rust binaries auditable
Apache License 2.0
646 stars 28 forks source link

Run wasm tests if wasm feature is enabled #148

Closed orhun closed 5 months ago

orhun commented 5 months ago

Hey, Arch packager of cargo-auditable here! :wave:

I was updating the package to 0.6.3 and realized the wasm tests are being run regardless of wasm feature is enabled or not.

This PR simply disables test_wasm if wasm feature is not enabled.

Let me know if this is a change that you want to make / if this needs any additional changes!

Cheers.

Shnatsel commented 5 months ago

Good catch! Thanks!

Shnatsel commented 5 months ago

Upon closer inspection, the extraction crates like auditable-extract do have wasm as an optional feature, but cargo auditable does not. So this change would always disable the test, which is a non-starter.

orhun commented 5 months ago

Added the feature in c7e77b17b4f2912ab9c79960b04ab3a6d57dfc6f, not sure if it should depend on anything. LMK!

Shnatsel commented 5 months ago

It would depend on wasm-gen, but I don't want to feature-gate this functionality. I don't really see the benefit of doing that, and it would complicate testing considerably because I'd have to test both configurations (feature enabled and disabled) as well as add more code to handle both cases.

Why are you looking to feature-gate it in the first place? Is it that the toolchain used for the build doesn't have the wasm32-unknown-unknown target installed, and that causes tests to fail?

orhun commented 5 months ago

Yes, the toolchain that I'm using inside a clean chroot does not have the target installed (since we are building with cargo directly, instead of rustup) - that is why this particular test was failing. Right now I'm skipping it but thought it would be nice to make it disabled as default. Maybe renaming the feature to wasm-tests would be better?

Shnatsel commented 5 months ago

I am reluctant to expose it as a feature because this is something that the end users would never have to touch, so there's no reason to expose it publicly.

I could add a #[cfg(not(disable_wasm_tests))] to the code, but that would be functionally identical to a distribution-level patch to skip the test, because you still need to set a distribution-level cfg. So I'd rather keep the code as-is, since every option requires an equal amount of hassle for the packagers.

We can revisit this in the future if we grow a large suite of WASM tests, at which point disabling them all with a single knob would be useful.

Shnatsel commented 5 months ago

By the way, I'm hoping to release a new version of auditable-extract with fewer dependencies in the next few days, once https://github.com/bytecodealliance/wasm-tools/issues/1528 is implemented.

I'm not sure if having fewer dependencies will make packaging easier for you or not, but I figured I'd mention it.