Skip to content

Update documentation for std::process::Command's new method #113787

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

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Update documentation for std::process::Command's new method #113787

merged 1 commit into from
Jul 20, 2023

Conversation

sanchopanca
Copy link
Contributor

In the current documentation, it's not specified that when creating a Command, the .exe extension can be omitted for Windows executables. However, for other types of executable files like .bat or .cmd, the complete filename including the extension must be provided.

I encountered it by noticing that Command::new("wt").spawn().unwrap() succeeds on my machine while Command::new("code").spawn().unwrap() panics. Turns out VS Code's entrypoint is .cmd file.

resolve_exe method mentions this behaviour in a comment, but it makes sense to mention it at a more visible place.

I've added this clarification to the documentation, which should make it more accurate and helpful for Rust developers working on the Windows platform.

@rustbot
Copy link
Collaborator

rustbot commented Jul 17, 2023

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cuviper (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jul 17, 2023
@ChrisDenton
Copy link
Member

ChrisDenton commented Jul 18, 2023

I would love better documentation here but I'm also a bit reluctant to encourage people to pass .bat files directly to Command::new instead of doing Command::new('cmd.exe').args(&["/c", "script.bat"]) as the former is not well supported (though it should hopefully work in that simple case).

We use CreateProcessW under the hood which says:

To run a batch file, you must start the command interpreter; set lpApplicationName to cmd.exe and set lpCommandLine to the following arguments: /c plus the name of the batch file.

Basically it's only for running .exe files and not script files. But it turned out this wasn't quite true. An undocumented feature is that it did attempt to do this transformation for you when given a .bat file (or .cmd). So when, for security reasons, Rust switched to using its own code to resolve exe paths we attempted to emulate this so as to avoid breaking people's existing code. However, I would still be nervous about people relying on that.

So, yes, I'm in favour of documenting the behaviour but I'm less certain about referencing "other executable files" because as far as CreateProcessW is concerned there's officially only one type of executable file.

@cuviper
Copy link
Member

cuviper commented Jul 18, 2023

If you don't mind, I'll let you own this review. :)

r? @ChrisDenton

@rustbot rustbot assigned ChrisDenton and unassigned cuviper Jul 18, 2023
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

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

For sure! As per my comment above I am generally in favour of this but have one concern.

In the current documentation, it's not specified that when creating
a Command, the .exe extension can be omitted for Windows executables.
However, for other types of executable files like .bat or .cmd,
the complete filename including the extension must be provided.

I encountered it by noticing that `Command::new("wt").spawn().unwrap()`
succeeds on my machine while `Command::new("code").spawn().unwrap()`
panics. Turns out VS Code's entrypoint is .cmd file.

`resolve_exe` method mentions this behaviour in a comment[1], but it
makes sense to mention it at more visible place.

I've added this clarification to the documentation, which should
make it more accurate and helpful for Rust developers
working on the Windows platform.

[1] https://github.com/rust-lang/rust/blob/e7fda447e7d05b6ca431fc8fe8f489b1fda810bc/library/std/src/sys/windows/process.rs#L425
@sanchopanca
Copy link
Contributor Author

@ChrisDenton thanks for the review. I have incorporated you suggestions.

@ChrisDenton
Copy link
Member

Thanks!

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Jul 19, 2023

📌 Commit 5dea766 has been approved by ChrisDenton

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 Jul 19, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#113710 (Fix rpath for libdir is specified)
 - rust-lang#113787 (Update documentation for std::process::Command's new method)
 - rust-lang#113795 (Properly document `lifetime_mapping` in `OpaqueTy`)
 - rust-lang#113857 (Add tests for `--document-hidden-items` option)
 - rust-lang#113871 (Use the correct span for displaying the line following a derive sugge…)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 538dcda into rust-lang:master Jul 20, 2023
@rustbot rustbot added this to the 1.73.0 milestone Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants