Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

install: add --profile flag to override profile #2075

Merged
merged 1 commit into from
Oct 20, 2019

Conversation

jonhoo
Copy link
Contributor

@jonhoo jonhoo commented Oct 19, 2019

Previously, the profile to use when installing a new toolchain had to be
set globally through rustup set profile, or at rustup-init time.
However, sometimes this is inconvenient, because you only want to use a
particular profile for a specific toolchain. This patch allows users to
pass --profile to rustup install to override the profile used for
that one install.

Fixes #2004.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for tackling this issue. There are a few small problems (such as not implementing the command actually asked for in the issue -- rustup toolchain install --profile...) and a big problem which is that I'm not comfortable with the big Clone change on Cfg and temp::Cfg

I'd rather see this implemented by passing through an Optiondist::Profile to the installation method functions. I appreciate this results in collateral None arguments in a bunch of places, but over-all I think it's more in keeping with the current codebase. Perhaps in the future we'll have some more live data held in Cfg (I was considering it for another PR but now I feel iffy about that)

I'm open to being convinced I should stop worrying about Cfg in that way, at which point I'd rather update() took &mut Cfg rather than all that Clone mess though, but in the meantime I knocked up a diff for the pass-argument approach on my fork if you wanted to take a look at that approach.

src/cli/rustup_mode.rs Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/dist/temp.rs Outdated Show resolved Hide resolved
tests/cli-misc.rs Outdated Show resolved Hide resolved
tests/cli-misc.rs Outdated Show resolved Hide resolved
tests/cli-misc.rs Outdated Show resolved Hide resolved
@kinnison
Copy link
Contributor

The branch mentioned in the review is at: https://github.com/kinnison/rustup.rs/tree/kinnison/profile-on-install

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 19, 2019

Yup, as you saw in the stream I was really unsure which approach would be better. I think it'd be handy to have a distinction between "active config" and "on-disk config", but agree that this PR is probably not the place to make that change. Would be happy to use your branch instead of mine -- they both fix the issue. My only comment on your version is that it's weird for profile to be the last argument in common::update_all_channels and Cfg::update_all_channels, but the second in install_from_dist. Oh, and that there's some weird formatting and lack of visual separation going on in the test. Happy to close this in favor of your change!

@kinnison
Copy link
Contributor

I think there may be a happy medium to be had here. I'm not entirely comfortable with the clone and would rather just have an override profile option which we can set via a method taking &mut Self on Cfg -- that'd remove the noisy commit messing with temp::Cfg.

If you then moved the test into cli-v2 where I really think it belongs, remove the other bogons, then we can start a principle of permitting ephemeral configuration data in Cfg on the basis that as soon as it gets beyond somewhere passing it &mut, you can no longer change it, it's frozen.

I'd rather we amended your PR than replaced it with my hacked up change (the inconsistency in argument position is in part due to me hacking on it while also watching what you did) since I do think having the precedent set now might help with some work I want to do with Ben Chen.

@jonhoo
Copy link
Contributor Author

jonhoo commented Oct 19, 2019

Sure thing! Here's a variant with all comments addressed, where we keep &mut Cfg in the "outer" layers of the application.

Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The over-all delta looks much better now. A few more comments and if you agree with them and make the change, you may as well rebase into a nicer set of commits ready for merge.

src/config.rs Outdated Show resolved Hide resolved
tests/cli-v2.rs Outdated Show resolved Hide resolved
tests/cli-v2.rs Outdated Show resolved Hide resolved
@kinnison
Copy link
Contributor

I am slightly confused as to how the test case failing in your CI runs is failing given your change though.

@kinnison
Copy link
Contributor

I worked it out and thanks to you permitting commits from maintainers, I've pushed a fix to your branch. If you're okay with it, and it passes the tests, and you resolve the other comments, then fold it into your commits, no need to try and credit me.

@bors
Copy link
Contributor

bors commented Oct 20, 2019

☔ The latest upstream changes (presumably #2074) made this pull request unmergeable. Please resolve the merge conflicts.

Previously, the profile to use when installing a new toolchain had to be
set globally through `rustup set profile`, or at rustup-init time.
Sometimes this is inconvenient, because you only want to use a
particular profile when installing a specific toolchain. This patch
allows users to pass `--profile` to `rustup install` to override the
profile used for that one install.

Fixes rust-lang#2004.
Copy link
Contributor

@kinnison kinnison left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Once the CI completes, I'll merge.

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.

Support selecting a profile at toolchain installation time
3 participants