refactor(toolchain)!: remove multiple components at once - #5080
Conversation
8d6b848 to
ad53fac
Compare
ad53fac to
57d1d7d
Compare
There was a problem hiding this comment.
I think this direction is okay? But I'd like smaller commits, and I'd probably move DistributableToolchain::remove_components() out of DistributableToolchain into rustup_mode as a first step.
If we're going to pass an IntoIterator<Item = Component> into it, then it doesn't really make sense to me in terms of program flow (as opposed to performance) to allocate a Vec for it upfront, then iterate over it to check some stuff, then transform the Vec into an iterator again.
So I'd like to challenge you to rethink this code a bit more than you're currently doing.
@djc I'm not sure to have fully understood your suggestion here. As it currently stands, What I'm doing in the current patch is really making the underlying implementation accept multiple components at once, so that the dispatching functions don't need to call it in a loop. If you agree with that, then in terms of argument parsing and history rewriting of course I can reevaluate my decisions. |
57d1d7d to
d1d66a8
Compare
This comment has been minimized.
This comment has been minimized.
d1d66a8 to
77abbd6
Compare
rami3l
changed the title
77abbd6 to
50cdc5e
Compare
@djc But looking back, what you have pointed out here makes a lot of sense actually. I should have tucked the allocation into the |
85346b2 to
9020f9b
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
9020f9b to
e229af3
Compare
The current version only checks whether we are removing target when we have at most one target installed. Unfortunately, the target to be removed don't necessary have to be the last one installed. Instead, we should simulate the remaining targets after the removal and check whether we have nothing left.
e229af3 to
367c94a
Compare
| if targets.contains(&cfg.default_host_tuple()?) { | ||
| warn!( | ||
| "removing the default host target; proc-macros and build scripts might no longer build" | ||
| ); |
There was a problem hiding this comment.
Nit: Shouldn't we print this after confirming the removal succeeded? I know the phrasing is "removing" and not "removed", but still, a command like rustup target rm <host> non-existent would give the warning and fail without removing anything.
Maybe we could move this to after the actual removal?
There was a problem hiding this comment.
Oops, I was looking into the code and did not see that this was already queued. Apologies :)
There was a problem hiding this comment.
Nit: Shouldn't we print this after confirming the removal succeeded? I know the phrasing is "removing" and not "removed", but still, a command like
rustup target rm <host> non-existentwould give the warning and fail without removing anything.
@FranciscoTGouveia Thanks for the feedback!
I'd like to clarify that since #4797, rustup target rm <host> non-existent will cause <host> to be removed, only AFTER which the warning about non-existent will surface (same thing even if you put non-existent before <host>, if you are thinking about that), so we are quite unlikely to encounter false positives here merely due to user input. If the command execution really fails in the middle, I would still like to warn the user at first.
4 participants
Part of rust-lang/goals#736.
Currently, a new transaction is set up when removing every component, which is clearly unnecessary.
This PR addresses that issue by removing multiple components at once.