Tell subprocesses the path to self in an env variable #3778 #3789
Conversation
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @brson (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
|
Could this be done in a perhaps less invasive way than setting an env var? Could we just set it for child processes where it's relevant? |
|
Sure, provided I'll be able to find the right / relevant places. I'll update... |
|
Ok, so I restricted the env var setting to external subcommands. (I'm still not sure about #3744, I'll update for that one later...) Edit: Updated setting env var for |
src/cargo/ops/cargo_test.rs
Outdated
| @@ -91,6 +92,9 @@ fn run_unit_tests(options: &TestOptions, | |||
| }; | |||
| let mut cmd = compilation.target_process(exe, pkg)?; | |||
| cmd.args(test_args); | |||
| if let Some(arg0) = env::args_os().next() { | |||
There was a problem hiding this comment.
If you follow the definition of target_process I think you'll find the fill_env function which is likely where this should go.
|
Yeah, it occured to me tests would be nice too, this is a bit tricky though, I'm gonna need an 'answering subprocess'. I have no idea what the testing environment requirements are, can I assume it is unixy and can execute basic shell scripts? |
|
Oh you can take a look at |
|
Uhh, lemme fix that... |
|
Reading this again, I think that we may also want to store this somewhere in |
|
Yeah, I've just rewritten it using |
|
Does something like |
|
Yeah sounds fine by me |
http://doc.crates.io/environment-variables.html seems like a good option. |
|
r=me with some docs @matklad pointed out, thanks @vojtechkral! |
|
(oops) |
| @@ -41,6 +41,8 @@ use term::color::{BLACK}; | |||
|
|
|||
| pub use util::{CargoError, CargoResult, CliError, CliResult, human, Config, ChainError}; | |||
|
|
|||
| pub const CARGO_ENV: &'static str = "CARGO"; | |||
There was a problem hiding this comment.
I think something more verbose like CARGO_EXECUTABLE would be a tiny bit better, but we already use plain RUSTC and RUSRDOC variables for similar purposes, and in this regard just CARGO is indeed the best name here.
There was a problem hiding this comment.
I don't really have a strong feeling one way or another (hehe) but I guess, like you said, CARGO is good for consistency with RUSTC ...
|
Writing the documentation makes me think whether maybe we should just call |
|
Yeah those two code paths are unfortunately pretty different |
|
(added doc + squashed) |
|
@bors: r+ Thanks @vojtechkral! |
|
📌 Commit 015a08a has been approved by |
Tell subprocesses the path to self in an env variable #3778 I'm just setting the env var on the process itself, letting subprocesses inherit that, as it's easier than setting for each subprocess individually. I'm not entirely sure this is the right spot though. Also, I should probably document this somewhere - what would be the best place?
|
☀️ Test successful - status-appveyor, status-travis |
I'm just setting the env var on the process itself, letting subprocesses inherit that, as it's easier than setting for each subprocess individually. I'm not entirely sure this is the right spot though.
Also, I should probably document this somewhere - what would be the best place?