Skip to content
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

feat: add env to activation table #1156

Merged
merged 9 commits into from May 24, 2024

Conversation

ruben-arts
Copy link
Contributor

@ruben-arts ruben-arts commented Apr 10, 2024

Note

I'm not a 100% happy with the fact that this doesn't work with environment variables that already exist. For example:
I would expect it to expand variables if you would do:

[activation.env]
PATH = "bla/bli/blu:$PATH"
PATH_TO_XACRO = "$PIXI_PROJECT_ROOT/.pixi/envs/default/share/ros/bla/bli/xacro.xacro

But like the scripts, this also isn't cross platform/shell.

That said we made the decision to clean this up and release it as a first version so users that can use it will benefit from it and it would give us more time to come up with a good solution.

@ruben-arts
Copy link
Contributor Author

@wolfv What do you think? Merge this already (after adding docs) or figuring out a better parsing solution for the variables?

@ruben-arts
Copy link
Contributor Author

After merging #972 This feature is to simple and will cause confusion

@ruben-arts
Copy link
Contributor Author

@baszalmstra Do you have some smart ideas on how to easily and in a cross-platform way, add these env vars?
Specifically reusing existing variables:

[activation]
env = { PATH = "$PIXI_PROJECT_ROOT/extra/path:$PATH", MODEL_LOCATION = "$PIXI_PROJECT_ROOT/models"}

One of my ideas would be parsing the strings ourself and use rusts std::env::var(), what do you think about that?

@baszalmstra
Copy link
Contributor

@baszalmstra Do you have some smart ideas on how to easily and in a cross-platform way, add these env vars? Specifically reusing existing variables:

Yeah that could work but its a little bit more tricky because on Windows the separator for the PATH variable is not :' but ;` so you'd have to normalize that too. How do other task executors do this? Did you check taskfile?

@ruben-arts
Copy link
Contributor Author

Will have to try it out as they seem to just use the direct naming but they do support taskfiles per OS.

@beenje
Copy link
Contributor

beenje commented May 11, 2024

After merging #972 This feature is to simple and will cause confusion

Sometimes you really want some variables per environment.
It can be done with activation script, but I personally don't like having to add 2 extra scripts (.sh and .bat) to the repo just for that. Could also need more scripts in case of multi-environments. It's much nicer to be able to keep that in the pixi.toml file.

I currently use the variables per task but when you have several tasks using the same variable(s), it becomes quite verbose. And you might want a variable to be set without defining a specific task.
I think having the same feature as variable per task but per env would be a nice addition. I also really like that those are default you can overwrite from the shell.

@ruben-arts
Copy link
Contributor Author

He @beenje, Thanks for the input. I completely agree with you. Still need to get back to this PR to test the above discussed change.

src/project/manifest/activation.rs Outdated Show resolved Hide resolved
src/project/manifest/feature.rs Outdated Show resolved Hide resolved
@ruben-arts ruben-arts marked this pull request as ready for review May 23, 2024 07:21
@ruben-arts ruben-arts enabled auto-merge (squash) May 24, 2024 15:20
@ruben-arts ruben-arts merged commit c1a9ca0 into prefix-dev:main May 24, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants