Skip to content

fix(manifest): consider possible renames in Component::try_new() #3991

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

Merged
merged 1 commit into from
Aug 19, 2024
Merged

fix(manifest): consider possible renames in Component::try_new() #3991

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
16 changes: 4 additions & 12 deletions src/dist/manifest.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -498,19 +498,11 @@ impl Component {
distributable: &DistributableToolchain<'_>,
fallback_target: Option<&TargetTriple>,
) -> Result<Self> {
let manifest = distributable.get_manifest()?;
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like distributable.components() also contains a call to distributable.get_manifest(), so doing that twice might be kind of wasteful? Is it worth doing that differently?

Copy link
Member Author

@rami3l rami3l Aug 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@djc For the moment I'd like to keep it that way for readability, as this really isn't a hot function. I agree that the implementation is weird underneath, but as soon as I replace that with an inlined .components() I realized .get_manifest() actually calls .get_manifestation() which belongs to another layer of problems to be solved... Let's keep it this way first?

It definitely requires some refactoring afterwards anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

Works for me!

for component_status in distributable.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,
));
let component = component_status.component;
if name == component.name_in_manifest() || name == component.name(&manifest) {
return Ok(component);
}
}

Expand Down
32 changes: 32 additions & 0 deletions tests/suite/cli_rustup.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1542,6 +1542,38 @@ async fn add_component_by_target_triple() {
assert!(cx.config.rustupdir.has(path));
}

#[tokio::test]
async fn add_component_by_target_triple_renamed_from() {
let mut cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config.expect_ok(&["rustup", "default", "nightly"]).await;
cx.config
.expect_ok(&["rustup", "component", "add", for_host!("rls-{}")])
.await;
cx.config
.expect_ok_contains(
&["rustup", "component", "list", "--installed"],
for_host!("rls-{}"),
"",
)
.await;
}

#[tokio::test]
async fn add_component_by_target_triple_renamed_to() {
let mut cx = CliTestContext::new(Scenario::SimpleV2).await;
cx.config.expect_ok(&["rustup", "default", "nightly"]).await;
cx.config
.expect_ok(&["rustup", "component", "add", for_host!("rls-preview-{}")])
.await;
cx.config
.expect_ok_contains(
&["rustup", "component", "list", "--installed"],
for_host!("rls-{}"),
"",
)
.await;
}

#[tokio::test]
async fn fail_invalid_component_name() {
let mut cx = CliTestContext::new(Scenario::SimpleV2).await;
Expand Down
Loading