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.

Sign up for GitHub

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

Jump to bottom

rustup_mode: add toolchain install --allow-downgrade option #2126

Merged
merged 1 commit into from
Nov 29, 2019
Merged

rustup_mode: add toolchain install --allow-downgrade option #2126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/cli/rustup_mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -319,6 319,12 @@ pub fn cli() -> App<'static, 'static> {
.help("Force an update, even if some components are missing")
.long("force")
.takes_value(false),
)
.arg(
Arg::with_name("allow-downgrade")
.help("Allow rustup to downgrade the toolchain to satisfy your component choice")
.long("allow-downgrade")
.takes_value(false),
),
)
.subcommand(
Expand Down Expand Up @@ -822,7 828,12 @@ fn update(cfg: &mut Cfg, m: &ArgMatches<'_>) -> Result<()> {
.values_of("targets")
.map(|v| v.collect())
.unwrap_or_else(Vec::new);
Some(toolchain.install_from_dist(m.is_present("force"), &components, &targets)?)
Some(toolchain.install_from_dist(
m.is_present("force"),
m.is_present("allow-downgrade"),
&components,
&targets,
)?)
} else if !toolchain.exists() {
return Err(ErrorKind::InvalidToolchainName(toolchain.name().to_string()).into());
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/cli/self_update.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -782,7 782,7 @@ fn maybe_install_rust(
println!();
} else if cfg.find_default()?.is_none() {
let toolchain = cfg.get_toolchain(toolchain_str, false)?;
let status = toolchain.install_from_dist(true, components, targets)?;
let status = toolchain.install_from_dist(true, false, components, targets)?;
cfg.set_default(toolchain_str)?;
println!();
common::show_channel_update(&cfg, toolchain_str, Ok(status))?;
Expand Down
6 changes: 3 additions & 3 deletions src/config.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 438,7 @@ impl Cfg {
ErrorKind::OverrideToolchainNotInstalled(name.to_string())
})
} else {
toolchain.install_from_dist(true, &[], &[])?;
toolchain.install_from_dist(true, false, &[], &[])?;
Ok(Some((toolchain, reason)))
}
}
Expand Down Expand Up @@ -547,7 547,7 @@ impl Cfg {
// Update toolchains and collect the results
let channels = channels.map(|(n, t)| {
let t = t.and_then(|t| {
let t = t.install_from_dist(force_update, &[], &[]);
let t = t.install_from_dist(force_update, false, &[], &[]);
if let Err(ref e) = t {
(self.notify_handler)(Notification::NonFatalError(e));
}
Expand Down Expand Up @@ -599,7 599,7 @@ impl Cfg {
) -> Result<Command> {
let toolchain = self.get_toolchain(toolchain, false)?;
if install_if_missing && !toolchain.exists() {
toolchain.install_from_dist(true, &[], &[])?;
toolchain.install_from_dist(true, false, &[], &[])?;
}

if let Some(cmd) = self.maybe_do_cargo_fallback(&toolchain, binary)? {
Expand Down
46 changes: 24 additions & 22 deletions src/dist/dist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -580,6 580,7 @@ pub fn update_from_dist<'a>(
profile: Option<Profile>,
prefix: &InstallPrefix,
force_update: bool,
allow_downgrade: bool,
old_date: Option<&str>,
components: &[&str],
targets: &[&str],
Expand All @@ -601,6 602,7 @@ pub fn update_from_dist<'a>(
profile,
prefix,
force_update,
allow_downgrade,
old_date,
components,
targets,
Expand All @@ -622,6 624,7 @@ fn update_from_dist_<'a>(
profile: Option<Profile>,
prefix: &InstallPrefix,
force_update: bool,
allow_downgrade: bool,
old_date: Option<&str>,
components: &[&str],
targets: &[&str],
Expand All @@ -644,21 647,24 @@ fn update_from_dist_<'a>(
Some(if provided < 1 { 1 } else { provided })
};

// We never want to backtrack further back than the nightly that's already installed.
// In case there is no allow-downgrade option set
// we never want to backtrack further back than the nightly that's already installed.
//
// If no nightly is installed, it makes no sense to backtrack beyond the first ever manifest,
// which is 2014-12-20 according to
// https://static.rust-lang.org/cargo-dist/index.html.
//
// We could arguably use the date of the first rustup release here, but that would break a
// bunch of the tests, which (inexplicably) use 2015-01-01 as their manifest dates.
let first_manifest = old_date
.map(|date| {
Utc.from_utc_date(
&NaiveDate::parse_from_str(date, "%Y-%m-%d").expect("Malformed manifest date"),
)
})
.unwrap_or_else(|| Utc.from_utc_date(&NaiveDate::from_ymd(2014, 12, 20)));
let first_manifest = Utc.from_utc_date(&NaiveDate::from_ymd(2014, 12, 20));
let old_manifest = old_date
.and_then(|date| utc_from_manifest_date(date))
.unwrap_or(first_manifest);
let last_manifest = if allow_downgrade {
first_manifest
} else {
old_manifest
};

loop {
match try_update_from_dist_(
Expand Down Expand Up @@ -713,22 719,12 @@ fn update_from_dist_<'a>(
// the components that the user currently has installed. Let's try the previous
// nightlies in reverse chronological order until we find a nightly that does,
// starting at one date earlier than the current manifest's date.
let try_next = Utc
.from_utc_date(
&NaiveDate::parse_from_str(
toolchain.date.as_ref().unwrap_or(&fetched),
"%Y-%m-%d",
)
.unwrap_or_else(|_| {
panic!(
"Malformed manifest date: {:?}",
toolchain.date.as_ref().unwrap_or(&fetched)
)
}),
)
let toolchain_date = toolchain.date.as_ref().unwrap_or(&fetched);
let try_next = utc_from_manifest_date(toolchain_date)
.unwrap_or_else(|| panic!("Malformed manifest date: {:?}", toolchain_date))
.pred();

if try_next < first_manifest {
if try_next < last_manifest {
// Wouldn't be an update if we go further back than the user's current nightly.
if let Some(e) = first_err {
break Err(e);
Expand Down Expand Up @@ -926,3 922,9 @@ fn dl_v1_manifest<'a>(download: DownloadCfg<'a>, toolchain: &ToolchainDesc) -> R

Ok(urls)
}

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))
}
4 changes: 4 additions & 0 deletions src/install.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 24,8 @@ pub enum InstallMethod<'a> {
DownloadCfg<'a>,
// --force
bool,
// --allow-downgrade
bool,
// toolchain already exists
bool,
// currently installed date
Expand Down Expand Up @@ -66,6 68,7 @@ impl<'a> InstallMethod<'a> {
update_hash,
dl_cfg,
force_update,
allow_downgrade,
exists,
old_date,
components,
Expand All @@ -79,6 82,7 @@ impl<'a> InstallMethod<'a> {
if exists { None } else { Some(profile) },
prefix,
force_update,
allow_downgrade,
old_date,
components,
targets,
Expand Down
3 changes: 3 additions & 0 deletions src/toolchain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 166,7 @@ impl<'a> Toolchain<'a> {
pub fn install_from_dist(
&self,
force_update: bool,
allow_downgrade: bool,
components: &[&str],
targets: &[&str],
) -> Result<UpdateStatus> {
Expand All @@ -177,6 178,7 @@ impl<'a> Toolchain<'a> {
update_hash.as_ref().map(|p| &**p),
self.download_cfg(),
force_update,
allow_downgrade,
self.exists(),
old_date.as_ref().map(|s| &**s),
components,
Expand All @@ -193,6 195,7 @@ impl<'a> Toolchain<'a> {
self.download_cfg(),
false,
false,
false,
None,
&[],
&[],
Expand Down
57 changes: 57 additions & 0 deletions tests/cli-v2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1392,3 1392,60 @@ fn check_pgp_keys() {
);
})
}

#[test]
fn install_allow_downgrade() {
clitools::setup(Scenario::MissingComponent, &|config| {
let trip = TargetTriple::from_build();

// this dist has no rls and there is no newer one
set_current_dist_date(config, "2019-09-14");
expect_ok(
config,
&[
"rustup",
"toolchain",
"install",
"nightly",
"--no-self-update",
],
);
Copy link
Contributor

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)

Copy link
Contributor Author

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.

expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-3");
expect_component_not_executable(config, "rls");

expect_err(
config,
&[
"rustup",
"toolchain",
"install",
"nightly",
"--no-self-update",
"-c",
"rls",
],
&format!(
"component 'rls' for target '{}' is unavailable for download for channel nightly",
trip,
),
);
Copy link
Contributor

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");

Copy link
Contributor Author

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 :)

expect_stdout_ok(config, &["rustc", "--version"], "hash-nightly-3");
expect_component_not_executable(config, "rls");

expect_ok(
config,
&[
"rustup",
"toolchain",
"install",
"nightly",
"--no-self-update",
"-c",
"rls",
"--allow-downgrade",
],
);
expect_stdout_ok(config, &["rustc", "--version"], "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.

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()

Copy link
Contributor Author

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

expect_component_executable(config, "rls");
});
}