Provide an API to extract fields from Command builder

[euclio]

This is now implemented on nightly via #77029.

Summary
The following accessors are available on Command behind the #![feature(command_access)] gate:

• Command::get_program
• Command::get_args
• Command::get_envs
• Command::get_current_dir

Unresolved issues
Some concerns I had with the implementation, but probably not important:

• Values with NULs on Unix will be returned as "<string-with-nul>". I don't think it is practical to avoid this, since otherwise a whole separate copy of all the values would need to be kept in Command.
• Does not handle arg0 on Unix. This can be awkward to support in get_args and is rarely used. I figure if someone really wants it, it can be added to CommandExt as a separate method.
• Does not offer a way to detect env_clear. I'm uncertain if it would be useful for anyone.
• Does not offer a way to get an environment variable by name (get_env). I figure this can be added later if anyone really wants it. I think the motivation for this is weak, though. Also, the API could be a little awkward (return a Option<Option<&OsStr>>?).
• get_envs could skip "cleared" entries and just return &OsStr values instead of Option<&OsStr>. I'm on the fence here. My use case is to display a shell command, and I only intend it to be roughly equivalent to the actual execution, and I probably won't display None entries. I erred on the side of providing extra information, but I suspect many situations will just filter out the Nones.
• Could implement more iterator stuff (like DoubleEndedIterator).

Original issue below

The the std::process::Command builder is useful for building up a Command in a cross-platform way. I've found that it would be useful to extract the name, args, environment, etc. of a Command they have been set.

There are at least two places in the Rust compiler that would benefit from such an API. Instead, the authors have had to resort to wrappers instead of using Command directly.

https://github.com/rust-lang/rust/blob/master/src/tools/compiletest/src/runtest.rs#L1527

https://github.com/rust-lang/rust/blob/master/src/librustc_trans/back/command.rs

[lukaslueg]

Related to #42200

[mackwic]

Let's say I am ok to implement this, how should I proceed ? Directly with a PR or with an RFC first ?

[dtolnay]

Seems reasonable. I would be interested in seeing this explored in a PR.

[ebkalderon]

I am working on a crate that would directly benefit from this. Is anyone still working on this feature? If not, would someone mind mentoring me to open a PR?

[ebkalderon]

Also, I have a tangential question not related to this issue, but might be raised as a result of introducing this feature: is there any way in the standard library to check whether a given std::process::Stdio is set to inherit, piped, or null? If we add the ability to extract these fields from the Command builder, I think it makes sense to add the ability to inspect the values at run-time as well.

[ehuss]

This is now implemented and available on nightly, I have updated the original description with details.

[kevleyski]

I feel this would be generally useful for debugging Command processes

[lf-]

@rust-lang/libs Can we stabilize this? It seems like the remaining concerns listed would not require API break to change.

[oli-obk]

cc @rust-lang/libs

[yaahc]

@rfcbot merge

[rfcbot]

Team member @yaahc has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se
• @sfackler
• @yaahc

Concerns:

• arg0 resolved by Provide an API to extract fields from Command builder #44434 (comment)
• preprocessing resolved by Provide an API to extract fields from Command builder #44434 (comment)
• use-case resolved by Provide an API to extract fields from Command builder #44434 (comment)

Once a majority of reviewers approve (and at most 2 approvals are outstanding), 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.

[yaahc]

Checking off sfacklers checkbox on the FCP since he left the libs team.

[joshtriplett]

@rfcbot concern arg0

I'm concerned about the potential for confusion introduced by indexing args without account for arg0.

[joshtriplett]

On Wed, Jul 28, 2021 at 01:43:46PM -0700, Eric Huss wrote: > I think it'd be entirely reasonable for the Debug impl on Command to print environment variables and anything else that has been set. I tested that out with bootstrap for a while, and the experience was pretty terrible. The amount of verbosity was overwhelming, and the tool usually wants more fine-grained control over what is exactly displayed (like no `-v`, `-v`, `-vv`). I personally would not like to see the Debug impl change until a long while after something like these getter methods are stabilized to give authors a chance to implement a better display. There is a separate issue #42200 for changing the Debug impl.

That sounds completely fair; thanks for clarifying!

[RalfJung]

Doing a read-modify-write of LD_LIBRARY_PATH doesn't seem ideal, especially if it needs to be repeatedly re-parsed. I agree that that shouldn't be open-coded, which seems like the issue with #85959. But I feel like that use case would work just as well by having a wrapper on Command that separately tracks a Vec, allows adding to that Vec, and overwrites the environment variable. That would factor out the modification code into one single place.

No (re-)parsing is necessary; adding a path to LD_LIBRARY_PATH is as trivial as prepending <new_path>: to the existing path (possibly with a special case for when LD_LIBRARY_PATH was not set at all yet).

I don't think one should have to write a wrapper for this. This kind of usecase should be supported out-of-the-box.

I tested that out with bootstrap for a while, and the experience was pretty terrible. The amount of verbosity was overwhelming, and the tool usually wants more fine-grained control over what is exactly displayed (like no -v, -v, -vv). I personally would not like to see the Debug impl change until a long while after something like these getter methods are stabilized to give authors a chance to implement a better display. There is a separate issue #42200 for changing the Debug impl.

We can provide at least 2 levels of verbosity with {:?} and {:#?}... but yeah that is still not terribly flexible.

[joshtriplett]

@rfcbot resolve use-case

[joshtriplett]

@rfcbot resolve preprocessing

[ChrisDenton]

Am I right in thinking that Command was originally designed as being, essentially, a function call with optional arguments (or at least the Rust equivalent)? So its use as a container type is basically being retrofitted?

[joshtriplett]

I've resolved two of the three concerns.

I also want to confirm, here: the only plan is to support get_args which iterates over the arguments, but not to have something like get_arg(index: usize)? Is there a planned API for changing a specific argument by index?

As long as there's no API that accepts a numeric "index", I'll resolve the arg0 concern as well. I just want to make sure that anything accepting an "index" uses 0 to mean arg0, and 1 to mean the first command-line argument.

[Amanieu]

My understanding is that we don't want to support any of these APIs because that is not the point of Command. As @ChrisDenton said, it's a function call with optional arguments, not long-term data storage.

There is some desire to support an extended Debug impl that also prints environment variables for logging purposes, but I feel that this should be a separate issue.

[RalfJung]

As @ChrisDenton said, it's a function call with optional arguments, not long-term data storage.

In building that function call incrementally (which is a common usecase for builder patterns), some amount of read access is required, as I laid down above. This has nothing to do with using Command as long-term storage.

[joshtriplett]

@rfcbot resolve arg0

[rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

The RFC will be merged soon.

[mathstuf]

Does not offer a way to get an environment variable by name (get_env). I figure this can be added later if anyone really wants it. I think the motivation for this is weak, though. Also, the API could be a little awkward (return a Option<Option<&OsStr>>?).

Not that it affects this PR, but something like this would be fine I think:

struct ProcessEnvSetting<'a> {
  Inherit,
  Unset,
  Set(&'a OsStr),
}

impl<'a> ProcessEnvSetting<'a> {
  // Convenience method.
  fn value_in_child(&self) -> Option<Cow<'a, OsStr>> {
    // query current environment if `Inherit`
  }
}

env_clear could then set an internal flag to return Unset rather than Inherit for any variable not explicitly set.

[JohnTitor]

Triage: The feature has been stabilized by #88436, closing as complete.