Skip to content

document the platform-specific behavior of Command::current_dir #53404

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
Aug 28, 2018

Conversation

oconnor663
Copy link
Contributor

See also #37868.

Here's my initial wording:

Note that if the program path is relative (e.g. "./script.sh"), the interaction between that path and current_dir varies across platforms. Windows currently ignores current_dir when locating the program, but Unix-like systems interpret the program path relative to current_dir. These implementation details aren't considered stable, and it's recommended to call canonicalize to get an absolute program path instead of using relative paths and current_dir together.

I'd like to get feedback on:

@rust-highfive
Copy link
Contributor

r? @aidanhs

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 15, 2018
@frewsxcv
Copy link
Member

Should we consider those details stable? It might be disruptive to change them, regardless of what I can get away with claiming in docs :)

cc @rust-lang/libs w.r.t. this question

@retep998 retep998 added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Aug 16, 2018
@alexcrichton
Copy link
Member

I would personally prefer not to document this in the sense that it makes it a larger hazard to fix in the future. Ideally we'd fix this as opposed to documenting the bug as expected behavior

@SimonSapin
Copy link
Contributor

Without documenting specifics of the current behavior, maybe we should still document that the interaction (or lack thereof, either way) of current_dir with relative command paths should not be relied on?

@oconnor663
Copy link
Contributor Author

Do we know what fix we want? This might not be specifically the ticket to discuss it, but I'm partial towards "have the Unix implementation call canonicalize, and in the unlikely event that it fails, fall back to best-effort-just-using-strings-ignoring-symlinks canonicalization." Would that worry anyone?

@alexcrichton
Copy link
Member

I'm not sure what sort of fix we want, but I like @SimonSapin's idea of basically documenting that this is a hazard and should likely be avoided as opposed to documenting specific behavior.

@oconnor663 oconnor663 force-pushed the current_dir_behavior branch from 2842027 to 0945b74 Compare August 20, 2018 21:03
@oconnor663
Copy link
Contributor Author

I've changed the wording and rebased:

If the program path is relative (e.g. "./script.sh"), it's ambiguous whether it should be interpreted relative to the parent's working directory or relative to current_dir. The behavior in this case is platform specific and unstable, and it's recommended to use canonicalize to get an absolute program path instead.

@alexcrichton
Copy link
Member

@bors: r+

Thanks!

@bors
Copy link
Collaborator

bors commented Aug 20, 2018

📌 Commit 2842027f860ecd731eb0f38e28a10f9e7a8d7fa2 has been approved by alexcrichton

@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 Aug 20, 2018
@oconnor663
Copy link
Contributor Author

@alexcrichton is it normal for bors to take a week to merge something? Anything I need to fix?

@alexcrichton
Copy link
Member

@bors: r+

hm it seems bors forgot about this PR!

@bors
Copy link
Collaborator

bors commented Aug 27, 2018

📌 Commit 0945b74 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Aug 28, 2018

⌛ Testing commit 0945b74 with merge ca63a4e...

bors added a commit that referenced this pull request Aug 28, 2018
document the platform-specific behavior of Command::current_dir

See also #37868.

Here's my initial wording:

> Note that if the program path is relative (e.g. `"./script.sh"`), the interaction between that path and `current_dir` varies across platforms. Windows currently ignores `current_dir` when locating the program, but Unix-like systems interpret the program path relative to `current_dir`. These implementation details aren't considered stable, and it's recommended to call `canonicalize` to get an absolute program path instead of using relative paths and `current_dir` together.

I'd like to get feedback on:

- _Should_ we consider those details stable? It might be disruptive to change them, regardless of what I can get away with claiming in docs :)
- Is `canonicalize` an appropriate recommendation? As discussed in #37868 above, there are reasons it's not called automatically in the `Command` implementation.
@bors
Copy link
Collaborator

bors commented Aug 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing ca63a4e to master...

@bors bors merged commit 0945b74 into rust-lang:master Aug 28, 2018
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-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants