Skip to content

feat(custom-toolchains): rustup show now reporting installed targets#4333

Merged
rami3l merged 3 commits intorust-lang:masterfrom
FranciscoTGouveia:show-targets-custom-toolchains
May 19, 2025
Merged

feat(custom-toolchains): rustup show now reporting installed targets#4333
rami3l merged 3 commits intorust-lang:masterfrom
FranciscoTGouveia:show-targets-custom-toolchains

Conversation

@FranciscoTGouveia
Copy link
Copy Markdown
Contributor

Closes #4251.

Hi everyone! This is my first PR on rustup, part of my community bonding period of GSoC (on the "Making Rustup Concurrent" project).

I'd appreciate your feedback on a few decisions I made, such as:

  • Assuming the existence of a rustlib/components file, even for custom toolchains.
  • Placing the function for listing custom targets in toolchain.rs.

Additionally, before I add a test, I’d like to ask where it would be most appropriate to place it: should it go in tests/cli_v2.rs or tests/cli_rustup.rs?

Thank you in advance for your time and input!

@rami3l rami3l self-assigned this May 16, 2025
Copy link
Copy Markdown
Member

@rami3l rami3l left a comment

Choose a reason for hiding this comment

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

Nice changes overall!

Pre-existing issue, but it'd probably be better if we make this new behavior more consistent across the board if rustup component list and rustup target list come into play as well. You can give the same outputs with or without the flag --installed if it's a custom toolchain, I believe.

As for the use of the rustlib/components file, I believe it's the best thing to do now, but do make sure the user experience is not heavily damaged when the file is missing (meaning you need to add a test case for the failing case as well).

cli_v1 and cli_v2 are for tests specific to a certain manifest version. Since this machinery is a fallback in the manifest-free case, it's better to put it in another file.

Many thanks in advance 🙏

@FranciscoTGouveia FranciscoTGouveia force-pushed the show-targets-custom-toolchains branch 2 times, most recently from 029cd9a to 515c70d Compare May 16, 2025 17:20
@FranciscoTGouveia
Copy link
Copy Markdown
Contributor Author

Pre-existing issue, but it'd probably be better if we make this new behavior more consistent across the board if rustup component list and rustup target list come into play as well.

Should I open a new issue/PR for this in order to keep this PR as small/atomic as possible, or should I do it here?

As for the use of the rustlib/components file, I believe it's the best thing to do now, but do make sure the user experience is not heavily damaged when the file is missing (meaning you need to add a test case for the failing case as well).

In the failing case should I display any kind of error or should I keep the previous behavior of simply not reporting any installed target?

@rami3l
Copy link
Copy Markdown
Member

rami3l commented May 19, 2025

Should I open a new issue/PR for this in order to keep this PR as small/atomic as possible, or should I do it here?

I'd prefer doing that in a separate PR that follows this one. Thanks :)

As for the use of the rustlib/components file, I believe it's the best thing to do now, but do make sure the user experience is not heavily damaged when the file is missing (meaning you need to add a test case for the failing case as well).

In the failing case should I display any kind of error or should I keep the previous behavior of simply not reporting any installed target?

It's okay to keep the previous behavior in the error case. We can always add that back later if this confuses some of our users.

@FranciscoTGouveia FranciscoTGouveia force-pushed the show-targets-custom-toolchains branch from c26e3a2 to 3fe8df2 Compare May 19, 2025 14:07
@rami3l rami3l enabled auto-merge May 19, 2025 14:21
@rami3l rami3l added this pull request to the merge queue May 19, 2025
Merged via the queue into rust-lang:master with commit 664ee2c May 19, 2025
29 checks passed
@rami3l rami3l added this to the 1.28.3 milestone Jul 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rustup show doesn't correctly report installed targets for custom toolchains

2 participants