-
Notifications
You must be signed in to change notification settings - Fork 314
fix: Override environment variables based on priority #3940
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
fix: Override environment variables based on priority #3940
Conversation
I find the priority order of As required, we need to change the priority order. So I need to pass the second parameter to To test whether the override function works, I constructed a hash map called opt_map, it has the key "CMAKE_ARGS" and the value "Foo123". Before After Expected behavior: |
c963cd4
to
0886b67
Compare
This change will introduce an issue for a use case we supported previously. That is the use of environment variables as arguments for functions. e.g.: [tasks.start]
cmd = "echo $ARG"
env = { ARG="default" } Usage:
Users could define their task like that. With this change that use-case will break and will always have the default value. We currently support arguments. But these are not named, only positional. So to get something like this working: [tasks.problem]
cmd = "echo {{ ARG1}} {{ ARG2 }} {{ ARG3 }}"
args = [
{arg = "ARG1", default = "default1"},
{arg = "ARG2", default = "default2"},
{arg = "ARG3", default = "default3"}
] You need to run the following
The problem arrises when you only want to override the 3 option. you need to do:
I think before we break that other behavior we need to improve the ux of tasks. As there is no way to define named variables right now. There is one more trick people could use: [tasks.trick]
cmd = "echo {{ ARG }}"
args = [{arg = "ARG", default = "$ARG"}] This would bring back the use of environment variables. But it doesn't allow a user to set a default as the evaluation happens after the template replacement. ProposalI think we can already improve this experience with a few convincing features:
@Hofer-Julian & @magentaqin please let me know what you think. When we merge this we should see if it introduces really non-working setups and work to improve the UX of tasks as required. |
b6a5b74
to
9186613
Compare
Hi, @ruben-arts! Thanks for your suggestion. My Question
If you run
From my understanding, this satisfies our expectation.
My ProposalBased on your proposal, my proposal is:
Then when you run like this:
2.Support both default value and environment variable.
Then when you run like this:
3.Support
|
5c63837
to
c8bc900
Compare
e96b34a
to
c8bc900
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks promising at first glance.
Added a few comments for now, one of them might be the cause of your Windows struggles.
Will do a more extensive review next round
src/task/executable_task.rs
Outdated
"PIXI_PROJECT_ROOT", | ||
"PIXI_PROJECT_NAME", | ||
"PIXI_PROJECT_MANIFEST", | ||
"PIXI_PROJECT_VERSION", | ||
"PIXI_PROMPT", | ||
"PIXI_ENVIRONMENT_NAME", | ||
"PIXI_ENVIRONMENT_PLATFORMS", | ||
"CONDA_PREFIX", | ||
"CONDA_DEFAULT_ENV", | ||
"PATH", | ||
"INIT_CWD", | ||
"PWD", | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good that you thought of that!
src/task/executable_task.rs
Outdated
fn get_export_specific_task_env(task: &Task) -> String { | ||
// Append the environment variables if they don't exist | ||
/// Get the environment variable based on their priority | ||
fn get_export_specific_task_env(task: &Task, command_env: &HashMap<OsString, OsString>) -> String { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of converting it later on, just require HashMap<String, String> right away
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good suggestion!
src/task/executable_task.rs
Outdated
export.push_str(&format!("export \"{}={}\";\n", key, value)); | ||
// If task.env() and command_env don't have duplicated keys, simply export task.env(). | ||
if env.keys().all(|k| !command_env_converted.contains_key(k)) { | ||
println!("env.keys() {:?}", env.keys()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Get rid of this println
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, I'll remove it. I was thinking about debugging on the windows pipeline.
src/task/executable_task.rs
Outdated
} | ||
} else { | ||
// Env map | ||
let mut env_map: HashMap<&'static str, Option<IndexMap<String, String>>> = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let mut env_map: HashMap<&'static str, Option<IndexMap<String, String>>> = | |
let mut env_map: HashMap<&'static str, IndexMap<String, String>> = |
Is the Option
really necessary here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, because the value of TASK_SPECIFIC_ENVS
can be None, and the value of env_map
should stay the same. So I use Option
here.
src/task/executable_task.rs
Outdated
let should_exclude = override_excluded_keys.contains(key.as_str()); | ||
if !should_exclude { | ||
tracing::info!("Setting environment variable: {}=\"{}\"", key, value); | ||
// Platform-specific export format with proper escaping |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We didn't have any of this escaping beforehand. Why is it necessary now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to solve the windows pipeline error. Let me first remove it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this platform specific export format should be deleted and just
export.push_str(&format!("export \"{}={}\";\n", key, value));
will be enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let me try this!
manifest = tmp_pixi_workspace.joinpath("pixi.toml") | ||
script_manifest = tmp_pixi_workspace.joinpath("env_setup.sh") | ||
toml = f""" | ||
[project] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[project] | |
[workspace] |
We try to use workspace
everywhere nowadays
pixi-foobar = "*" | ||
""" | ||
test_script_file = """ | ||
#!/bin/bash |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work on Unix.
Something like this should work:
[target.unix.activation]
scripts = ["scripts/activate.sh"]
[target.win-64.activation]
scripts = ["scripts/activate.bat"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will only work on Unix.
Something like this should work:
[target.unix.activation] scripts = ["scripts/activate.sh"] [target.win-64.activation] scripts = ["scripts/activate.bat"]
good catch!
382b459
to
35cdb9f
Compare
acb9791
to
1499388
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's clean up the tests a bit. Will then focus on the actual implementation.
Good work so far!
[tasks.task] | ||
cmd = "echo $MY_ENV" | ||
[tasks.foo] | ||
cmd = "echo $MY_ENV" | ||
[tasks.foobar] | ||
cmd = "echo $FOO_PATH" | ||
[tasks.task.env] | ||
MY_ENV = "test456" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[tasks.task] | |
cmd = "echo $MY_ENV" | |
[tasks.foo] | |
cmd = "echo $MY_ENV" | |
[tasks.foobar] | |
cmd = "echo $FOO_PATH" | |
[tasks.task.env] | |
MY_ENV = "test456" | |
[tasks.task] | |
cmd = "echo $MY_ENV" | |
env = { MY_ENV = "test456" } | |
[tasks.foo] | |
cmd = "echo $MY_ENV" | |
[tasks.foobar] | |
cmd = "echo $FOO_PATH" |
this makes it a bit more readable IMO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed!
windows_toml = f""" | ||
[workspace] | ||
name = "test" | ||
channels = ["{dummy_channel_1}"] | ||
platforms = ["linux-64", "osx-64", "osx-arm64", "win-64"] | ||
[activation.env] | ||
MY_ENV = "test123" | ||
[target.unix.activation] | ||
scripts = ["env_setup.sh"] | ||
[target.win-64.activation] | ||
scripts = ["env_setup.bat"] | ||
[tasks.task] | ||
cmd = "echo $env:MY_ENV" | ||
[tasks.foo] | ||
cmd = "echo $env:MY_ENV" | ||
[tasks.foobar] | ||
cmd = "echo $env:FOO_PATH" | ||
[tasks.task.env] | ||
MY_ENV = "test456" | ||
[dependencies] | ||
pixi-foobar = "*" | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the difference to the unix_toml?
Can't we use the same toml here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's the compatible windows test pipeline issue, let me revert to the original one!
if platform.system() == "Windows": | ||
manifest.write_text(windows_toml) | ||
script_manifest.write_text(""" | ||
$env:MY_ENV = "activation_script" | ||
$env:FOO_PATH = "activation_script" | ||
""") | ||
else: | ||
manifest.write_text(unix_toml) | ||
script_manifest.write_text(""" | ||
#!/bin/bash | ||
export MY_ENV="activation_script" | ||
export FOO_PATH="activation_script" | ||
""") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need an if condition here.
Just write it in both cases
# Validate that without experimental it does not use the cache | ||
assert not tmp_pixi_workspace.joinpath(".pixi/activation-env-v0").exists() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Validate that without experimental it does not use the cache | |
assert not tmp_pixi_workspace.joinpath(".pixi/activation-env-v0").exists() | |
# Validate that without experimental caching it does not use the cache | |
assert not tmp_pixi_workspace.joinpath(".pixi/activation-env-v0").exists() |
# Enable the experimental cache config | ||
verify_cli_command( | ||
[ | ||
pixi, | ||
"config", | ||
"set", | ||
"--manifest-path", | ||
manifest, | ||
"--local", | ||
"experimental.use-environment-activation-cache", | ||
"true", | ||
], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really see why you'd set this config here. I feel the test is already pretty complicated as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove it!
verify_cli_command( | ||
[pixi, "run", "--manifest-path", manifest, "task"], | ||
stdout_contains="test456", | ||
env={"MY_ENV": "outside_ev"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env={"MY_ENV": "outside_ev"}, | |
env={"MY_ENV": "outside_env"}, |
verify_cli_command( | ||
[pixi, "run", "--manifest-path", manifest, "foo"], | ||
stdout_contains="test123", | ||
env={"MY_ENV": "outside_ev"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
env={"MY_ENV": "outside_ev"}, | |
env={"MY_ENV": "outside_env"}, |
# Test 3: activation.env > outside environment variable - should use activation.env | ||
verify_cli_command( | ||
[pixi, "run", "--manifest-path", manifest, "foo"], | ||
stdout_contains="test123", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stdout_contains="test123", | |
stdout_contains="test123", | |
stdout_excludes="outside_env" |
# Run an actual install | ||
verify_cli_command( | ||
[pixi, "install", "--manifest-path", manifest], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# Run an actual install | |
verify_cli_command( | |
[pixi, "install", "--manifest-path", manifest], | |
) |
That shouldn't be necessary
verify_cli_command( | ||
[pixi, "run", "--manifest-path", manifest, "foobar"], | ||
stdout_contains="activation_script", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am missing tests here, that environment variables that are set from the outside and are NOT overridden by anything inside Pixi still work as expected
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also choose the tests a bit different.
So you have this goal:
- outside environment variables
- activation scripts of dependencies
- activation.scripts
- activation.env
- task specific envs like tasks.start = {cmd = "…", env={ENV_VAR="SOME"}}
So add tests that 5>4, 4>3, 3>2, 2>1 and finally that 1 on its own works
Then you have everything covered
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't quite understand what you mean. Is the test order thing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I want you to add asserts that "activation scripts of dependencies" wins over "outside environment variables".
"activation.scripts" should win over "activation scripts of dependencies", and so on. If nothing else is set "outside environment variables" should be considered.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2728253#diff-e6813bccd5c3cad11b9c048122f69487eafb612de804912383e94995b10f188b I added the test '"activation scripts of dependencies" wins over "outside environment variables", and update the test order. Have a look!
src/task/executable_task.rs
Outdated
let expected_prefix = if cfg!(windows) { | ||
"env:FOO = \"bar\"" | ||
} else { | ||
"export \"FOO=bar\"" | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let expected_prefix = if cfg!(windows) { | |
"env:FOO = \"bar\"" | |
} else { | |
"export \"FOO=bar\"" | |
}; | |
let expected_prefix = "export \"FOO=bar\""; |
I think there are more than one problem here for windows and in general for the logic of priority listing.
Should be more than enough. This will also limit the COMMAND_ENV and TASK_SPECIFIC_ENVS additions where you can safely delete them and just get them from the activation/runtime instead.
etc. before exporting them as combined env variables (before export push str) |
(^▽^)わくわく, @zelosleone! Thanks for your great effort on this PR and nice suggestion.👍🏻 1.For the exported script, I agree with u. Now I update my strategy: only export the statement that has key in Based on the above updates, the test pipeline on windows passed! 👏🏻
|
304022c
to
b74990b
Compare
Description
This PR is created to solve Environment variable of tasks are broken when defined in surrounding scope , Unit tests and integration
What functionality changes I made
1.Change the priority of environment variables
Original Order(highest to lowest):
activation scripts > activation.env > activation scripts of dependencies > environment variables > task specific envs
Current Order(highest to lowest):
task specific envs > activation scripts > activation.env > activation scripts of dependencies > environment variables
2.What code changes I made
1)
get_export_specific_task_env
function refactor inexecutable_task.rs
.Previously,
std::env::var
has deterministic issues. So I updatedget_export_specific_task_env
to let it explicitly receive the parametercommand_env
.priority
array: store the keys of the environment variable map. For extension, as in the future, there may be other kinds of environment variable map passing in.env_map
map:and we can merge the map according to the priority order. Finally, we get the
export_merged
array and export all the environment variables to the shell.I also add escape logic and exclude environment variables that can't be overidden.
2)
run_activation
inactivation.rs
process the
activator.run_activation()
result and handling the overidden stuff.TO FIX
Test compatibility issues on windows platform, which causes the pipeline fails.