CLI should not depend on rustup #1996
Conversation
mamcx
force-pushed
the
mamcx/no_rustup_cli
branch
from
5918c82
to
0156bba
Compare
| use super::*; | ||
|
|
||
| #[test] | ||
| fn test_has_target() -> anyhow::Result<()> { |
There was a problem hiding this comment.
It would be cool if we could, like, configure a system here with both configurations and check, but that's not really feasible in a Rust test. I think this test is ok as-is, assuming we have wasm32 on CI.
| use std::path::Path; | ||
|
|
||
| /// Find an executable in the `PATH`. | ||
| pub(crate) fn find_executable(exe_name: impl AsRef<Path>) -> Option<std::path::PathBuf> { |
There was a problem hiding this comment.
There's also https://github.com/harryfei/which-rs/, but it seems to mostly do stuff we don't need, so I think this is fine for now.
mamcx
force-pushed
the
mamcx/no_rustup_cli
branch
from
0156bba
to
f820488
Compare
| println!("Warning: Failed to install rustfmt: {err}"); | ||
| } | ||
| } else { | ||
| return Err(anyhow::anyhow!("rustfmt is not installed. Please install it.")); |
There was a problem hiding this comment.
it's a bit strange to me that if we fail to install rustfmt above, we just print a warning, but if we don't have rustup at all then we return an error. Should we print a warning in this case too, or should we return an Err above?
| println!("Warning: Failed to install wasm32-unknown-unknown target: {}", err); | ||
| } | ||
| } else { | ||
| anyhow::bail!("wasm32-unknown-unknown target is not installed. Please install it."); |
There was a problem hiding this comment.
some comment here as above - strange that we bail if rustup is missing, but only warn if rustup fails to install the target.
|
Looks basically good to me, two small comments |
mamcx
force-pushed
the
mamcx/no_rustup_cli
branch
from
39bcb5f
to
d22b6f9
Compare
Description of Changes
Rustup could not be installed (for example, for people who use
nix).Now, the
clidefensively check for it. It also try to find the targetwasm32-unknown-unknownin case is not installed.Closes #1971.
API and ABI breaking changes
If this is an API or ABI breaking change, please apply the
corresponding GitHub label.
Expected complexity level and risk
1
Testing
Describe any testing you've done, and any testing you'd like your reviewers to do,
so that you're confident that all the changes work as expected!