How to handle modifications to the environment variables?

[tavianator]

Commands inherit the parent's environment variables by default. Right now we check the size of the environment when you create a Command, but it can be modified in the meantime:

    #[test]
    fn test_env() {
        let mut cmd = Command::new("/bin/echo");

        // Add as many arguments as possible                                                                                                                                                                                                                                    
        while cmd.try_arg("foo").is_ok() {}

        for i in 0..1024 {
            std::env::set_var(format!("VAR{i}"), "VALUE");
        }

        assert!(cmd.status().unwrap().success());
    }

test tests::test_env ... FAILED

failures:

---- tests::test_env stdout ----
thread 'tests::test_env' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 7, kind: ArgumentListTooLong, message: "Argument list too long" }', src/lib.rs:204:30

We can document that std::env::set_var() invalidates the calculations I guess. But we'll still have to think about this to implement try_env() if we want it.

[sharkdp]

Thank you for bringing this up.

I guess there are several ways to solve this.

• We could just document it for now
• We could document it and add an additional check. As you suggested, we determine the size of the environment upon construction of the argmax::Command (via size_of_environment) and store it internally. When spawn/output/… are used to actually run the command, we make sure that the current size of the environment is still the same. If not, we either assert! or even panic! (since this is a misuse of the library) or return a proper io::Error (?).
• We could store the entire (current) environment upon construction of the argmax::Command. Later on, when running the actual command, we could run self.inner.env_clear() and self.inner.envs(self.stored_envs). This way, there is no risk of external modification. But we would have to document the changed behavior with respect to std::Command.

I think the last approach would also allow us to implement try_env(). Are there any downsides to this?

[tavianator]

I like the third option. The only disadvantage is it makes it slightly incompatible with std::process::Command.

[Freaky]

For command-limits I have a new_capture_env constructor so the caller can decide if they want to pay the overhead of copying the environment or not. If they don't do that and modify the global environment anyway, shrug.

I'm not going to make everyone pay the cost of double-checking or double-storing just on the off chance someone overflows the reserved space with new env data. Just let the OS complain if it's too long.