Uh oh!
There was an error while loading. Please reload this page.
Installation profiles - #1673
Merged
Merged
Conversation
kinnison
reviewed
Mar 3, 2019
kinnison
left a comment
Contributor
There was a problem hiding this comment.
A very lightweight review, just to get started getting a feel for the change. Hope my comments are useful.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bors
commented
Mar 5, 2019
Contributor
☔ The latest upstream changes (presumably #1616) made this pull request unmergeable. Please resolve the merge conflicts. |
nrc
commented
May 13, 2019
MemberAuthor
Status update: this branch is updated to master of last week, review comments have been addressed, there a few tests, and most existing tests pass. |
kinnisonforce-pushed
the
profiles
branch
4 times, most recently
from
May 26, 2019 16:19
987d849 to
36941f1Comparekinnison
previously requested changes
May 27, 2019
kinnison
left a comment
Contributor
There was a problem hiding this comment.
We need to fix where we've lost error message content, and decide what to do about the other two points.
| // Non-required components that were required before profiles exist. | ||
| pub fn legacy_profile() -> Vec<String> { | ||
| ["rust-std", "rustc", "cargo", "rust-docs"] |
Contributor
There was a problem hiding this comment.
This does not match that the profiles in the now-released channels contain rust-mingw - what are we to do about that?
MemberAuthor
There was a problem hiding this comment.
I think we should add rust-mingw here (it should match what we're doing now by default). We might need to handle it not being present on some platforms, but I don't think that will cause big problems.
Contributor
There was a problem hiding this comment.
Okay, this means that we also need to decide what we're actually going to need to do regarding rust-mingw in the real world. Currently we fail to install any toolchain other than a mingw one because that component is only available for the mingw targets.
| if update.nothing_changes() { | ||
| // TODO changes and ,manifest are empty? | ||
| eprintln!("update: {:?}", update); |
Contributor
There was a problem hiding this comment.
We need to make a decision about what's going on here
MemberAuthor
There was a problem hiding this comment.
I think this is just a stray TODO and println from my debugging and can be deleted
MemberAuthor
There was a problem hiding this comment.
unless we're hitting it in a test which is still failing
Contributor
There was a problem hiding this comment.
Okay we're not hitting it so we can remove this. Currently the test suite passes, though I expect that we need to add some more profile-specific tests.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
kinnison
commented
May 27, 2019
Contributor
@nrc I need your input on the remaining two comments above. One of them (the |
nrc
commented
Jun 6, 2019
MemberAuthor
We discussed this in last weeks meeting (and I really should have written these notes sooner, sorry). We need to handle rust-mingw. It is part of the current set of default components (on Windows only). The best path forward is to add it to the default profile (or should it be minimal), and add a section to the manifest for platform-specifics (probably a table where any component can list the platforms it supports (default is all)). If that table is missing rustup should treat the manifest as if there were no profiles section. We noted that any missing component should cause update to fail (unless the user supplies We also noted that we should remove the We need to decide on what tests we should have to ensure the profiles feature is well-tested. I'm away this weekend, I hope to have some time for this next week. |
nrc
commented
Jun 12, 2019
MemberAuthor
So it turns out we don't need to add anything to the manifest. There is a list of components for the |
nrcforce-pushed
the
profiles
branch
2 times, most recently
from
June 15, 2019 05:20
feb1222 to
13738f0CompareMemberAuthor
TestsWe should test the following. Note that some of these might have tests in this PR already, or might be tested by existing tests. Each item may be one or more tests, or possibly multiple items can be tested by a single test. The first useful task is seeing if a test is still needed. The second useful task is implementing the remaining tests. Install
Update
Update with missing components
Install with missing components
Add component
Remove component
Add a component to a profile
show profile
set profile
|
bors
commented
Jun 24, 2019
Contributor
☔ The latest upstream changes (presumably #1890) made this pull request unmergeable. Please resolve the merge conflicts. |
kinnison
commented
Jun 24, 2019
Contributor
Since I merged the thing which broke the merge, I've pushed a rebase for you. |
nrc
commented
Jul 2, 2019
MemberAuthor
I ticked a few tests off based on what is already implemented in the PR. We also test 'set profile' with an invalid name. |
kinnison
commented
Jul 2, 2019
Contributor
Cool. I have hope that one day soon I'll have time to add some more tests. |
bors
commented
Jul 3, 2019
Contributor
☔ The latest upstream changes (presumably #1931) made this pull request unmergeable. Please resolve the merge conflicts. |
nrc
commented
Sep 4, 2019
MemberAuthor
I do not like the word 'fascinating' associated with test failures :-(
Sigh. Should be fixed now; I'm an idiot. |
kinnison
commented
Sep 5, 2019
Contributor
I think it was to do with the macro changes.
Yay. Merge looks clean, I'll do a proper review next. D. |
kinnison
suggested changes
Sep 5, 2019
kinnison
left a comment
Contributor
There was a problem hiding this comment.
A few bogons, and a few nits. Otherwise this looks like a good simplification of your previous work. nice!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Signed-off-by: Nick Cameron <nrc@ncameron.org>
nrc
commented
Sep 10, 2019
MemberAuthor
@kinnison I think all comments are addressed |
kinnison
left a comment
Contributor
There was a problem hiding this comment.
This looks good enough that I'm good to merge once it's squashed. I can do that if you're too busy.
nrc
commented
Sep 11, 2019
MemberAuthor
Let's let GH do it - 'squash and merge' ftw |
softprops
commented
Sep 12, 2019
This is awesome! How soon can we expect a new release? |
kinnison
commented
Sep 12, 2019
Contributor
Github does squash not fixup, so the commit message is sucky, but yay, merged! 🎉 |
kinnison
commented
Sep 12, 2019
Contributor
We will want this to be tested for a while before we make a release with it. I don't expect for a couple of weeks at least. |
Arnavion
commented
Sep 12, 2019
I just tried it with the minimal profile on an x86_64 Linux host and it worked wonderfully. Thanks! ( It might be useful to have a subcommand like $ curl -Ls 'https://static.rust-lang.org/dist/2019-09-12/channel-rust-nightly.toml'| grep -A5 '\[profiles\]'
[profiles]
complete = ["rustc", "cargo", "rust-std", "rust-mingw", "rust-docs", "rustfmt-preview", "clippy-preview", "rls-preview", "rust-src", "llvm-tools-preview", "lldb-preview", "rust-analysis", "miri-preview"]
default = ["rustc", "cargo", "rust-std", "rust-mingw", "rust-docs", "rustfmt-preview", "clippy-preview"]
minimal = ["rustc", "cargo", "rust-std", "rust-mingw"] |
|
Hi. This commit broke the ability to install a specific nightly toolchain, apparently: How it looks like when it works: (yeah I stopped it, for testing purposes) And how it looks like when it doesn't work(exit code is 0 btw): (the warnings are irrelevant btw, I can get rid of them with no effects on the issue) Oh, credits for telling me which was the good version/commit go to |
kinnison
commented
Oct 8, 2019
Contributor
I have no idea how @howaboutsynergy got that, because: somniloquy% rustup toolchain install nightly-2019-09-05 info: syncing channel updates for'nightly-2019-09-05-x86_64-unknown-linux-gnu'
info: latest update on 2019-09-05, rust version 1.39.0-nightly (c6e9c76c5 2019-09-04)
info: downloading component 'cargo'
info: downloading component 'clippy'
info: downloading component 'rust-docs'
11.7 MiB / 11.7 MiB (100 %) 7.4 MiB/s in 1s ETA: 0s
info: downloading component 'rust-std'
175.3 MiB / 175.3 MiB (100 %) 7.5 MiB/s in 23s ETA: 0s
info: downloading component 'rustc'
66.1 MiB / 66.1 MiB (100 %) 7.7 MiB/s in 8s ETA: 0s
info: downloading component 'rustfmt'
info: installing component 'cargo'
info: installing component 'clippy'
info: installing component 'rust-docs'
11.7 MiB / 11.7 MiB (100 %) 2.8 MiB/s in 3s ETA: 0s
info: installing component 'rust-std'
175.3 MiB / 175.3 MiB (100 %) 16.3 MiB/s in 11s ETA: 0s
info: installing component 'rustc'
66.1 MiB / 66.1 MiB (100 %) 7.5 MiB/s in 8s ETA: 0s
info: installing component 'rustfmt'
nightly-2019-09-05-x86_64-unknown-linux-gnu installed - rustc 1.39.0-nightly (c6e9c76c5 2019-09-04)
info: checking for self-updates
rustup toolchain install nightly-2019-09-05 86.77s user 2.59s system 142% cpu 1:02.84 total
somniloquy% rustup --version rustup 1.19.0+64 (cba68ab58 2019-10-08)
somniloquy% |
Well I'll be. I wonder if something else I've modified influenced it: Oh and if I'm outside the override dir, it's: My
default_host_triple = "x86_64-unknown-linux-gnu"default_toolchain = "master-installed"version = "12"
[overrides]Also note: I can repro. with latest git master: no tests failed for the bad commit: Details |
@kinnison do you have a |
kinnison
commented
Oct 8, 2019
Contributor
I do have a profile set. I'm surprised you don't, but perhaps if you've not installed the rustup you built you might not have one set. Mine is set to |
ghost
commented
Oct 8, 2019
That would explain why it doesn't work for me. Thanks. I install my rustup.rs via The |
kinnison
commented
Oct 8, 2019
Contributor
Gosh that's complex. I'd change that for But you're right, we probably should detect absence of |
ghost
commented
Oct 8, 2019
but some migration(of existing toolchains?) might be needed, because if I simply (makes sense it's read only) the breaking is this: the toolschains seem fine. Hmm... Maybe I was wrong... since the breakage is only for but it works fine for this one hmm:
now I see what you meant by installed, not Then there's no issue here, sorry for the noise! The only remaining problem/difference then is this:with rustup commit: ec6d1af but with the prev. rustup commit aka 039a0a8 |
kinnison
commented
Oct 8, 2019
Contributor
Yep, the default profile includes rustfmt which isn't available on that nightly version. You should either set profile to |
ghost
commented
Oct 8, 2019
Right-O, thanks for your time&help @kinnison |
ghost
commented
Oct 8, 2019
+ // Returns a profile, if one exists in the settings file.+ //+ // Returns `Err` if the settings file could not be read or the profile is+ // invalid. Returns `Ok(Some(...))` if there is a valid profile, and `Ok(None)`+ // if there is no profile in the settings file. The last variant happens when+ // a user upgrades from a version of Rustup without profiles to a version of+ // Rustup with profiles.+ pub fn get_profile(&self) -> Result<Option<dist::Profile>> {+ self.settings_file.with(|s| {+ let p = match &s.profile {+ Some(p) => p,+ None => return Ok(None),+ };+ let p = dist::Profile::from_str(p)?;+ Ok(Some(p))+ })+ }According to both the above, it means that I wouldn't be getting a DetailsI'm trying to figure out why the but not in this case: but strictly in the source code I mean. @@ -86,6 +87,7 @@ pub fn main() -> Result<()> {
},
("set", Some(c)) => match c.subcommand() {
("default-host", Some(m)) => set_default_host_triple(&cfg, m)?,
+ ("profile", Some(m)) => set_profile(&cfg, m)?,
(_, _) => unreachable!(),
},
("completions", Some(c)) => {+fn set_profile(cfg: &Cfg, m: &ArgMatches) -> Result<()> {+ cfg.set_profile(&m.value_of("profile-name").unwrap())?;+ Ok(())+}+ah well this explains it: @@ -501,6 +504,16 @@ pub fn cli() -> App<'static, 'static> {
SubCommand::with_name("default-host")
.about("The triple used to identify toolchains when not specified")
.arg(Arg::with_name("host_triple").required(true)),
+ )+ .subcommand(+ SubCommand::with_name("profile")+ .about("The default components installed")+ .arg(+ Arg::with_name("profile-name")+ .required(true)+ .possible_values(Profile::names())+ .default_value(Profile::default_name()),+ ),
),
);
yup, it's set there as default! I'm going to just use a noobish hack (so I don't get bit by this again) until an actual dev does do that mentioned TODO, for my knowledge is limited xD: diff --git a/src/dist/dist.rs b/src/dist/dist.rs
index 09cfb2f9..9ea41cf6 100644
--- a/src/dist/dist.rs+++ b/src/dist/dist.rs@@ -765,7 +765,11 @@ fn try_update_from_dist_<'a>(
let profile_components = match profile {
Some(profile) => m.get_profile_components(profile, &toolchain.target)?,
- None => Vec::new(),+ None => {+ eprintln!("Profile not set in ~/.rustup/settings.toml try running `rustup set profile minimal` or `default`. Assuming `minimal`.");+ m.get_profile_components(Profile::Minimal, &toolchain.target)?+ },+ //Vec::new(),
};
use crate::dist::manifest::Component;result: good enough for my... |
kinnison
commented
Oct 8, 2019
Contributor
If you could file an issue about the upgrade case I'd appreciate it. |
ghost
commented
Oct 8, 2019
Done #2045 Thanks. |
fnichol added a commit
to fnichol/dotvim
that referenced
this pull request
Oct 16, 2019
Due to some updates in the Installation Profiles PR in Rustup (rust-lang/rustup#1673), as of Rustup 1.20.0, there is no longer a ` (default)` ending indicator on the active target when calling `rustup target list`. Looking at the update, it's not totally clear that this was ever completely accurate so another strategy was in order. This change, uses the knowledge of the current active toolchain (via `rustup show active-toolchain`) to list all installed components for that toolchain. The `rustc` component should be installed in an active toolchain (although this is honestly a stated assumption which could be invalidated), so if we strip off `rustc-` from the start of the component name, we should be left with the full target triple of the active toolchain, hence the "default active target". Signed-off-by: Fletcher Nichol <fnichol@nichol.ca>
AJ-Ianozi pushed a commit
to AJ-Ianozi/getada-download
that referenced
this pull request
Oct 12, 2023
* Add profile argument to rustup install * Address some warnings Signed-off-by: Nick Cameron <nrc@ncameron.org> * Review changes Signed-off-by: Nick Cameron <nrc@ncameron.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.