fix(cli): Help users know possible `--target` values #12607

epage · 2023-08-31T14:01:38Z

What does this PR try to resolve?

I was needing to do some more cross-compilation and forgot what the target triple was that I needed to run. I realized i had to re-remember the command yet again. Especially with #12585 in memory, I realized that --target isn't working like --package and other arguments that can report supported values.

In working on it, I realized I probably didn't want to report supported values yet out of concern for how big the list is (see also #12585), so I decided to just list the relevant commands for now. We might be able to parse the rustup output to report those targets but I didn't receive a glowing endorsement from the rustup team about parsing the list (more of "yes, hyrums law and at least its interactive rather than CI").

Before:

error: a value is required for '--target <TRIPLE>' but none was supplied

For more information, try '--help'.

After:

error: "--target" takes a target architecture as an argument.

Run `rustup target list` to see possible targets.

(quotes were used because the other "list available" use them, we should probably work to be uniform in how we quote)

How should we test and review this PR?

First PR adds a test showing the existing output and then through the rest you can see how the output changed

rustbot · 2023-08-31T14:01:43Z

r? @weihanglo

(rustbot has picked a reviewer for you, use r? to override)

weihanglo · 2023-08-31T14:08:46Z

tests/testsuite/cargo_build/help/stdout.log

@@ -42,7 +42,7 @@ Compilation Options:
      --profile <PROFILE-NAME>  Build artifacts with the specified profile
  -j, --jobs <N>                Number of parallel jobs, defaults to # of CPUs.
      --keep-going              Do not abort the build as soon as there is an error
-      --target <TRIPLE>         Build for the target triple
+      --target [<TRIPLE>]       Build for the target triple


I wish we could have a way to avoid this showing as optional 🥲.

(Never mind we already have this issue everywhere.)

weihanglo · 2023-08-31T14:08:58Z

src/cargo/util/command_prelude.rs

+            let cmd = if is_rustup() {
+                "rustup target list"
+            } else {
+                "ustc --print target-list"


Not a real objection. Just this recommendation could often be false when using +channel command line override.

And typo

Suggested change

"ustc --print target-list"

"rustc --print target-list"

Typo fixed. I can go either way in how we handle the recommendation.

cargo/src/cargo/core/compiler/standard_lib.rs

Lines 245 to 252 in 40f1f67

match target_data.config.get_env("RUSTUP_TOOLCHAIN") {

Ok(rustup_toolchain) => {

anyhow::bail!("{} --toolchain {}", msg, rustup_toolchain);

}

Err(_) => {

anyhow::bail!(msg);

}

}

We've already got something alike, though applying toolchain doesn't seem too useful. The output of rustup target list shouldn't change too much across versions, and people doing weird stuff usually know the trick.

I'll merge it as is.

weihanglo · 2023-08-31T14:56:51Z

@bors r+

bors · 2023-08-31T14:56:54Z

📌 Commit 802cb38 has been approved by weihanglo

It is now in the queue for this repository.

bors · 2023-08-31T14:57:00Z

⌛ Testing commit 802cb38 with merge 77a9b2d...

bors · 2023-08-31T15:50:23Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 77a9b2d to master...

bors · 2023-08-31T15:50:24Z

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 77a9b2d to master...

bors · 2023-08-31T15:50:28Z

👀 Test was successful, but fast-forwarding failed: 422 Changes must be made through a pull request.

Update cargo 21 commits in 96fe1c9e1aecd8f57063e3753969bb6418fd2fd5..d14c85f4e6e7671673b1a1bc87231ff7164761e1 2023-08-29 20:10:34 +0000 to 2023-09-05 22:28:10 +0000 - fix(resolver): Make resolver behavior independent of package order (rust-lang/cargo#12602) - cargo-credential: change serialization of cache expiration (rust-lang/cargo#12622) - Update registry-web-api.md yank/unyank comments (rust-lang/cargo#12619) - test: new options of debuginfo are no longer unstable (rust-lang/cargo#12618) - use split_once for cleaner code (rust-lang/cargo#12615) - stop using lazy_static (rust-lang/cargo#12616) - doc: adjust all doc headings one level up (rust-lang/cargo#12595) - chore(deps): update compatible (rust-lang/cargo#12609) - chore(deps): update rust crate cargo_metadata to 0.17.0 (rust-lang/cargo#12610) - Prepare for partial-version package specs (rust-lang/cargo#12591) - refactor: Use more serde_untagged (rust-lang/cargo#12581) - fix(cli): Help users know possible `--target` values (rust-lang/cargo#12607) - Tab completion for --target uses rustup but fallsback to rustc (rust-lang/cargo#12606) - Fewer temporary needless strings (rust-lang/cargo#12604) - fix(help): Provide better commands heading for styling (rust-lang/cargo#12593) - fix(update): Clarify meaning of --aggressive as --recursive (rust-lang/cargo#12544) - docs(changelog): Clarify language for Cargo.lock policy (rust-lang/cargo#12601) - fix typo: "default branch branch" -> "default branch" (rust-lang/cargo#12598) - fix: add error for unsupported credential provider version (rust-lang/cargo#12590) - fix(help): Explain --explain (rust-lang/cargo#12592) - fix(help): Remove redundant information from new/init (rust-lang/cargo#12594) r? ghost

epage added 7 commits August 30, 2023 20:34

test(cli): Show existing --target behavior

801b223

refactor(cli): Allow --targets to fail

92e252b

refactor(tree): Lookup --target like other commands

ca173bc

refactor(cli): Manually implement missing arg error for --target

b87c932

refactor(cli): Pull out rustup check

fe37a36

fix(cli): Help people find possible --target values

adea3d1

fix(cli): Be more specific in what is needed

802cb38

rustbot assigned weihanglo Aug 31, 2023

rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-clean Command-fetch Command-package Command-publish Command-tree S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2023

weihanglo reviewed Aug 31, 2023

View reviewed changes

epage force-pushed the target branch from 58f2237 to 802cb38 Compare August 31, 2023 14:39

weihanglo approved these changes Aug 31, 2023

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 31, 2023

bors merged commit 77a9b2d into rust-lang:master Aug 31, 2023
19 checks passed

epage deleted the target branch August 31, 2023 15:57

bors mentioned this pull request Aug 31, 2023

feat(help): Add styling to help output #12578

Merged

weihanglo mentioned this pull request Sep 6, 2023

Update cargo rust-lang/rust#115593

Merged

ehuss added this to the 1.74.0 milestone Sep 30, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(cli): Help users know possible `--target` values #12607

fix(cli): Help users know possible `--target` values #12607

epage commented Aug 31, 2023

rustbot commented Aug 31, 2023

weihanglo Aug 31, 2023

weihanglo Aug 31, 2023 •

edited

Loading

epage Aug 31, 2023

weihanglo Aug 31, 2023

weihanglo commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

	match target_data.config.get_env("RUSTUP_TOOLCHAIN") {
	Ok(rustup_toolchain) => {
	anyhow::bail!("{} --toolchain {}", msg, rustup_toolchain);
	}
	Err(_) => {
	anyhow::bail!(msg);
	}
	}

fix(cli): Help users know possible --target values #12607

fix(cli): Help users know possible --target values #12607

Conversation

epage commented Aug 31, 2023

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Aug 31, 2023

weihanglo Aug 31, 2023

Choose a reason for hiding this comment

weihanglo Aug 31, 2023 • edited Loading

Choose a reason for hiding this comment

epage Aug 31, 2023

Choose a reason for hiding this comment

weihanglo Aug 31, 2023

Choose a reason for hiding this comment

weihanglo commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

bors commented Aug 31, 2023

fix(cli): Help users know possible `--target` values #12607

fix(cli): Help users know possible `--target` values #12607

weihanglo Aug 31, 2023 •

edited

Loading