Allow rustup component add to install multiple components in one update#4790
Allow rustup component add to install multiple components in one update#4790Inconnu08 wants to merge 1 commit intorust-lang:mainfrom
Conversation
There was a problem hiding this comment.
LGTM, thank you!
Small note: please avoid referencing issues (or PRs) directly in commit messages as this can generate unwanted notification spam. This is mentioned in the antepenultimate paragraph of the rustc-dev-guide.
src/cli/rustup_mode.rs
Outdated
| ); | ||
| distributable.add_component(new_component).await?; | ||
| } | ||
| let new_components = targets |
There was a problem hiding this comment.
Nit: suggest to avoid binding this, and just passing it directly to add_components().
There was a problem hiding this comment.
... and in this case you don't need to specify the Vec<_> type, I think,
There was a problem hiding this comment.
Ah, yes, thank you. I inlined the collection and removed the explicit Vec<_> type.
src/toolchain/distributable.rs
Outdated
| .get(&self.desc.target) | ||
| .expect("installed manifest should have a known target"); | ||
|
|
||
| if !targ_pkg.components.contains(&component) { |
There was a problem hiding this comment.
Kind of pre-existing, but please invert this condition to continue early, reducing rightward drift.
There was a problem hiding this comment.
Good point, I refactored this section to reduce nesting and improve readability
There was a problem hiding this comment.
I also hoisted the manifest target lookup out of the loop since it doesn’t depend on the individual component...
Inconnu08
force-pushed
the
component-add-multi-install
branch
2 times, most recently
from
647d3af to
20f047f
Compare
Inconnu08
force-pushed
the
component-add-multi-install
branch
from
20f047f to
3c7796a
Compare
4 participants
This updates
rustup component addto handle multiple components in asingle operation instead of processing them sequentially.
Closes #4787
Related to #731