Remove cargo dev fmt reliance on rustup

[DylanBulfin]

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: none

I hope PRs like this are welcome, it's a small update to the cargo dev fmt subcommand. I noticed that it uses rustup which rustfmt to get the full command path, and fails if this doesn't work. I think it would be useful to fall back to which or where if someone is using something other than rustup for toolchain management.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Jarcho (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[samueltardieu]

I thought using rustup here was used to get the "right" rustfmt, i.e. the one corresponding to the active toolchain used with Clippy. In this case, it makes sense to use the full path to rustfmt.

If this fails, why not just use rustfmt (which will search $PATH when spawning the command) instead of its full path? which rustfmt will just return the result of searching $PATH, we could let Command::new() do that for us, just like it finds rustup and which.

[DylanBulfin]

I thought using rustup here was used to get the "right" rustfmt, i.e. the one corresponding to the active toolchain used with Clippy. In this case, it makes sense to use the full path to rustfmt.

If this fails, why not just use rustfmt (which will search $PATH when spawning the command) instead of its full path? which rustfmt will just return the result of searching $PATH, we could let Command::new() do that for us, just like it finds rustup and which.

So it's for if the rustfmt in PATH doesn't match the one we want to use? That makes sense. Good point about which/where, I've simplified the code to just fall back to "rustfmt" as the path. Thanks!

[flip1995]

The reason for using rustup which rustfmt is to get the correct rustfmt version. I never had issues with that e.g. in my IDE, but I think that's because I'm using rustup for toolchain management.

What are you using for Rust toolchain mangement? Using rustup is stongly recommended in the Clippy repo, as the toolchain updates every 2 weeks and is then the only toolchain that will just work.

[DylanBulfin]

The reason for using rustup which rustfmt is to get the correct rustfmt version. I never had issues with that e.g. in my IDE, but I think that's because I'm using rustup for toolchain management.

What are you using for Rust toolchain mangement? Using rustup is stongly recommended in the Clippy repo, as the toolchain updates every 2 weeks and is then the only toolchain that will just work.

I was using the fenix overlay for NixOS, which does read from the same toolchain file but doesn't install the rustup binary or use it for management. I realize that a lot of dev tools are inherently dependent on rustup but it seems like it would make sense to support other configurations when possible.

It still uses rustup which first so it shouldn't affect anyone who does use rustup, but since this subcommand is automatically run when testing it would be nice to make it more universal.

I found the NO_FMT_TEST env variable as well so if you'd rather keep this method the way it is I can just add some documentation about that somewhere, to help anyone running into the same problem as me.

[Jarcho]

I have no issue with falling back to rustfmt locally. You might end up with a version that doesn't match the build toolchain, but it's usually not an issue even when that happens unless you get a very outdated version. Having the fmt test pass is a little iffy since it may not pass in CI. Having a visible warning is probably enough to prevent any real problems.

CI, however, should definitely not have a fallback. Doing so has the possibility of having a silent error that blows up at some point in the future.

[DylanBulfin]

I have no issue with falling back to rustfmt locally. You might end up with a version that doesn't match the build toolchain, but it's usually not an issue even when that happens unless you get a very outdated version. Having the fmt test pass is a little iffy since it may not pass in CI. Having a visible warning is probably enough to prevent any real problems.

CI, however, should definitely not have a fallback. Doing so has the possibility of having a silent error that blows up at some point in the future.

Good point, I wasn't thinking about that. I ended up having to switch to rustup anyway to get compiler internal completion working so I think I was hasty in making this. Closing now.