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.

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

Enhance help texts of position args #11740

Merged
merged 1 commit into from
Feb 20, 2023
Merged

Conversation

weihanglo
Copy link
Member

What does this PR try to resolve?

Fixes #9707

In #9707, there is an assumption that people tend to run cargo run --help and expect it shows the help text of the binary to run. However, the two proposed solution there both may lead to a false positive. I personally don't feel like any of them is a solution we would consider.

How should we test and review this PR?

Run each command below and check its help text.

cargo run --help
cargo rustdoc --help
cargo rustc --help

@rustbot
Copy link
Collaborator

rustbot commented Feb 19, 2023

r? @ehuss

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

@rustbot rustbot added A-cli Area: Command-line interface, option parsing, etc. Command-run Command-rustc Command-rustdoc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2023
@ehuss
Copy link
Contributor

ehuss commented Feb 20, 2023

This looks good to me, but I'm going to r? @epage

@rustbot rustbot assigned epage and unassigned ehuss Feb 20, 2023
@epage
Copy link
Contributor

epage commented Feb 20, 2023

Agree that this is good on its own. Do we want to block closing out the issue on cargo help? If so, we could either block this PR on it or change the PR from saying "Fixes". I leave that to @weihanglo

@weihanglo
Copy link
Member Author

I still think #9707 won't go anywhere, given we never knows if --help in your binary is really a help text or something else. Also, at the time the issue created, clap puts ARGS section at the bottom. It is now on the very top, which hugely improves the visibility. #9707 is generally “fixed” in terms of discovery I feel like.

@epage
Copy link
Contributor

epage commented Feb 20, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Feb 20, 2023

📌 Commit e59ae78 has been approved by epage

It is now in the queue for this repository.

@bors 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 Feb 20, 2023
@bors
Copy link
Contributor

bors commented Feb 20, 2023

⌛ Testing commit e59ae78 with merge 6e66ee8...

@bors
Copy link
Contributor

bors commented Feb 20, 2023

☀️ Test successful - checks-actions
Approved by: epage
Pushing 6e66ee8 to master...

@bors bors merged commit 6e66ee8 into rust-lang:master Feb 20, 2023
@weihanglo weihanglo deleted the issue/9707 branch February 20, 2023 16:41
@pnkfelix
Copy link
Member

I'm confused, and maybe I will go try to write my own pull request to resolve my own confusion. But before I go down that path, I want to understand the current situation.

However, the two proposed solution there both may lead to a false positive.

When you say the second suggestion may lead to a false positive, where the second suggestion in question is something covering the following two bullets:

  • cargo run --help would also include a line suggesting that one might do cargo run -- --help
  • cargo run --bin program --help would include a line suggesting cargo run --bin program -- --help

(where perhaps I should have emphasized "one might do" in the above).

I'm trying to understand the pro's and con's of suggesting the above to a user, who may not be familiar with the pattern of using -- to stop argument parser in other command line tools like ls.

Is the problem that concerns you that passing --help to the underlying binary could cause something other than a help message to be printed; for example, some destructive action could take place in response to the user issuing the command in question?

While I don't disagree that what you describe is possible in as a pathological scenario, I think it would solely arise as a pathological case. Someone who is willing to run a misunderstood (or worse yet, untrusted) binary via a blind invocation of cargo run is in for the same world of pain as someone willing to invoke cargo run -- --help.


Furthermore, ... maybe I misunderstand how cargo has evolved since I filed #9707, but ... the current help output for cargo run --help doesn't even mention the role of -- ...?

So, how is a user going to figure out from the current help output that what they need to do is pass -- --help?

At least the old help output used to include a mention [--] in the output, so one had a hint that it might be something one might need to use. But that [--] text no longer appears in the current output, perhaps due to underlying changes in clap, I do not know the details there.

@weihanglo
Copy link
Member Author

weihanglo commented Feb 21, 2023

Thank you for the reply! My bad I shouldn't rush on this change.

So, how is a user going to figure out from the current help output that what they need to do is pass -- --help?

The complete help is embedded in cargo help run today, which mentions the usage of --. It is unfortunately less visible though.

Is the problem that concerns you that passing --help to the underlying binary could cause something other than a help message to be printed; for example, some destructive action could take place in response to the user issuing the command in question?

Yes. From the angle of correctness, I am a bit reluctant to to give an answer that may not exist or not correct. For example, a user runs cargo run --bin x --target wasm-wasi --help. Should cargo suggest cargo run --bin x --target wasm32-wasi -- --help or cargo run --bin x -- --target wasm32-wasi --help, or even cargo run -- --bin x --target wasm32-wasi --help?

Another point is why only recommending --help but not other flags. But yep --help is more common and may be worth a line or two of suggestion.

From the technical aspect, the current help text is, iirc, static. There might be more works to make a dynamic suggestion happen.

To make this actionable, we could probably add back a concise version of the old message mentioning -- (current lives in cargo help run). I'll reopen the issue and thank you for still being aware of this issue!

weihanglo added a commit to weihanglo/rust that referenced this pull request Feb 23, 2023
15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6
2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000

- refactor(job_queue): docs and move types around (rust-lang/cargo#11758)
- Scrub more of the test environment (rust-lang/cargo#11757)
- Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754)
- Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755)
- use consistent case (rust-lang/cargo#11748)
- Switch some tests from `build` to `check` (rust-lang/cargo#11725)
- Fix typo in sparse-registry warning message (rust-lang/cargo#11753)
- reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750)
- Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727)
- Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749)
- mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747)
- Cleanup tests (rust-lang/cargo#11745)
- Enhance help texts of position args (rust-lang/cargo#11740)
- Fix typo (rust-lang/cargo#11741)
- Update comment about cargo-ok (rust-lang/cargo#11724)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2023
Update cargo

15 commits in 17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6..9d5b32f503fc099c4064298465add14d4bce11e6
2023-02-17 19:45:09 +0000 to 2023-02-22 23:04:16 +0000

- refactor(job_queue): docs and move types around (rust-lang/cargo#11758)
- Scrub more of the test environment (rust-lang/cargo#11757)
- Make more reads of environment variables go through the `Config` (rust-lang/cargo#11754)
- Revert "Update curl-sys to use libcurl 7.88.1" (rust-lang/cargo#11755)
- use consistent case (rust-lang/cargo#11748)
- Switch some tests from `build` to `check` (rust-lang/cargo#11725)
- Fix typo in sparse-registry warning message (rust-lang/cargo#11753)
- reuse url encoding from `url` crate, don't use separate `percent-encoding` (rust-lang/cargo#11750)
- Read environment variables through `Config` instead of `std::env::var(_os)` (rust-lang/cargo#11727)
- Update curl-sys to use libcurl 7.88.1 (rust-lang/cargo#11749)
- mdman: update pretty_assertions to reduce deps (rust-lang/cargo#11747)
- Cleanup tests (rust-lang/cargo#11745)
- Enhance help texts of position args (rust-lang/cargo#11740)
- Fix typo (rust-lang/cargo#11741)
- Update comment about cargo-ok (rust-lang/cargo#11724)
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli Area: Command-line interface, option parsing, etc. Command-run Command-rustc Command-rustdoc S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Detect if --help was likely intended for binary being run
6 participants