Add std::process::Command::envs()

[zackw]

Command::envs() adds a vector of key-value pairs to the child
process environment all at once. Suggested in #38526.

This is not fully baked and frankly I'm not sure it even works, but I need some help finishing it up, and this is the simplest way to show you what I've got. The problems I know exist and don't know how to solve, from most to least important, are:

• I don't know if the type signature of the new function is correct.
• The new test might not be getting run. I didn't see it go by in the output of x.py test src/libstd --stage 1.
• The tidy check says process.rs:402: different `since` than before which I don't know what it means.

r? @brson

[rust-highfive]

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.

[achanda]

The tidy check error will go away if you rename the feature to something else. A feature called process has been declared to be stable sine 1.0.0, in the same file. You are trying to use the same name. The error message from tidy can be better though.

[zackw]

@achanda

The tidy check error will go away if you rename the feature to something else.

Ah, I misunderstood how that works. I thought that I was expressing "this was added to the 'process' feature in version 1.16". It sounds like you're saying the contents of a 'feature' are permanently frozen once laid down. Is that right? Is it necessary to give this its own feature tag, then, and what would an appropriate name be? If not, what should be done instead?

(Please bear with me; I haven't even fully learned the language itself, never mind the procedures for adding stuff to the library. Much of the relevant documentation only talks about bigger "new features" than adding a single function to an existing module.)

[aturon]

@zackw

Is it necessary to give this its own feature tag, then, and what would an appropriate name be?

Yep. A good name would be command_envs.

[zackw]

@aturon Thanks. That should be fixed now, and I also figured out how to make the test actually run, so my only remaining concern is I need someone to check whether I got the new function's type signature right.

[achanda]

@zackw you should be able to run the tests by doing ./x.py test src/test/run-pass. (sorry I missed your last comment!)

[zackw]

@achanda I figured that out for myself this morning, but thanks.

[aturon]

cc @rust-lang/libs, new proposed unstable API.

[aturon]

@zachW We would want to land this API as unstable initially, to give it some time for feedback before committing to it fully.

[aturon]

Regarding the function signature, you might consider generalizing to take an IntoIterator rather than just a slice.

[alexcrichton]

Looks good to me, and yeah I'd be in favor of generalizing to IntoIterator rather than only taking a slice. We could also take this opportunity to generalize args as well.

[zackw]

I'd be in favor of generalizing to IntoIterator rather than only taking a slice

I agree with this, but I have gotten stuck trying to implement it. Consider

use std::ffi::OsStr;
use std::collections::HashMap;
use std::env;

fn test<I, K, V>(x: &I)
    where I: IntoIterator<Item=(K, V)>,
          K: AsRef<OsStr>, V: AsRef<OsStr>
{
    for (ref k, ref v) in *x {
        println!("{:?}: {:?}", k.as_ref(), v.as_ref());
    }
}

fn test_vec() {
    let filtered_env : Vec<(String, String)> =
        env::vars().filter(
            | &(ref k, _) |
            k == "TERM" || k == "TZ" || k == "LANG" || k == "PATH"
        ).collect();

    test(&filtered_env);
}

fn test_hashmap() {
    let mut map = HashMap::new();
    map.insert("a", "foo");
    map.insert("b", "bar");
    map.insert("c", "baz");

    test(&map);
}

fn main() {
    test_vec();
    test_hashmap();
}

test has the right external signature, I think, but it fails to compile with

error[E0507]: cannot move out of borrowed content
 --> test.rs:8:27
  |
8 |     for (ref k, ref v) in *x {
  |                           ^^ cannot move out of borrowed content

Shouldn't it be possible to take a read-only iterator over a borrowed collection?

[alexcrichton]

@zackw oh I think you'd take just I instead of &I

[zackw]

@alexcrichton It's more subtle than that. If we can't make this work for both &[(&str, &str)] and HashMap<&str, &str> then I don't think it buys us much of anything. But...

use std::collections::HashMap;

fn test<I, K, V>(x: I)
    where I: IntoIterator<Item=(K, V)>,
          K: AsRef<str>, V: AsRef<str>
{
    for (ref k, ref v) in x {
        println!("{}: {}", k.as_ref(), v.as_ref());
    }
}

fn main() {
    let vc = vec![
        ("a", "foo"),
        ("b", "bar"),
        ("c", "baz")
    ];
    test(&vc);

    let map: HashMap<&str, &str> = [
        ("d", "blurf"),
        ("e", "quux"),
        ("f", "xyzzy")
    ].iter().cloned().collect();
    test(&map);
}

fails to compile with

error[E0271]: type mismatch resolving `<&std::vec::Vec<(&str, &str)> 
    as std::iter::IntoIterator>::Item == (_, _)`
  --> test.rs:18:5
   |
18 |     test(&vc);
   |     ^^^^ expected reference, found tuple
   |
   = note: expected type `&(&str, &str)`
   = note:    found type `(_, _)`
   = note: required by `test`

I asked about this on Stack Overflow and was told that an adaptor trait is required. I'm hesitant to make up a one-off adaptor trait for a standard library function, though; it seems like if we're going to go that way, something like E_net4's KeyValue trait should be designed and given as many impl's as possible... and I feel like we're headed deep into the land of mission creep here. 😟

[alexcrichton]

Oh right yeah that's a very good point that slices by default yield references, so they wouldn't work here. Ok I think we can scratch that idea then and we should stick with slices, sorry for the runaround!

[zackw]

Thinking about it some more, I can make the above program work if I change it like so...

    let vc = vec![
        ("a", "foo"),
        ("b", "bar"),
        ("c", "baz")
    ];
-   test(&vc);
+   test(vc.iter().map(|&(ref k, ref v)| (k, v)));

Abstractly, having this API expect the Item type you get from HashMap::iter() seems like the most natural thing, and I can also feed it env::vars() directly. I only wanted &[(&str, &str)] to work in the first place because I had a program that did

let mut child_env: Vec<(String, String)> =
    env::vars().filter(|&(ref k, _)|
        k == "TERM" || k == "TZ" || k == "LANG" || k.starts_with("LC_")
    ).collect();

and I could perfectly well make that child_env: HashMap<String, String> instead.

So maybe we should go ahead with IntoIterator after all. What do you think?

[alexcrichton]

Yeah it's true that you can still go from an iterator of a references-to-tuples to an iterator of tuples relatively easy, but it's definitely not the most ergonomic. I'd be fine either way in terms of this API, I think the use cases should just drive it (e.g. does it come from a vec or hash map more often?

[zackw]

I think the use cases should just drive it (e.g. does it come from a vec or hash map more often?

I don't know the answer to that question (and I'm not sure anyone does, yet); the thing that I wanted it for doesn't particularly care.

The revision I just pushed uses the version that works with maps but not vec-of-tuples, and marks it unstable. It also includes the generalization of args() to iterators, which I hope can be done without destabilizing args() - unlike envs() we don't have two subtly different Item signatures to choose from, so it should Just Work with more stuff than it used to.

[BurntSushi]

I think I'm OK with an API like this.

Should the generalization of args get a crater run? These types of changes have a habit of introducing breakage because of inference failures, although I don't know how likely those are in this case.

[aturon]

@BurntSushi Yeah, it's probably a good idea.

[brson]

I've started crater

• crater Add std::process::Command::envs() #38856 (comment)
  • zackw:process-envs
  • https://github.com/zackw/rust
  • before 74e5b7d
  • after 2580950
  • inspector link: https://tools.taskcluster.net/task-inspector/#OWvDlzZPSaW0ayNwjhHN6A
  • inspector link: https://tools.taskcluster.net/task-inspector/#eQoYEkcHQ1y-D-7BrGJ_uw

[brson]

Toolchains built, testing crates.

[brson]

Crater says.

[zackw]

Five of the six failures appear to all have the same basic cause: calling args with an &&[stringy] used to work, but doesn't with the changed signature. The lsio failure is one of the simplest: this code used to compile and now doesn't...

pub fn run_args(cmd: &str, args: &[String], shell: bool) -> Result<(Output)> {
    let output;

    if args.len() > 0 {
        if !shell {
            output = try!(Command::new(cmd).args(&args).output());
        } else {
    ...

The & in &args here is unnecessary, but an easy mistake to make. I would abstractly be okay with declaring this code buggy.

skia-sys' build system is too convoluted for the problem to be obvious, but if

mkdir: cannot create directory '/home/crate/target/debug/build/skia-sys-e80b95b369ab0855/out/src/ports': No such file or directory
make: *** [/home/crate/target/debug/build/skia-sys-e80b95b369ab0855/out/src/ports/SkAtomics_sync.h] Error 1

is the actual primary failure (which I'm not certain of) then I don't see how it can be caused by this change.

[aturon]

@zackw Thanks for the analysis!

That fallout doesn't seem too bad (and this kind of change is definitely permitted under our semver guidelines). That said, it'd be good to try to make PRs against these repos before landing the change.

Otherwise, I'm 👍 on going forward. @alexcrichton?

[alexcrichton]

Sounds good to me!

[aturon]

@bors: r+

Thanks much, @zackw! If you're up for making PRs against the root regression repos from crater, that's always appreciated (but not required).

[bors]

📌 Commit 2580950 has been approved by aturon

[zackw]

If you're up for making PRs against the root regression repos from crater

I'm sorry, I don't have time to do that this week.

[bors]

⌛ Testing commit 2580950 with merge 94d4589...

[bors]

☀️ Test successful - status-appveyor, status-travis
Approved by: aturon
Pushing 94d4589 to master...

[Ms2ger]

ring is fixed in briansmith/ring#430

[rustonaut]

just as a side note, the example in the documentation is not displayed as code block for some reason (instead as text with three ` at begin and end... At last with the documentation you get shipped with rustup/nightly.