-
Notifications
You must be signed in to change notification settings - Fork 178
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(cli): add --no-activation option to prevent env activation during global install/upgrade #1980
Conversation
Thanks for your contribution @183amir. Two comments after a quick glance:
|
Hi @Hofer-Julian thank you for taking a look. I understand your hesitation, but since there’s no clear timeline for the new implementation, this feature can provide immediate value and will be needed in the new implementation as well. I made sure it’s backward-compatible, so no disruption to existing workflows. Happy to adjust anything if needed! Thanks for considering! 🙌 |
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 to hold out on that one. This for sure needs more thoughts
I'd rather try this approach first before we add a symbolic option: #1382 (comment) |
Is this what you had in mind? Please let me know if this is ok. |
Thanks for looking into this. But no, I don't mean to add any new options, but instead add logic that only modifes the relevant env vars if the package has an activation script. |
I don't think an automatic solution is possible for this. Many conda packages don't include an explicit activation script but still depend on environment variable changes that occur during |
You raise good points. I think I am convinced now that we need that option. One thing that would be good to figure out later is how many packages benefit from this option. If it's only a few, important ones like pip, uv and zsh we might improve things a bit by implying this option for a few hardcoded packages. |
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.
Thanks!
I had a few comments. After those are implemented and someone tested the PR on Windows, I think this is ready to be merged :)
src/cli/global/upgrade.rs
Outdated
/// Do not insert conda_prefix, path modifications, and activation script into the installed executable script. | ||
#[clap(long)] | ||
no_activation: bool, |
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 only add this flag to pixi global install
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.
not sure how you would upgrade a no activation package without this option. maybe that will only be possible in the new format of global install.
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.
Yeah, fair point. Let's add the flag to the two update commands for now then.
Let's also revert the change to |
…lobal install/upgrade docs: update usage instructions with `--no-activation` flag in global cli documentation test: add integration tests for `--no-activation` option in global install/upgrade paths
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
Co-authored-by: Hofer-Julian <[email protected]>
…ds for simplification and standardization
That came from running pre-commit locally but the lint passes here so I am not sure what is wrong. |
The tests are failing but seems to be an issue from the main branch. |
Yeah that seems to be the case. Next week, as soon as main is fixed I will update and test this PR. If it works as expected, I will merge 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.
Looks good to me. Let's merge this.
A first version of the new pixi global
will land soon, and might not immediately feature --no-activation
. Let's keep track of adding this feature soon.
This describes this recently added [functionality](prefix-dev#1980) and how it could be added to the new `pixi global`
Thank you for merging. I will be happy to reimplement this in the new version if needed. |
This describes this recently added [functionality](#1980) and how it could be added to the new `pixi global` @183amir --------- Co-authored-by: Amir Mohammadi <[email protected]>
Introduce the
--no-activation
flag ininstall.rs
,upgrade.rs
, andupgrade_all.rs
to allow users to globally install executable scripts without activating the underlying conda environment. Fixes #1382docs: update usage instructions with
--no-activation
flag in global cli documentationtest: add integration tests for
--no-activation
option in global install/upgrade paths