-
Notifications
You must be signed in to change notification settings - Fork 168
Add support for conda init --condabin
, which adds condabin/
to PATH
#965
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?
Conversation
…o pydantic-schema
condabin/
to PATHconda init --condabin
, which adds condabin/
to PATH
@marcoesters, I think this is ready! It'd be nice to check if it behaves as intended locally too. Maybe I can extend the tests to actually check for Also, I'd like to hear what you think about the proposed UI/text changes. I'd be happy to change whatever there, not feeling particularly strongly, but I do want to make it less confusing for end users. |
It would still be good to test that the installers implement this correctly.
On Windows, I'm not sure it's obvious what the benefit of one option over the other is, so having both available is not a great idea. So, I propose to either have installer providers a) choose what they want |
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 great! Just a couple of questions
### `add_condabin_to_path_default` | ||
|
||
Default value for the option added by `add_condabin_to_path`. The default | ||
is true for GUI installers (EXE, PKG) and false for shell installers. The user |
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 motivation behind making this default value different for different types of installers?
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.
Consistency with previous UX. I assume the notion is that GUI users don't want to mess too much with the terminal, while shell installer users would be more ok with some tinkering. It's a bad proxy though.
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 think consistency with previous UX is to not make it the default because in past UX, that option did not exist. If I updated constructor
without making changes to the construct.yaml
file, my installer would behave differently.
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.
An installer built with an updated constructor wouldn't see any changes in behaviour because add_condabin_to_path
defaults to false
. This is the default value once add_condabin_to_path
is true
.
It's on the installer creator to offer both. By default, the
They have two flags, which can be used together if needed, but it's on the installer creator to actually provide them.
This will need a different PR, and it actually opened a can of worms. I'll open an issue to discuss it and then a potential fix. |
{%- if add_condabin_to_path %} | ||
{%- if add_condabin_to_path_default %} | ||
-z do not add PREFIX/condabin to PATH; ignored in interactive mode | ||
{%- else %} | ||
-c add PREFIX/condabin to PATH; ignored in interactive mode | ||
{%- endif %} | ||
{%- endif %} |
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 concerned about adding these additional, complementary flags here. #982 already tried to implement an extra flag for another option. If we add #1004 to the mix, I am concerned about maintainability.
The order of the flags is also getting a little chaotic. I wonder if we should keep the existing behavior of adding to path and initializing should be off in non-interactive mode unless opted in.
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 still need the flags to opt-in so which ones do you recommend? We can also make it a single flag option like -c OPTION Whether to initialize conda in some way: shell, condabin, none
. This would make initialize_conda
and add_condabin_to_path
mutually exclusive though. Do you think we should turn initialize_conda
into an enum type instead of a bool directly?
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, we need flags to opt in. My concern is more to have two different flags for the opt-in/opt-out behavior. It would be ideal if we used long options, which are more explicit than -c
. What do you think?
inject some shell functions so you can run `conda` from anywhere. That is, the `conda` executable | ||
you usually run is not the Python entry point, but a shell function that ends up calling that entry | ||
point. This wrapper is needed by `conda (de)activate` to operate correctly, since it needs to |
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.
The sentence starting with "That is, ..." was a little confusing for me to read. Is it needed at all?
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 think it's an important distinction to make, but happy to hear alternative wordings to make it clearer.
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.
inject some shell functions so you can run `conda` from anywhere. That is, the `conda` executable | |
you usually run is not the Python entry point, but a shell function that ends up calling that entry | |
point. This wrapper is needed by `conda (de)activate` to operate correctly, since it needs to | |
inject some shell functions so you can run `conda` from anywhere. That is, the `conda` "executable" | |
you usually run is not the `conda` entry point, but a shell function that ends up calling the actual entry | |
point. This wrapper is needed by `conda (de)activate` to operate correctly, since it needs to |
I think the "Python entry point", while technically correct, is coming a little out of nowhere. Maybe this?
### `add_condabin_to_path_default` | ||
|
||
Default value for the option added by `add_condabin_to_path`. The default | ||
is true for GUI installers (EXE, PKG) and false for shell installers. The user |
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 think consistency with previous UX is to not make it the default because in past UX, that option did not exist. If I updated constructor
without making changes to the construct.yaml
file, my installer would behave differently.
Co-authored-by: Marco Esters <[email protected]>
Description
Closes #960.
Based on #943 to avoid conflicts down the line because this adds new options to
construct.yaml
. Excuse the diff while 943 gets reviewed.Windows:
macOS:
Checklist - did you ...
news
directory (using the template) for the next release's release notes?