-
Notifications
You must be signed in to change notification settings - Fork 176
Improve handling of initialize_conda / register_python including its default values #1105
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
constructor/nsis/main.nsi.tmpl
Outdated
| {%- endif %} | ||
| !if ${INIT_CONDA_OPTION} == 1 | ||
| # For consistency with other code, only for "Just Me" installation | ||
| ${If} $InstMode = ${JUST_ME} |
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.
Note that ${If} $InstMode = ${JUST_ME} is new (the option was still correctly disabled but this code should also account for this, correct?), but seems like it was missing based on the comment in OptionsDialog.nsh? I quote:
# AddToPath is only an option for JustMe installations; it is disabled for AllUsers
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.
It is checked in the python code that's called, but I think I found the same thing. The code in main should have a check like that already.
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.
If I understand correctly, the additional ${If} $InstMode = ${JUST_ME} I added is redundant and can safely be removed?
Update:
I saw when resolving the merge conflicts that you added ${If} ${FileExists} "$INSTDIR\.nonadmin" so I have removed the ${If} $InstMode = ${JUST_ME}
|
I've tested buildings installers and run them locally where I toggle all of these 4 options in various combinations and it looks good. Let me know if you need any screenshots. |
| ${EndIf} | ||
|
|
||
| ClearErrors | ||
| ${GetOptions} $ARGV "/RegisterPython=" $ARGV_RegisterPython |
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 old code (and not the updated one - yet) does not reflect what the docstring says:
/RegisterPython=[0|1] [default: AllUsers: 1, JustMe: 0]
i.e. the default value was never set based on scope. Should I update the code to reflect this? Or should we always just default it to 0?
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 help text should be adjusted to what the code has been doing all along.
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.
It's now /RegisterPython=[0|1] [default: 0]
d458a35 to
89d6393
Compare
marcoesters
left a comment
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.
My initial set of comments.
| ${EndIf} | ||
|
|
||
| ClearErrors | ||
| ${GetOptions} $ARGV "/RegisterPython=" $ARGV_RegisterPython |
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 help text should be adjusted to what the code has been doing all along.
d69585b to
355a508
Compare
| ${EndIf} | ||
|
|
||
| ClearErrors | ||
| ${GetOptions} $ARGV "/RegisterPython=" $ARGV_RegisterPython |
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.
It's now /RegisterPython=[0|1] [default: 0]
constructor/nsis/main.nsi.tmpl
Outdated
| {%- endif %} | ||
| !if ${INIT_CONDA_OPTION} == 1 | ||
| # For consistency with other code, only for "Just Me" installation | ||
| ${If} $InstMode = ${JUST_ME} |
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.
If I understand correctly, the additional ${If} $InstMode = ${JUST_ME} I added is redundant and can safely be removed?
Update:
I saw when resolving the merge conflicts that you added ${If} ${FileExists} "$INSTDIR\.nonadmin" so I have removed the ${If} $InstMode = ${JUST_ME}
| ${IfNot} ${Errors} | ||
| ${If} $ARGV_RegisterPython == "1" | ||
| StrCpy $REG_PY 1 | ||
| ${ElseIf} $ARGV_RegisterPython == "0" | ||
| StrCpy $REG_PY 0 | ||
| ${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.
Looking at #1004 and your help text, this should default to 0 in the CLI, just like the SH installer. As it stands, it defaults to the default value. So, we need an else clause here for when that option has not been found. Some with the conda initialization.
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.
If I'm not missing anything it does default to zero, however I have made it more clear (and safe?) in this commit https://github.com/conda/constructor/pull/1105/commits
I also noticed another doc-page that claimed that 1 is default so I changed that in the same commit.
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 us coming to different conclusions shows that it's necessary to be safe here 😅 Can you initialize $INIT_CONDA the same way?
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 agree, added here 666c2e3
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.
Update: I had to adjust it accordingly 83ed89a
I missed that Function .onInit calls !insertmacro ParseCommandLineArgs twice. First, early in the function call and then again (is this a mistake?) Call onInit_Release -> Which contains !insertmacro ParseCommandLineArgs
Therefore, we now have the following initialization:
Function .onInit
${LogSet} on
Push $0
Push $1
Push $2
Push $R1
Push $R2
# Initialize INIT_CONDA and REG_PY, note that they to default 0 if the installer
# is not configured to prompt the user with a choice.
!if ${INIT_CONDA_OPTION} == 1
StrCpy $INIT_CONDA ${INIT_CONDA_DEFAULT_VALUE}
!else
StrCpy $INIT_CONDA 0
!endif
!if ${REGISTER_PYTHON_OPTION} == 1
StrCpy $REG_PY ${REGISTER_PYTHON_DEFAULT_VALUE}
!else
StrCpy $REG_PY 0
!endif
...
| # Ensure we initialize from compile-time default value | ||
| StrCpy $INIT_CONDA ${INIT_CONDA_DEFAULT_VALUE} | ||
| !else | ||
| StrCpy $INIT_CONDA 0 |
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 need to update the documentation in the schema that the default values have no effect if initialize_conda and register_python are false. Maybe also add a sentence or two into the release notes for this since this is a behavior change?
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.
So for initialize_by_default it says already:
Only applies if initialize_conda is not false.
And for register_python_default:
Only applies if register_python is true.
Are you thinking of anything else in addition to this?
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.
Ah, I didn't see those isolated paragraphs, my bad. The release notes should be updated though since this is new for Windows. The documentation should be updated either way to emphasize that these values only apply to interactive installations and are false for CLI installations.
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.
Updated again in db68a7c
It got pretty long, please leave some feedback. I'm also trying to stay consistent with the existing style of the release notes.
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 left some feedback to be more user-oriented. We also need to update the description in the schema so that this is documented in CONSTRUCT.md. I think after those small comments are addressed, we're in business.
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.
Okay so I have made a couple of changes, and there is already some text in the documentation I think already explains the situation. I'll try to explain it so we are aligned. First here are the changes from last 2 commits I've just done: https://github.com/conda/constructor/pull/1105/files/db68a7c3cdeaeaf53a0d832b100eb3bd666ca424..bf24d45b2aadbf0010e26118f5b89a380a107d34
- We had
initialize_by_default: bool | None = None, but comparing it toregister_python_defaultI changed it toinitialize_by_default: bool | None = False. And to be honest, I think we can drop the entireNone-situation,True/Falseshould suffice but I assume this was implemented asNonebecause the option might be irrelevant depending on its parent (initialize_conda/register_python). - It says that
initialize_by_defaultisTruefor GUI-installers, I quote: The default is true for GUI installers (EXE, PKG). But this is not correct because inwinexe.pywe can seevariables["initialize_by_default"] = info.get("initialize_by_default", None), which means it's by defaultNone. In the commit I linked to I also changedNonetoFalsefor consistency with the other installers, the impact this has in the NSIS code is the same. - For
register_pythonI added the additional text "If the installer bundles a Python instance,", to account for the situation thathas_pythoncould be false.
Finally, this leaves us with the following:
### `initialize_conda`
Add an option to the installer so the user can choose whether to run `conda init`
after the installation (Unix), or to add certain subdirectories of the installation
to PATH (Windows). Requires `conda` to be part of the `base` environment. Valid options:
- `classic` or `True`: runs `conda init` on Unix, which injects a shell function in the
shell profiles. On Windows, it adds `$INSTDIR`, `$INSTDIR/Scripts`, `$INSTDIR/Library/bin`
to `PATH`. This is the default.
- `condabin`: only adds `$INSTDIR/condabin` to `PATH`. On Unix, `conda>=25.5.0` is required
in `base`.
- `False`: the installer doesn't perform initialization.
See also `initialize_by_default`.
### `initialize_by_default`
Default value for the option added by `initialize_conda`. The default is
true for PKG installers, and false for EXE (GUI and CLI installations) and shell installers. The user
is able to change the default during interactive installation. NOTE: For Windows,
`AddToPath` is disabled when `InstallationType=AllUsers`.
Only applies if `initialize_conda` is not false.
### `register_python`
If the installer installs a Python instance, offer the user an option to register the installed Python instance as the
system's default Python. (Windows only)
### `register_python_default`
Default choice for whether to register the installed Python instance as the
system's default Python. The user is still able to change this during
interactive installation. (Windows only).
Let me know what you think.
| The options `initialize_by_default` and `register_python_default` only applies for interactive installations. | ||
| Note that when installing via the command-line, the corresponding options `AddToPath` and `RegisterPython` are by default set to `0`. (#1003, #1004 via #1105) |
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 options `initialize_by_default` and `register_python_default` only applies for interactive installations. | |
| Note that when installing via the command-line, the corresponding options `AddToPath` and `RegisterPython` are by default set to `0`. (#1003, #1004 via #1105) | |
| Windows CLI installations now don't `conda` to `PATH` or register Python by default. (#1003, #1004 via #1105) |
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.
Added 666c2e3
Description
This PR is to address:
/AddToPathand/RegisterPythonalways available #1003initialize_conda/initialize_by_defaulthandling across installers #1004Checklist - did you ...
newsdirectory (using the template) for the next release's release notes?