Skip to content

fix: allow check-rust to complete without cargo hack#2588

Open
hippietrail wants to merge 6 commits intoAutomattic:masterfrom
hippietrail:warn-instead-of-fail-without-cargo-hack
Open

fix: allow check-rust to complete without cargo hack#2588
hippietrail wants to merge 6 commits intoAutomattic:masterfrom
hippietrail:warn-instead-of-fail-without-cargo-hack

Conversation

@hippietrail
Copy link
Collaborator

Issues

N/A

Description

I noticed lately that when I do just precommit that the test comes to an abrupt halt after doing all kinds of heavy testing with a dozen Chromium windows open at once etc. just to tell me I don't have cargo hack installed.

This PR will instead issue a warning at that point but continue the testing.

instead of failing at this point it will issue the warning

⚠️  cargo-hack not found. Install with 'cargo install cargo-hack' for complete feature testing.

I'm not 100% clear if this is acceptable or not though. Another way would be to check if it's installed at the start of the test so that you don't waste time and system resources only for the test to fail to complete.

I'm not a shell expert and the LLM's first method of checking for the presence of cargo hack didn't actually work - so please check carefully!

How Has This Been Tested?

Manually.

Checklist

  • I have performed a self-review of my own code
  • I have added tests to cover my changes

instead of failing at this point it will issue the warning
```
⚠️  cargo-hack not found. Install with 'cargo install cargo-hack' for complete feature testing.
```
@elijah-potter
Copy link
Collaborator

I would prefer this to work similarly to how we have it set up with wasm-opt. Right now, if you wish to skip the wasm-opt step (if you do not wish to have it installed, or if you are impatient like me), you can simply export DISABLE_WASM_OPT in your .bashrc. I think if you don't want to have cargo-hack installed, we should have a similar environment variable, like DISABLE_CARGO_HACK.

@hippietrail
Copy link
Collaborator Author

I would prefer this to work similarly to how we have it set up with wasm-opt. Right now, if you wish to skip the wasm-opt step (if you do not wish to have it installed, or if you are impatient like me), you can simply export DISABLE_WASM_OPT in your .bashrc. I think if you don't want to have cargo-hack installed, we should have a similar environment variable, like DISABLE_CARGO_HACK.

Is like this OK or would you prefer without the warning since DISABLE_WASM_OPT doesn't have one?

@elijah-potter
Copy link
Collaborator

My main concern is that I don't want CI to fail silently if the cargo hack set up breaks. Would you mind making sure that the command will fail if cargo hack is missing and DISABLE_CARGO_HACK is off?

…cified

Make `check-rust` fail if `cargo-hack` is missing unless `DISABLE_CARGO_HACK=1`
is set, following the same pattern as `DISABLE_WASM_OPT`.
@hippietrail
Copy link
Collaborator Author

I would prefer this to work similarly to how we have it set up with wasm-opt. Right now, if you wish to skip the wasm-opt step (if you do not wish to have it installed, or if you are impatient like me), you can simply export DISABLE_WASM_OPT in your .bashrc. I think if you don't want to have cargo-hack installed, we should have a similar environment variable, like DISABLE_CARGO_HACK.

Is like this OK or would you prefer without the warning since DISABLE_WASM_OPT doesn't have one?

Like this right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants