feat: add support for PIXI_ENVIRONMENT_NAME and PS1 prompt modification #4101
Conversation
| @@ -17,12 +17,12 @@ def test_concurrent_exec(pixi: Path, dummy_channel_1: str) -> None: | |||
| futures = [ | |||
| executor.submit( | |||
| verify_cli_command, | |||
| [pixi, "exec", "-c", dummy_channel_1, "dummy-f"], | |||
| [pixi, "exec", "-c", dummy_channel_1, "--", "dummy-f"], | |||
There was a problem hiding this comment.
The feature is not important to introduce a breaking change in our CLI.
I don't really understand though why this change is necessary, can you explain the problem to me?
There was a problem hiding this comment.
On my windows machine + github ci it wouldn't pass before i added the seperator between the flags for command, i think it was because of clap's own problem. But then again that was when the backend was down and it was giving multiple errors that could be unrelated to it (though that change solved at least 2 test errors that is related to my pr, again due to clap's own problems)
But now, since i also added default feature being false to clap and made new bool flag for clap to read-process the entire thing, maybe i can try without these changes.
| // On Windows, when using cmd.exe or cmd, we need to set environment variables in a special way | ||
| // because cmd.exe has its own environment variable expansion rules | ||
| if cfg!(windows) && (command.to_lowercase().ends_with("cmd.exe") || command == "cmd") { | ||
| let mut env = std::env::vars().collect::<HashMap<String, String>>(); | ||
| env.extend(activation_env); | ||
| cmd.envs(env); | ||
| } else { | ||
| cmd.envs(activation_env.iter().map(|(k, v)| (k.as_str(), v.as_str()))); | ||
| } |
There was a problem hiding this comment.
I don't understand this code.
For which situation is it necessary?
There was a problem hiding this comment.
Otherwise we are having problems with windows not recognizing and not properly setting ps1 env variables (had to build in debug mode and manually test it to check that one out)
There was a problem hiding this comment.
I also dont understand this code. Maybe you can clarify what is different between windows and other cases?
There was a problem hiding this comment.
on windows, the prompt modification variables (_PIXI_PROMPT, PROMPT) doesnt work unless we explicitly pass all environment variables to the child process which i only found out from my testing with pixi exec --with dummy-b --with dummy-c -- cmd /c "echo %PIXI_ENVIRONMENT_NAME%" (manually) should i add it as comment to code?
It gave me multiple errors with the existing code and just a few env variables and a flag such as:
But ultimately it was the --with flag that made me write more code on this and made me spent a lot of time on this, i was having an incredibly weird time with that flag combined with specs names. |
|
|
||
| activation_env.insert("PIXI_ENVIRONMENT_NAME".into(), env_name.clone()); | ||
|
|
||
| if !args.no_modify_ps1 && std::env::current_dir().is_ok() { |
There was a problem hiding this comment.
Why is it important that there is a current_dir?
There was a problem hiding this comment.
Purely to prevent an UX issue that would give broken prompts (supposing the directory is in temp or gets deleted etc. during running and cmd giving errors for that.) I think its pretty low-cost as well but i can remove it if you want
closes #3999
hopefully tests are enough, i also tested manually and things seemed to pass correctly. though we cannot use the specs flag with multiple packages due to clap. on that note we could set the disable flag to include disabling specs as well but i will leave it open for that.
but now we correctly set env variables alongside with ps1 prompt with packages via specs. there is also a flag now that is used to disable this. default behavior is true and env and ps1 prompt is set by default.