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

Fix component remove error by component name with explicit target triple #3601

Merged
merged 3 commits into from
Apr 23, 2024
Merged

Fix component remove error by component name with explicit target triple #3601

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
04f5a5e
Replace Component::new_with_target by Component::try_new
ongchi Dec 24, 2023
1720239
Add tests for add/remove components by name with target triple
ongchi Dec 24, 2023
cc15f59
Remove TryFrom<PartialTargetTriple> for TargetTriple
ongchi Dec 24, 2023
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
6 changes: 2 additions & 4 deletions src/cli/rustup_mode.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1364,8 +1364,7 @@ fn component_add(cfg: &Cfg, m: &ArgMatches) -> Result<utils::ExitCode> {
let target = get_target(m, &distributable);

for component in m.get_many::<String>("component").unwrap() {
let new_component = Component::new_with_target(component, false)
.unwrap_or_else(|| Component::new(component.to_string(), target.clone(), true));
let new_component = Component::try_new(component, &distributable, target.as_ref())?;
distributable.add_component(new_component)?;
}

Expand All @@ -1385,8 +1384,7 @@ fn component_remove(cfg: &Cfg, m: &ArgMatches) -> Result<utils::ExitCode> {
let target = get_target(m, &distributable);

for component in m.get_many::<String>("component").unwrap() {
let new_component = Component::new_with_target(component, false)
.unwrap_or_else(|| Component::new(component.to_string(), target.clone(), true));
let new_component = Component::try_new(component, &distributable, target.as_ref())?;
distributable.remove_component(new_component)?;
}

Expand Down
16 changes: 0 additions & 16 deletions src/dist/dist.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -468,22 +468,6 @@ impl TargetTriple {
}
}

impl std::convert::TryFrom<PartialTargetTriple> for TargetTriple {
type Error = &'static str;
fn try_from(value: PartialTargetTriple) -> std::result::Result<Self, Self::Error> {
if value.arch.is_some() && value.os.is_some() && value.env.is_some() {
Ok(Self(format!(
"{}-{}-{}",
value.arch.unwrap(),
value.os.unwrap(),
value.env.unwrap()
)))
} else {
Err("Incomplete / bad target triple")
}
}
}

impl FromStr for PartialToolchainDesc {
type Err = anyhow::Error;
fn from_str(name: &str) -> Result<Self> {
Expand Down
45 changes: 31 additions & 14 deletions src/dist/manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,9 @@ use std::str::FromStr;

use anyhow::{anyhow, bail, Context, Result};

use crate::dist::dist::{PartialTargetTriple, Profile, TargetTriple};
use crate::dist::dist::{Profile, TargetTriple};
use crate::errors::*;
use crate::toolchain::distributable::DistributableToolchain;
use crate::utils::toml_utils::*;

use super::{config::Config, dist::ToolchainDesc};
Expand Down Expand Up @@ -588,21 +589,37 @@ impl Component {
}
}

pub(crate) fn new_with_target(pkg_with_target: &str, is_extension: bool) -> Option<Self> {
for (pos, _) in pkg_with_target.match_indices('-') {
let pkg = &pkg_with_target[0..pos];
let target = &pkg_with_target[pos + 1..];
if let Some(partial) = PartialTargetTriple::new(target) {
if let Ok(triple) = TargetTriple::try_from(partial) {
return Some(Self {
pkg: pkg.to_string(),
target: Some(triple),
is_extension,
});
}
pub(crate) fn try_new(
rami3l marked this conversation as resolved.
Show resolved Hide resolved
name: &str,
distributable: &DistributableToolchain<'_>,
fallback_target: Option<&TargetTriple>,
) -> Result<Self> {
let manifestation = distributable.get_manifestation()?;
let config = manifestation.read_config()?.unwrap_or_default();
let manifest = distributable.get_manifest()?;
let manifest_components = manifest.query_components(distributable.desc(), &config)?;

for component_status in manifest_components {
let short_name = component_status.component.short_name_in_manifest();
let target = component_status.component.target.as_ref();

if name.starts_with(short_name)
&& target.is_some()
&& name == format!("{}-{}", short_name, target.unwrap())
{
return Ok(Component::new(
short_name.to_string(),
target.cloned(),
false,
));
}
}
None

Ok(Component::new(
name.to_string(),
fallback_target.cloned(),
true,
))
}

pub(crate) fn wildcard(&self) -> Self {
Expand Down
100 changes: 98 additions & 2 deletions tests/suite/cli_rustup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1366,6 +1366,63 @@ fn add_component() {
});
}

#[test]
rami3l marked this conversation as resolved.
Show resolved Hide resolved
fn add_component_by_target_triple() {
test(&|config| {
config.with_scenario(Scenario::SimpleV2, &|config| {
config.expect_ok(&["rustup", "default", "stable"]);
config.expect_ok(&[
"rustup",
"component",
"add",
&format!("rust-std-{}", clitools::CROSS_ARCH1),
]);
let path = format!(
"toolchains/stable-{}/lib/rustlib/{}/lib/libstd.rlib",
this_host_triple(),
clitools::CROSS_ARCH1
);
assert!(config.rustupdir.has(path));
})
});
}

#[test]
fn fail_invalid_component_name() {
test(&|config| {
config.with_scenario(Scenario::SimpleV2, &|config| {
config.expect_ok(&["rustup", "default", "stable"]);
config.expect_err(
&[
"rustup",
"component",
"add",
&format!("dummy-{}", clitools::CROSS_ARCH1),
],
&format!("error: toolchain 'stable-{}' does not contain component 'dummy-{}' for target '{}'",this_host_triple(), clitools::CROSS_ARCH1, this_host_triple()),
);
})
});
}

#[test]
fn fail_invalid_component_target() {
test(&|config| {
config.with_scenario(Scenario::SimpleV2, &|config| {
config.expect_ok(&["rustup", "default", "stable"]);
config.expect_err(
&[
"rustup",
"component",
"add",
"rust-std-invalid-target",
],
&format!("error: toolchain 'stable-{}' does not contain component 'rust-std-invalid-target' for target '{}'",this_host_triple(), this_host_triple()),
);
})
});
}

#[test]
fn remove_component() {
test(&|config| {
Expand All @@ -1383,22 +1440,61 @@ fn remove_component() {
});
}

#[test]
fn remove_component_by_target_triple() {
let component_with_triple = format!("rust-std-{}", clitools::CROSS_ARCH1);
test(&|config| {
config.with_scenario(Scenario::SimpleV2, &|config| {
config.expect_ok(&["rustup", "default", "stable"]);
config.expect_ok(&["rustup", "component", "add", &component_with_triple]);
let path = PathBuf::from(format!(
"toolchains/stable-{}/lib/rustlib/{}/lib/libstd.rlib",
this_host_triple(),
clitools::CROSS_ARCH1
));
assert!(config.rustupdir.has(&path));
config.expect_ok(&["rustup", "component", "remove", &component_with_triple]);
assert!(!config.rustupdir.has(path.parent().unwrap()));
})
});
}

#[test]
fn add_remove_multiple_components() {
let files = [
"lib/rustlib/src/rust-src/foo.rs".to_owned(),
format!("lib/rustlib/{}/analysis/libfoo.json", this_host_triple()),
format!("lib/rustlib/{}/lib/libstd.rlib", clitools::CROSS_ARCH1),
format!("lib/rustlib/{}/lib/libstd.rlib", clitools::CROSS_ARCH2),
];
let component_with_triple1 = format!("rust-std-{}", clitools::CROSS_ARCH1);
let component_with_triple2 = format!("rust-std-{}", clitools::CROSS_ARCH2);

test(&|config| {
config.with_scenario(Scenario::SimpleV2, &|config| {
config.expect_ok(&["rustup", "default", "nightly"]);
config.expect_ok(&["rustup", "component", "add", "rust-src", "rust-analysis"]);
config.expect_ok(&[
"rustup",
"component",
"add",
"rust-src",
"rust-analysis",
&component_with_triple1,
&component_with_triple2,
]);
for file in &files {
let path = format!("toolchains/nightly-{}/{}", this_host_triple(), file);
assert!(config.rustupdir.has(&path));
}
config.expect_ok(&["rustup", "component", "remove", "rust-src", "rust-analysis"]);
config.expect_ok(&[
"rustup",
"component",
"remove",
"rust-src",
"rust-analysis",
&component_with_triple1,
&component_with_triple2,
]);
for file in &files {
let path = PathBuf::from(format!(
"toolchains/nightly-{}/{}",
Expand Down
Loading