Fix duplicated PATH entries

[chansuke]

Fixes #2848

[rbtcollins]

This is going to need a test case, I suggest a unit test of some sort close to the code, since the external contract is only changing in the manner of the duplication, and getting at this will be a little tricky.

[chansuke]

@rbtcollins Thanks for your comment. I will add some unit test.

[chansuke]

I am trying to add a unit test in tests/cli-misc.rs but it gives panicked at 'No process instance' error. How can I get the result of cmd.env(name, new_value); in the test case?

    let a = OsString::from("/home/a/.cargo/bin");
    let path_a = PathBuf::from(a);
    let b = OsString::from("/home/b/.cargo/bin");
    let path_b = PathBuf::from(b);
    let c = OsString::from("/home/c/.cargo/bin");
    let path_c = PathBuf::from(c);

    let path_entries = vec![path_a, path_b, path_c];
    let mut cmd = Command::new("test");
    // add duplicated path
    env_var::prepend_path("/home/c/.cargo/bin", path_entries, &mut cmd);
    // trying to get the result of `prepend_path()` with `get_envs()` because `prepend_path` itself doesn't return any value.
    let envs: Vec<(&OsStr, Option<&OsStr>)> = cmd.get_envs().collect();
    // gives `panicked at 'No process instance'` error
    assert_eq!(envs, &[(OsStr::new("wip"), Some(OsStr::new("wip")))]);

[kinnison]

Unit tests belong in the crate, not in tests/ which is where we have our integration tests.
There is the TestProcess struct in the currentprocess module which is provided for this purpose. You can see its use in a number of unit tests throughout the src/ tree. It should be fairly easy to deduce how to construct an appropriate test, but if you can't, then I'm sure Robert (or I) can help further.

[chansuke]

I had a wrong understanding. I will try to fix it. Thanks so much for your comment.

[chansuke]

I'm using cmd.get_envs(), an unstable function to check the result of prepend_path, but it fails clippy check on CI. Is there any workaround for this?

[rbtcollins]

Sorted PATH entries changes the semantic meaning of them: they are not a set, they are a sorted list, because 'bash' in a lower-index entry wins over 'bash' in a higher index entry.

[rbtcollins]

one way you could avoid using this unstable method is to have the code you're testing depend on a trait defined in this module.

Implement it once for Command, and once for a trivial test struct (or use mockall), and then you can assert on what the set env is, rather than interacting with Command at all.

[chansuke]

I'm trying to reimplement the unit test but I'm stuck on this part. Which trait should I depend on?

a trait defined in this module.

[rbtcollins]

Define one:

trait ProcessEnvs {
    type Item;
    type Iter: Iterator;
    fn get_envs(&self) -> Self::Iter<Item=Self::Item>;
    fn env<K, V>(&mut self, key: K, val: V) -> &mut Self
where
    K: AsRef<OsStr>,
    V: AsRef<OsStr>;
}

And then in your test you can just use a mock (e.g. with mockall) or a manually constructed test double. Change prepend/append _path to be generic over that trait, and provide an impl for Process.

[chansuke]

pub fn append_path(
    name: &str,
    value: Vec<PathBuf>,
    cmd: &mut dyn ProcessEnvs<Item = Command, Iter = vec::IntoIter<Command>>,
) {

I am stack on the trait ProcessEnvs cannot be made into an object error. I tried to separate the trait but it didn't work. This is the latest commit.

[rbtcollins]

I think this will work but I have a couple of suggestions:

• append_path should be changed likewise to prevent a regression in future.
• value is itself a vec but not deduped
• perhaps a helper function that takes two vecs and returns a unified vec taken in-order from vec 1 and vec 2 and with all duplicates not added to the output.

[chansuke]

@rbtcollins Thanks so much for your suggestion. I will fix this.

[chansuke]

Thanks for the support. I am stuck for about a month and would like to close it.

[rbtcollins]

Sorry I didn't realise you're stuck. https://internals.rust-lang.org/t/generic-methods-over-object-safe-traits/6774 is the thing to read to understand this - and the reason you needed trait objects is because you didn't make prepend_path generic over an implementation of ProcessEnvs. I've got a fix - should be pushed up in a minute.

However the patch fails tests now :)

[rbtcollins]

I've also rebased all the incremental commits together - there isn't enough here for a large commit history.
@chansuke do you want to finish this off, or should someone else do that?

[dmartin]

@chansuke @rbtcollins Hi, I don't have much experience with rustup's internals nor the time to dig too deeply into them right now, but I have been affected by PyO3/pyo3#1708 and would like to help push this across the finish line if possible.

If the only thing blocking this PR from being merged is the failing test on Windows, I submitted a PR with a fix here (though I'm not sure whether it's considered okay to call .unwrap() as I did within tests). Let me know if I can do anything else to help get this merged in!

[chansuke]

@rbtcollins @dmartin
sorry for the late reply I'd like to finish this.

[gussmith23]

Wondering what the status of this is -- seems to be blocking PyO3/pyo3#1708 (among other things)

[chansuke]

I have requested the review to @rbtcollins

[klimburg]

I too have come here from PyO3/pyo3#1708

@rbtcollins / @chansuke what is needed to push this to completion? Is there someone else who can review?

[rbtcollins]

wow I had forgotten this. thank you. I'm sure we could code golf it more but the essentials are fine:

• it fixes the bug
• it has a test to prevent accidental recurrence

[rbtcollins]

I'm quite confident the android failure is unrelated; will merge this.