-
Notifications
You must be signed in to change notification settings - Fork 877
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
rustup_mode: add toolchain install --allow-downgrade option #2126
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial feedback.
a9b2325
to
db96bf9
Compare
The failed CI run ended with the message:
I consider this flakyness of the underlying system, probably some filesystem hickup. I don't know if it is probably related to code in rustup set default-host which could be more robust against yet uncertain filesystem races. @kinnison what do you think of it? |
@payload Yes that's a transient issue I only ever tend to see on Travis. As such I've not managed to track it down to where it actually happens to add a retry in for that case. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the test, I'd be tempted to add the --allow-downgrade
variant to the end of the longer test, so that the scenario plays out in full as:
- Install
2 Check for rls -- should not be present - Attempt to add rls -- this should fail
- Attempt to add rls with downgrate permitted -- this should succeed
- Check for rls -- should be present
- Check hash for rustc -- should have downgraded
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty close to mergeable -- some more test ideas. Nice job 👍
"component 'rls' for target '{}' is unavailable for download for channel nightly", | ||
trip, | ||
), | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps here add a check that the rls
component is not installed You can do that with:
expect_component_not_executable(config, "rls");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I added it I would rather see that in a different testcase which checks if toolchain install does not change anything without allow-downgrade. When I look at the subcommand toolchain install I see 7 arguments and would expect at least as many as 7 tests at the level of cli-v2.rs, but there are not. Or else I don't know yet about all the test levels. Well that's obvious :)
"nightly", | ||
"--no-self-update", | ||
], | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After this, I'd add an expect to check the rustc version is the version we expect (i.e. one without rls)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I add that. It is though like a sanity check to find out if our test setup is based on the right assumptions.
"--allow-downgrade", | ||
], | ||
); | ||
expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-2"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is useful to check that it did downgrade, that'd be clearer if you'd done the equivalent above as commented. Also you could usefully assert that the rls
component is now available via expect_component_executable()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the expect_component_executable check much more than the version check. Thx
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks pretty good now. Could you please rebase to clean up the commits? I'd like to see a commit for the code and a commit for the tests at minimum. Once that's done, I'll give it a final once-over but I think this very close to mergeable
In case you have an already installed nightly toolchain and you want to install a nightly toolchain with a component which is not available in the installed toolchain, rustup is searching for the most recent nightly supporting all requested components and which is not older than the currently installed nightly. With this option set it will also search for toolchains older than the currently installed one.
a11791f
to
cabee8c
Compare
@kinnison squashed it together. I have put the code change and its test together. When you split the test from the implementation it is less clear what belongs together. One reason to split test from implementation is of course to put the test before the implementation so you see the test failing first. Do you mean to put a separate test commit before the implementation commit? |
In case you have an already installed nightly toolchain
and you want to install a nightly toolchain with a component
which is not available in the installed toolchain,
rustup is searching for the most recent nightly supporting
all requested components and which is not older than the currently
installed nightly.
With this option set it will also search for toolchains older
than the currently installed one.
Should fix #2067