Skip to content

Conversation

@lrandersson
Copy link
Contributor

@lrandersson lrandersson commented Nov 10, 2025

Description

This PR is to address:

  1. NSIS: /AddToPath and /RegisterPython always available #1003
  2. Inconsistencies in initialize_conda / initialize_by_default handling across installers #1004

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@github-project-automation github-project-automation bot moved this to 🆕 New in 🔎 Review Nov 10, 2025
@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Nov 10, 2025
{%- endif %}
!if ${INIT_CONDA_OPTION} == 1
# For consistency with other code, only for "Just Me" installation
${If} $InstMode = ${JUST_ME}
Copy link
Contributor Author

@lrandersson lrandersson Nov 10, 2025

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

Copy link
Contributor

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.

Copy link
Contributor Author

@lrandersson lrandersson Nov 11, 2025

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}

@lrandersson
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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]

Copy link
Contributor

@marcoesters marcoesters left a 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
Copy link
Contributor

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.

${EndIf}

ClearErrors
${GetOptions} $ARGV "/RegisterPython=" $ARGV_RegisterPython
Copy link
Contributor Author

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]

{%- endif %}
!if ${INIT_CONDA_OPTION} == 1
# For consistency with other code, only for "Just Me" installation
${If} $InstMode = ${JUST_ME}
Copy link
Contributor Author

@lrandersson lrandersson Nov 11, 2025

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}

Comment on lines +373 to +378
${IfNot} ${Errors}
${If} $ARGV_RegisterPython == "1"
StrCpy $REG_PY 1
${ElseIf} $ARGV_RegisterPython == "0"
StrCpy $REG_PY 0
${EndIf}
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@lrandersson lrandersson Nov 17, 2025

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
    ...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, early in the function call and then again (is this a mistake?)

That is a good question. My guess is that the mui_AnaCustomOptions_InitDefaults function maybe overwrote some variables, but not the ones that are needed for all the pre-install checks.

Therefore, we now have the following initialization:

If I understand your changes correctly, this would re-introduce the inconsistency between shell installers and EXE installers. The shell installers do not initialize anything in a CLI installation. The thought behind that is that we do not want to change shell profiles without explicit user consent. Your current initialization would set the default to the value set in construct.yaml (which contradicts the current help text where you write that the default is always 0). The difference is that in EXE installers, users can still opt out though.

I think initializing everything to 0 is the correct way to do this. In the CLI, ParseCommandLineArgs will override the default with user input. For GUI installers, mui_AnaCustomOptions_InitDefaults will initialize the correct defaults for the GUI. The easiest and cleanest way is probably:

${If} ${Silent}
  !insetmacro ParseCommandLineArgs
${Else}
  Call mui_AnaCustomOptions_InitDefaults
${EndIf}

That will, of course, mean that OnInit_Release cannot insert the command line parsing macro anymore, which I don't think is a bad thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See latest commit a913928

# Ensure we initialize from compile-time default value
StrCpy $INIT_CONDA ${INIT_CONDA_DEFAULT_VALUE}
!else
StrCpy $INIT_CONDA 0
Copy link
Contributor

@marcoesters marcoesters Nov 13, 2025

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

@lrandersson lrandersson Nov 17, 2025

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

  1. We had initialize_by_default: bool | None = None, but comparing it to register_python_default I changed it to initialize_by_default: bool | None = False. And to be honest, I think we can drop the entire None-situation, True/False should suffice but I assume this was implemented as None because the option might be irrelevant depending on its parent (initialize_conda/register_python).
  2. It says that initialize_by_default is True for GUI-installers, I quote: The default is true for GUI installers (EXE, PKG). But this is not correct because in winexe.py we can see variables["initialize_by_default"] = info.get("initialize_by_default", None), which means it's by default None. In the commit I linked to I also changed None to False for consistency with the other installers, the impact this has in the NSIS code is the same.
  3. For register_python I added the additional text "If the installer bundles a Python instance,", to account for the situation that has_python could 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And to be honest, I think we can drop the entire None-situation, True/False should suffice but I assume this was implemented as None because the option might be irrelevant depending on its parent

None means "not specified in constructor.yaml". Since initialize_by_default is different between PKG, EXE, and SH, None is still necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed back here 16ffe6e.
I feel like register_python_default should also be None then instead of default False? I'm not planning to change it now but it seems like initialize_by_default and register_python_default should have the same defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See latest commit a913928

Comment on lines 5 to 6
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added 666c2e3

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More adjustments in the latest commit a913928

Copy link
Contributor

@marcoesters marcoesters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the code is there - just a few word changes. Please verify that my suggestions caught the spirit of it.

StrCpy $INIT_CONDA 0
${EndIf}
${Else}
# If we have Errors, i.e. the option is not explicitly set by the user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# If we have Errors, i.e. the option is not explicitly set by the user
# If we have Errors, the option is not explicitly set by the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

* EXE: Improve handling of options `initialize_conda`, `register_python` with their corresponding default values. The behavior of these options
with respect to `initialize_conda_default` and `register_python_default` is now consistent with `.sh` and `.pkg` installers. (#1003, #1004 via #1105)
with respect to `initialize_by_default` and `register_python_default` is now consistent with `.sh` and `.pkg` installers.
Windows CLI installations now don't add `conda` to `PATH` or register Python by default, and, command-line arguments are
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Windows CLI installations now don't add `conda` to `PATH` or register Python by default, and, command-line arguments are
Windows CLI installations now don't add `conda` to `PATH` or register Python by default, and command-line arguments are

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with respect to `initialize_conda_default` and `register_python_default` is now consistent with `.sh` and `.pkg` installers. (#1003, #1004 via #1105)
with respect to `initialize_by_default` and `register_python_default` is now consistent with `.sh` and `.pkg` installers.
Windows CLI installations now don't add `conda` to `PATH` or register Python by default, and, command-line arguments are
only parsed when installing in silent mode (enabled with the flag `/S`).(#1003, #1004 via #1105)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
only parsed when installing in silent mode (enabled with the flag `/S`).(#1003, #1004 via #1105)
only parsed when installing in silent mode (enabled with the flag `/S`). (#1003, #1004 via #1105)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

StrCpy $REG_PY 0
${EndIf}
${Else}
# If we have Errors, i.e. the option is not explicitly set by the user
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# If we have Errors, i.e. the option is not explicitly set by the user
# If we have Errors, the option is not explicitly set by the user

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 760 to 762
StrCpy $Ana_ClearPkgCache_State {{ '${BST_UNCHECKED}' if keep_pkgs else '${BST_CHECKED}' }}
StrCpy $Ana_PreInstall_State {{ '${BST_CHECKED}' if pre_install_exists else '${BST_UNCHECKED}' }}
StrCpy $Ana_PostInstall_State {{ '${BST_CHECKED}' if post_install_exists else '${BST_UNCHECKED}' }}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting that these are not initialized in mui_AnaCustomOptions_InitDefaults. Something to add to #1109 or #1110

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm glad you mentioned that. I was also giving them a side-eye yesterday, I have updated #1110 to fix this.

Comment on lines 704 to 705
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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)
If the installer installs a Python instance, offer the user an option to register the installed Python instance as the
system's default Python. Defaults to `true` for GUI and `false` for CLI installations. (Windows only)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment on lines 695 to 698
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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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`.
Default value for the option added by `initialize_conda`. The default is
true for PKG installers, and false for EXE and SH shell installers.
The user is able to change the default during interactive installations.
Non-interactive installations are not affected by this value: users must explicitly request
to add to `PATH` via CLI options.
NOTE: For Windows, `/AddToPath` is disabled when `/InstallationType=AllUsers`.

I tried to make this explicit. Let me know what you think.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think this is a good change. I feel likely in general it's a bit confusing as a user when reading about these two (four) options. We have one option to just make another option visible, and then the actual option to toggle the value. Easy to get confused.
Now with being this explicit it will be easier to understand. I've applied the suggestion in cf93b7a.

@github-project-automation github-project-automation bot moved this from 🆕 New to ✅ Approved in 🔎 Review Nov 19, 2025
@marcoesters marcoesters merged commit 227bb65 into conda:main Nov 19, 2025
34 of 36 checks passed
@github-project-automation github-project-automation bot moved this from ✅ Approved to 🏁 Done in 🔎 Review Nov 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla-signed [bot] added once the contributor has signed the CLA

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Inconsistencies in initialize_conda / initialize_by_default handling across installers NSIS: /AddToPath and /RegisterPython always available

3 participants