feat: pass enviroment variables during pypi resolution and install #818
Conversation
| lock_file_usage: LockFileUsage, | ||
| ) -> miette::Result<HashMap<String, String>> { | ||
| // Get the prefix which we can then activate. | ||
| get_up_to_date_prefix(environment, lock_file_usage, false, IndexMap::default()).await?; |
There was a problem hiding this comment.
Isn't something like this already done as a prerequisite for the pypi resolve?
There was a problem hiding this comment.
this is the old part of existing get_activation_env. I've extracted the part what I need in get_env_and_activation_variables, so I could call it without causing infinite recursion
There was a problem hiding this comment.
Awesome! Nice work on getting an actual test package for this! And good idea of using the arc and lock. My experience with those is limited @tdejager or @baszalmstra could you take another quick look?
src/project/environment.rs
Outdated
| @@ -35,6 +42,9 @@ pub struct Environment<'p> { | |||
|
|
|||
| /// The environment that this environment is based on. | |||
| pub(super) environment: &'p manifest::Environment, | |||
|
|
|||
| /// The cache that contains environment variables | |||
| vars: Arc<Mutex<HashMap<String, String>>>, | |||
There was a problem hiding this comment.
This variable might be lost if someone requests the environment again from the project instead of cloning this type. It might be better to store a hashmap in the project, indexed by environment name.
src/project/environment.rs
Outdated
| pub async fn get_env_variables(&self) -> miette::Result<HashMap<String, String>> { | ||
| let mut locked = self.vars.lock().await; | ||
|
|
||
| if !locked.is_empty() { |
There was a problem hiding this comment.
I think its never good practice (and very pythonic) to depend on something being empty as meaning that it wasnt set. Its better to wrap these things in an Option to provide more semantics to the type.
Additionally for this usecase you could use an async_once_cell.
| self.solve_group.environments.iter().map(|env_idx| { | ||
| Environment::new( | ||
| self.project, | ||
| &self.project.manifest.parsed.environments.environments[*env_idx], |
There was a problem hiding this comment.
I don't like the [] indexing too much because it can panic, I feel its better to handle this explicitly, by varing what applies in this case
- Panicking with a better error message, if it not being there is a program error
- Returning a Result
- Filtering out the environments that do not match.
There was a problem hiding this comment.
could you please suggest what would be the best scenario in this case?
I just reformatted this code so I'm unaware of solve_groups matching behaviour when environment is missing.
Should I throw an miette: error ?
There was a problem hiding this comment.
I also don't know this part well enough let's ask @baszalmstra
There was a problem hiding this comment.
I would leave as is. If this code panics than some other thing is very wrong.
|
Other than the comments, look good! |
|
Thanks for the review! |
|
Hi, this looks great is there an example on how to pass ENV variables? |
|
He @zachcp, this pr shows an example, you need to add a activation script, e.g. |
|
Also: https://github.com/prefix-dev/pixi/tree/main/examples/pypi shows it on the main branch. |
|
Thank you, both. I was trying to use this mechanism to set the
Nonetheless - this could be very handy. |
No description provided.