-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Document that Process::command will search the PATH #38018
Conversation
(rust_highfive has picked a reviewer for you, use r? to override) |
@@ -253,6 +253,10 @@ impl Command { | |||
/// Builder methods are provided to change these defaults and | |||
/// otherwise configure the process. | |||
/// | |||
/// The `PATH` from the Command's environment will be searched | |||
/// if the program contains only a single path component, as for | |||
/// `execvp`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don’t get what “as for execvp
“ part is supposed to mean. Why are you including references to POSIX specific functions in documentation of platform agnostic part of documentation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I was wondering whether that would help make the behavior clearer to people or if it was better style not to mention it. I can remove that phrase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would just give a couple of examples - "For example, PATH
will be searched for rustc
but not bin/rustc
".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That isn't the case on Windows. It uses the child's PATH
(badly: #37519) even if the program name contains multiple path components.
Windows and Linux have very different behaviour here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I saw that, what a mess.
The main thing I found missing here, as a reader, is that unqualified names will be searched along the PATH. (That's implied by some examples but I think worth having clear.)
I'd like to also say you can control that by setting the PATH environment, and it seems you can, except for various bugs on Windows. Perhaps this is too complex to get into here? It looks like general Rust style is not to mention bugs or caveats in the API docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we could leave this as just
If
program
is not an absolute path, thePATH
will be searched in an OS-defined way.
we could optionally add
The search path to be used may be controlled by setting the
PATH
environment variable on theCommand
,
but this has some implementation limitations on Windows (see #37519).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If program is not an absolute path, the PATH will be searched in an OS-defined way.
Something like that seems reasonable. I think the main thing that we need to include is that the behaviour is OS dependant.
@@ -253,6 +253,10 @@ impl Command { | |||
/// Builder methods are provided to change these defaults and | |||
/// otherwise configure the process. | |||
/// | |||
/// The `PATH` from the Command's environment will be searched |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
... the `Command`'s environment...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
How about this? |
Sounds reasonable to me! Looks like the tidy checks are failing though? |
whitespace fixed. |
Thanks! Could you also squash the commits? |
Done, commits squashed |
@bors: r+ |
📌 Commit db93677 has been approved by |
Document that Process::command will search the PATH
No description provided.