rustup

rustup copied to clipboard

Fixes #2848

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.

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

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")))]);

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.

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

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?

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.

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

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

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 :)

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?

@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 https://github.com/PyO3/pyo3/issues/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!

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

Wondering what the status of this is -- seems to be blocking https://github.com/PyO3/pyo3/issues/1708 (among other things)

I have requested the review to @rbtcollins