fix: Override environment variables based on priority

[magentaqin]

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 in executable_task.rs.
Previously, std::env::var has deterministic issues. So I updated get_export_specific_task_env to let it explicitly receive the parameter command_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:

{
  "COMMAND_ENV": {...},
  "TASK_SPECIFIC_ENV": {...}
}

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 in activation.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.

[magentaqin]

I find the priority order of activation scripts and activation.env is changed by activator.run_activation, which is the function run_activation in rattler_shell crate. Details can be seen here: https://github.com/prefix-dev/pixi/pull/3940/files#diff-f84977a6e7fdf8147b0d4b75f15a45112eff4507146389b1fb84e4647e0489c8

As required, we need to change the priority order. So I need to pass the second parameter to run_activation to override the environment variable.

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 run_activation is called, I printed the current activator. It reads environment variable I defined in activation.env, that is "CMAKE_ARGS": "-DCMAKE_BUILD_TYPE=Debug"

After run_activation is called, I printed activator again. It reads environment variable I defined in activation_scripts, that is CMAKE_ARGS": "from_activation_script

Expected behavior:
After run_activation is called, activator should be CMAKE_ARGS": "Foo123, as I passed it as second parameter and overrides it.

[ruben-arts]

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:

~/envs/args  
➜ pixi run start
✨ Pixi task (start): echo $ARG
default

~/envs/args  
➜ ARG=hoi pixi run start
✨ Pixi task (start): echo $ARG
hoi

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

➜ pixi run problem "hello 1" "test 2" "test 3"
✨ Pixi task (problem): echo hello 1 test 2 test 3
hello 1 test 2 test 3

The problem arrises when you only want to override the 3 option. you need to do:

➜ pixi run problem "" "" "test 3"              
✨ Pixi task (problem): echo   test 3
test 3

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.

Proposal

I think we can already improve this experience with a few convincing features:

• Named arguments, in the cli, have a way to specify which argument you want to modify.
• Extend the minijinja functions with the ability to read env vars: args = [{arg = "name", default = "{{ env.ARG or "default_value" }}"}]

@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.

[magentaqin]

Hi, @ruben-arts! Thanks for your suggestion.

My Question

1. Could you please explain more about the [tasks.trick] problem?

[tasks.trick]
cmd = "echo {{ ARG }}"
args = [{arg = "ARG", default = "$ARG"}]
`

If you run ARG=123 pixi run trick
✨ Pixi task (trick): echo $ARG

123

From my understanding, this satisfies our expectation.

2. As this PR is to solve priority issues, for the CLI optimization part, shall we open another PR and another issue to solve it separately?

My Proposal

Based on your proposal, my proposal is:
1.Support named arguments and alias for named arguments.

[tasks.foo]
cmd = "echo {{ input }} {{ output }} {{ verbose }}"
args = [
    {name = "input", alias = "i", default = "Input file"},
    {name = "output", alias = "o", default = "out.txt"},
    {name = "verbose", alias = "v", default = false}
]

Then when you run like this:

pixi run foo -i input.txt
✨ Output
input.txt "out.txt" false

2.Support both default value and environment variable.

[tasks.foo]
cmd = "echo {{ ARG }}"
args = [{name = "ARG", default = "${ARG:-Guest"}}]

Then when you run like this:

ARG=123 pixi run foo
✨ Output
123
Then when you run like this:
pixi run foo
✨ Output
Guest

3.Support required field to force the user set value to override and support interactive prompts.(prompt, type and options)

[tasks.deploy]
cmd = "deploy {{ env }} {{ version }} {{ confirm }}"
args = [
    {
     name = "env",
     required = true, 
     prompt = "Deployment environment", 
     type = "select", 
     options = ["test", "staging", "production"]
   },
   {name = "version", default = "latest", prompt = "Version to deploy"},
   {name = "confirm", type = "bool", required = true, prompt = "Confirm deployment"}
]

$ pixi run deploy
✨ Output
Deployment environment: production
Version to deploy (latest): v1.2.3
Confirm deployment (y/N): y
Running: deploy production v1.2.3 true

[Hofer-Julian]

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

[Hofer-Julian]

Let's clean up the tests a bit. Will then focus on the actual implementation.

Good work so far!

[zelosleone]

I think there are more than one problem here for windows and in general for the logic of priority listing.

• I don't think we should show all the system exports in output, there was a similar case with debugging mode being added in rattler-build and this looks very similar to that in terms of output, since we don't need all of them to be seen and just task-related ones will be enough. This is one of the problems for windows, as deno shell cannot parse everything related to windows correctly (programfiles env names etc.) not to mention that many of them at once, its having hard time even with a special logic to escape quotes.
• Additionally, I think the order for placing the function that orders the tasks in priority should be moved to activation state as an async function. Something like:

pub async fn get_task_env_for_task(
    task: &Task,
    environment: &Environment<'_>,
    clean_env: bool,
    lock_file: Option<&LockFile>,
    force_activate: bool,
    experimental_cache: bool,
) -> miette::Result<HashMap<String, String>> {
    let mut env = get_task_env(
        environment,
        clean_env,
        lock_file,
        force_activate,
        experimental_cache,
    )
    .await?;
    if let Some(task_env) = task.env() {
        for (key, value) in task_env {
            env.insert(key.clone(), value.clone());
        }
    }

    Ok(env)
}

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.

• Additionally, tests should use .bat for windows (.ps1 extension will give errors)
• Also, to add to backwards tick problem of windows you can place

.replace('\\', "\\\\")
.replace('"', "\\\"")
.replace('$', "\\$");

etc. before exporting them as combined env variables (before export push str)

[magentaqin]

(^▽^)わくわく, @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 Task_Specific_Env map.
2. update tests using .bat for windows

Based on the above updates, the test pipeline on windows passed! 👏🏻

3. For the priority order handling logic, I think we need further discussion. To be more decoupled, I still think they should be put in executable_task module instead of activation module.
4. In the previous commits, I've tried escaping characters. But it has nothing to do with the error. As the old version of the functionality doesn't have escaping and there're a lot of escaping cases, I would suggest the task in a separate PR(...this PR hasn't been closed for a quite long time😭)

[lucascolley]

I think we should hold off on merging this until gh-4148 is in, as some niche uses of https://pixi.sh/latest/workspace/advanced_tasks/#environment-variables will not have a nice replacement otherwise (e.g. https://github.com/VSLAM-LAB/VSLAM-LAB/blob/9b3ad8edfb5493530b1466d4333c44c95080964e/pixi.toml#L9)

EDIT: never mind, I think we can get by with positional args there

[magentaqin]

I am afraid that this PR escapes env var so that it blocks expansion. This is something we recently had problems with, so I added a test for that. It is failing again on this PR: https://github.com/prefix-dev/pixi/actions/runs/16568792912/job/46855536683?pr=3940

Hi, Julian. I saw the test you added!
I understand that the original product needs for the PR is sort the priority: task.env > activation.env > activation.scripts > activation scripts of dependencies > outside environment variables.
But in your test, you added another case: [workspace.name]. I would suggest we put it in another PR instead of this PR.

[Hofer-Julian]

But in your test, you added another case: [workspace.name]. I would suggest we put it in another PR instead of this PR.

The test is about variable expansion and that needs to work before we can merge this PR :)

[magentaqin]

@Hofer-Julian So now the priority should be workspace > task.env > activation.env > activation.scripts > activation scripts of dependencies > outside environment variables? Workspace's priority is the highest, right?

[lucascolley]

@Hofer-Julian So now the priority should be workspace > task.env > activation.env > activation.scripts > activation scripts of dependencies > outside environment variables? Workspace's priority is the highest, right?

I don't think there is anything to do with priority that needs changing! The problem is that $PIXI_PROJECT_NAME is not being evaluated/expanded into its value. As you can see in the failing CI log, it is just being treated as a string, with $ included:

FAILED tests/integration_python/test_environment_variables.py::test_variable_expansion - AssertionError: 'The project name is expansion-test'
 not found in stdout:
 The project name is $PIXI_PROJECT_NAME

$PIXI_PROJECT_NAME should have been replaced with its value expansion-test.

So I think there is just a bug in the evaluation/expansion of env var values which has been introduced in this PR.

[lucascolley]

@Hofer-Julian do you know if we already have something like this elsewhere?

[Hofer-Julian]

At least after a quick look at our code base, I wouldn't be able to find it

[magentaqin]

@Hofer-Julian @lucascolley I think the doc would be better if we put it into environment_variables.md instead of advanced_tasks.md:

1. NOT all cases belong to tasks section
2. The topic is about "environment variables".
   Let me know your thoughts on it!

[lucascolley]

I think we should keep the migration guidance under tasks as it replaces what is made obsolete by this PR, and we can link out to the examples and full description of priority in the env var section. I can push a commit!

[lucascolley]

let me know if that looks okay to you @magentaqin !

[Hofer-Julian]

Awesome work @magentaqin!

Thanks also to @lucascolley for reviewing!

[magentaqin]

I think we should keep the migration guidance under tasks as it replaces what is made obsolete by this PR, and we can link out to the examples and full description of priority in the env var section. I can push a commit!

It looks good to me! Thanks!

[magentaqin]

Awesome work @magentaqin!

Thanks also to @lucascolley for reviewing!

Thanks @Hofer-Julian @lucascolley and @zelosleone for reviewing! ❤️