-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Capture Command
environment at spawn
#46789
Conversation
r? @TimNN (rust_highfive has picked a reviewer for you, use r? to override) |
While writing this, I found a few potential issues which I didn't attempt to fix as part of this PR:
|
r? @dtolnay since you already commented on the issue and I don't feel like I have enough understanding of the whole topic. |
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.
This is exactly what I had in mind. Nicely done, and thanks for flagging the confusing previous behavior.
@rust-lang/libs As of Rust 1.22 the behavior of modifying environment variables on a // Is this visible to child process? I would hope so.
os::set_var("A", "A");
let mut command = Command::new("...");
// Is this visible? Basically arbitrary, but currently yes.
os::set_var("B", "B");
if cond() {
// Set in child env but not parent process.
command.env("C", "C");
}
// Is this visible? It seems obvious to me that the answer should be the same
// as for B. Currently the answer is maybe! Depends on cond(). Child process
// sees either C or D but not both.
os::set_var("D", "D");
command.spawn() This PR makes A, B, C, D all visible to the child process. The behavior is as if every environment modification to the |
@rfcbot fcp merge |
Team member @dtolnay has proposed to merge this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
☔ The latest upstream changes (presumably #46798) made this pull request unmergeable. Please resolve the merge conflicts. |
9325c91
to
6679768
Compare
🔔 This is now entering its final comment period, as per the review above. 🔔 |
@bors: r=dtolnay |
📌 Commit 6679768 has been approved by |
⌛ Testing commit 6679768bb144337c8141e46eb5a1052297786637 with merge 97e576bbf0e204ebdf1aed46f5dc7b4790d06b69... |
💔 Test failed - status-travis |
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.
wasm32-unknown
failed, legit.
src/libstd/sys/wasm/process.rs
Outdated
|
||
//////////////////////////////////////////////////////////////////////////////// | ||
// Command | ||
//////////////////////////////////////////////////////////////////////////////// | ||
|
||
pub struct Command { | ||
env: CommandEnv |
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.
Should be CommandEnv<DefaultEnvKey>
here?
[00:41:37] error[E0243]: wrong number of type arguments: expected 1, found 0
[00:41:37] --> /checkout/src/libstd/sys/wasm/process.rs:24:10
[00:41:37] |
[00:41:37] 24 | env: CommandEnv
[00:41:37] | ^^^^^^^^^^ expected 1 type argument
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 will fix that - ooi, is there a cargo check
equivalent for x.py
? I'd like to be able to compile targets like wasm/fuchsia/etc. without needing to have a whole cross-compiling toolchain...
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.
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.
Fixed.
6679768
to
da90b1d
Compare
@bors r=dtolnay |
📌 Commit da90b1d has been approved by |
Capture `Command` environment at spawn Fixes #28975 This tracks a set of changes to the environment and then replays them at spawn time.
💔 Test failed - status-travis |
src/libstd/sys/wasm/process.rs
Outdated
} | ||
|
||
pub fn env_clear(&mut self) { | ||
pub fn env_mut(&mut self) -> CommandEnv<DefaultEnvKey> { |
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.
Moar wasm typo (missing &mut
).
[00:39:21] error[E0308]: mismatched types
[00:39:21] --> /checkout/src/libstd/sys/wasm/process.rs:52:9
[00:39:21] |
[00:39:21] 51 | pub fn env_mut(&mut self) -> CommandEnv<DefaultEnvKey> {
[00:39:21] | ------------------------- expected `sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>` because of return type
[00:39:21] 52 | &mut self.env
[00:39:21] | ^^^^^^^^^^^^^ expected struct `sys_common::process::CommandEnv`, found mutable reference
[00:39:21] |
[00:39:21] = note: expected type `sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>`
[00:39:21] found type `&mut sys_common::process::CommandEnv<sys_common::process::DefaultEnvKey>`
da90b1d
to
7f61b91
Compare
7f61b91
to
ccc91d7
Compare
@kennytm think I've got it this time... |
@bors r=dtolnay |
📌 Commit ccc91d7 has been approved by |
Capture `Command` environment at spawn Fixes #28975 This tracks a set of changes to the environment and then replays them at spawn time.
☀️ Test successful - status-appveyor, status-travis |
Update outdated comment in unix Command. The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`. That can still be done in the future, but I think for now it would be good to just correct the comment.
Update outdated comment in unix Command. The big comment in the `Command` struct has been incorrect for some time (at least since rust-lang#46789 which removed `envp`). Rather than try to remove the allocations, this PR just updates the comment to reflect reality. There is an explanation for the reasoning at rust-lang#31409 (comment), discussing the potential of being able to call `Command::exec` after `libc::fork`. That can still be done in the future, but I think for now it would be good to just correct the comment.
Fixes #28975
This tracks a set of changes to the environment and then replays them at spawn time.