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

Implement --allow-downgrade option for "component add" subcommand #2176

Closed

Conversation

BeniCheni
Copy link
Contributor

@BeniCheni BeniCheni commented Dec 26, 2019

Implement --allow-downgrade option for component add sub-commend (e.g. rustup component add rls --allow-downgrade), to attempt to resolve #2146.

When --allow-downgrade is provide, it would run toolchain.install_from_dist(...), to reuse the solution in #2126.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
@kinnison
Copy link
Contributor

Hi @BeniCheni

Thank you for taking the time to implement this. And thanks to @lzutao for some initial code review, that was super-nice of you to do given I've been taking a break :D

I shall look at this as soon as I can.

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.

At the moment, there are a number of comments to be addressed. Over-all I feel like the strange double-pathway through the component add cli handling ought to be unified into a single function which essentially behaves similarly to how toolchain install works -- if that means that we end up with the Toolchain::add_component() code becoming obsolete, then so be it.

I'd like answers to my questions, and once those are resolved, a treatment of my concern over the what the test suggests the behaviour currently is. Once we've got through those, we can look to fixing up the code.

A very good start, but let's take some time to ensure we know exactly what behaviour we're expecting from this before we go further.

src/cli/rustup_mode.rs Outdated Show resolved Hide resolved
Comment on lines +1179 to +1184
let release_channel = match toolchain.name() {
s if s.starts_with("nightly") => "nightly",
s if s.starts_with("stable") => "stable",
s if s.starts_with("beta") => "beta",
_ => "nightly",
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This kind of functionality feels like it belongs on the Toolchain type


let status = if !temp_toolchain.is_custom() {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test feels like it belongs at an early bailout rather than so deep into this function.

Comment on lines +1221 to +1229
if let Some(UpdateStatus::Installed) = status {
temp_toolchain.make_default()?;
temp_toolchain.add_component(new_component)?;
toolchain.make_default()?;
} else if let Some(UpdateStatus::Unchanged) = status {
temp_toolchain.make_default()?;
temp_toolchain.add_component(new_component)?;
toolchain.make_default()?;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these pathways necessary? If we passed through install_from_dist surely we will have added the listed components? Also this odd changing of the default toolchain worries me, in case the inner add_component() calls fails, the default toolchain would have been unexpectedly altered to one which doesn't exist. That feels wrong.

Comment on lines +1545 to +1549
fn utc_from_manifest_date(date_str: &str) -> Option<Date<Utc>> {
NaiveDate::parse_from_str(date_str, "%Y-%m-%d")
.ok()
.map(|date| Utc.from_utc_date(&date))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks like it is a direct copy of code from elsewhere which might be better lifted into a utility function if it's not already in one.

Comment on lines +1487 to +1493
// rls was added to downgraded nightly-2019-09-13
// switch to the version and assert rls as executable
expect_stdout_ok(
config,
&["rustup", "default", "nightly-2019-09-13"],
"hash-nightly-2",
);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand where this new nightly toolchain has come from. I would have expected with --allow-downgrade that the installed nightly toolchain was downgraded rather than that a brand new toolchain turned up.

@kinnison kinnison added this to the 1.22.0 milestone Jan 6, 2020
Co-Authored-By: Daniel Silverstone <dsilvers@digital-scurf.org>
@BeniCheni
Copy link
Contributor Author

Thank you for your thorough questions @kinnison. I've reviewed and deemed that my implementation in this PR has a lot of confusing parts. I'd need to take a step back to grok what my thoughts were when I was writing this code when the analysis of other code to have a clear plan to edit and provide answers to your questions.

I'm going to look and think over thinks and come back with some sensible updates on this the rest of this week / weekend. Sorry about the delay. Will definitely ask questions on Discord if too stuck.

@kinnison
Copy link
Contributor

kinnison commented Jan 9, 2020

Take all the time you need @BeniCheni -- And ask all the questions you feel will help you on your way. I look forward to your reworked branch.

@kinnison
Copy link
Contributor

kinnison commented Feb 1, 2020

Hey @BeniCheni

How're things going? If you're too busy / unable to get back to this, then please just let me know so I can look for someone else to carry your work forward. I really want to see this functionality added to rustup.

Thanks,

Daniel.

@BeniCheni
Copy link
Contributor Author

Thanks for checking in @kinnison. Been a busy month. Unfortunately, I won’t be able to devote time in the next couple of weeks to follow up this PR. So I’m more than happy if someone wants to carry #2146 forward. I’ll respectfully close this PR. After the busy next two weeks, I’ll definitely come back to check out issues of rustup again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Downgrade nightly when adding component
3 participants