From 01c86420b1a2773810640866b53737379628ce44 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 21 Feb 2024 23:49:29 +0000 Subject: [PATCH 1/5] gh-115554: Improved logic for handling multiple existing py.exe launcher installs --- ...-02-21-23-48-59.gh-issue-115554.02mpQC.rst | 4 + .../PythonBootstrapperApplication.cpp | 162 +++++++++++------- Tools/msi/bundle/bundle.wxs | 2 +- 3 files changed, 103 insertions(+), 65 deletions(-) create mode 100644 Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst diff --git a/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst new file mode 100644 index 000000000000000..f8bbdeb7666bf88 --- /dev/null +++ b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst @@ -0,0 +1,4 @@ +The installer now has more strict rules about updating :ref:`launcher`. In +general, most users only have a single launcher installed and will see no +difference. When multiple launchers have been installed, the option to +install the launcher may be disabled until all but one have been removed. diff --git a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp index 3a17ffbaa0b6550..d53545e383f925a 100644 --- a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp +++ b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp @@ -718,25 +718,76 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { __in DWORD64 /*dw64Version*/, __in BOOTSTRAPPER_RELATED_OPERATION operation ) { - if (BOOTSTRAPPER_RELATED_OPERATION_MAJOR_UPGRADE == operation && - (CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_AllUsers", -1) || + if ((CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_AllUsers", -1) || CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_JustForMe", -1))) { - auto hr = LoadAssociateFilesStateFromKey(_engine, fPerMachine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER); - if (hr == S_OK) { - _engine->SetVariableNumeric(L"AssociateFiles", 1); - } else if (hr == S_FALSE) { - _engine->SetVariableNumeric(L"AssociateFiles", 0); - } else if (FAILED(hr)) { - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Failed to load AssociateFiles state: error code 0x%08X", hr); - } + BalLog(BOOTSTRAPPER_LOG_LEVEL_INFO, "Detected existing launcher install"); - LONGLONG includeLauncher; - if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher)) - || includeLauncher == -1) { - _engine->SetVariableNumeric(L"Include_launcher", 1); + LONGLONG detectedLauncher; + if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { + detectedLauncher = -1; + } + if (detectedLauncher == 1) { + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Multiple launcher installs have been detected."); + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "No launcher will be installed or upgraded until one has been removed."); + _engine->SetVariableNumeric(L"Include_launcher", 0); _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); } - _engine->SetVariableNumeric(L"DetectedOldLauncher", 1); + else { + _engine->SetVariableNumeric(L"DetectedLauncher", 1); + + LONGLONG includeLauncher, includeLaucherAllUsers; + if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher))) { + includeLauncher = -1; + } + if (FAILED(BalGetNumericVariable(L"InstallLauncherAllUsers", &includeLauncherAllUsers))) { + includeLauncherAllUsers = -1; + } + + if (includeLauncher == 1) { + // We already want to install, so check if settings are consistent + LONGLONG perMachine; + if (includeLauncherAllUsers == -1) { + // Unspecified value, so update to match what was detected + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); + } else if (includeLauncherAllUsers != (fPerMachine ? 1 : 0)) { + // Inconsistent preference, so disable launcher install + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Existing launcher is inconsistent with install request - disabling install"); + _engine->SetVariableNumeric(L"Include_launcher", 0); + } + } + else if (includeLauncher == 0) { + // Launch requested no launcher, but we found one. This is + // fine as long as the user leaves the setting unchanged, + // which ought to be handled in OnDetectComplete because + // Include_launcher != -1 and DetectedLauncher == 1 + } + else if (BOOTSTRAPPER_RELATED_OPERATION_DOWNGRADE == operation) { + // Found a higher version, so we can't install ours. + _engine->SetVariableNumeric(L"Include_launcher", 0); + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); + } + else if (BOOTSTRAPPER_RELATED_OPERATION_MAJOR_UPGRADE == operation) { + // Found an older version, so let's run the equivalent as an upgrade + // This overrides "unknown" all users options, but will leave alone + // any that have already been set/detected. + // User can deselect the option to include the launcher, but cannot + // change it from the current per user/machine setting. + _engine->SetVariableNumeric(L"Include_launcher", 1); + if (installLauncherAllUsers < 0) { + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); + } + _engine->SetVariableNumeric(L"DetectedOldLauncher", 1); + } + + auto hr = LoadAssociateFilesStateFromKey(_engine, fPerMachine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER); + LONGLONG associateFiles = 1; + if (FAILED(hr)) { + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Failed to load AssociateFiles state: error code 0x%08X", hr); + } else if (hr == S_FALSE) { + associateFiles = 0; + } + _engine->SetVariableNumeric(L"AssociateFiles", associateFiles); + } } return CheckCanceled() ? IDCANCEL : IDNOACTION; } @@ -784,48 +835,7 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { __in LPCWSTR wzPackageId, __in HRESULT hrStatus, __in BOOTSTRAPPER_PACKAGE_STATE state - ) { - if (FAILED(hrStatus)) { - return; - } - - BOOL detectedLauncher = FALSE; - HKEY hkey = HKEY_LOCAL_MACHINE; - if (CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_AllUsers", -1)) { - if (BOOTSTRAPPER_PACKAGE_STATE_PRESENT == state || BOOTSTRAPPER_PACKAGE_STATE_OBSOLETE == state) { - detectedLauncher = TRUE; - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", 1); - } - } else if (CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_JustForMe", -1)) { - if (BOOTSTRAPPER_PACKAGE_STATE_PRESENT == state || BOOTSTRAPPER_PACKAGE_STATE_OBSOLETE == state) { - detectedLauncher = TRUE; - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", 0); - } - } - - LONGLONG includeLauncher; - if (SUCCEEDED(BalGetNumericVariable(L"Include_launcher", &includeLauncher)) - && includeLauncher != -1) { - detectedLauncher = FALSE; - } - - if (detectedLauncher) { - /* When we detect the current version of the launcher. */ - _engine->SetVariableNumeric(L"Include_launcher", 1); - _engine->SetVariableNumeric(L"DetectedLauncher", 1); - _engine->SetVariableString(L"Include_launcherState", L"disable"); - _engine->SetVariableString(L"InstallLauncherAllUsersState", L"disable"); - - auto hr = LoadAssociateFilesStateFromKey(_engine, hkey); - if (hr == S_OK) { - _engine->SetVariableNumeric(L"AssociateFiles", 1); - } else if (hr == S_FALSE) { - _engine->SetVariableNumeric(L"AssociateFiles", 0); - } else if (FAILED(hr)) { - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Failed to load AssociateFiles state: error code 0x%08X", hr); - } - } - } + ) { } virtual STDMETHODIMP_(void) OnDetectComplete(__in HRESULT hrStatus) { @@ -835,20 +845,44 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { } if (SUCCEEDED(hrStatus)) { - LONGLONG includeLauncher; - if (SUCCEEDED(BalGetNumericVariable(L"Include_launcher", &includeLauncher)) - && includeLauncher == -1) { + // Update launcher install states + // If we didn't detect any existing installs, Include_launcher and + // InstallLauncherAllUsers will both be -1, so we will set to their + // defaults and leave the options enabled. + // Otherwise, if we detected an existing install, we disable the + // options so they remain fixed. + // The code in OnDetectRelatedMsiPackage is responsible for figuring + // out whether existing installs are compatible with the settings in + // place during detection. + LONGLONG detectedLauncher, includeLauncher, includeLauncherAllUsers; + if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { + detectedLauncher = 0; + } + if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher))) { + includeLauncher = -1; + } + if (FAILED(BalGetNumericVariable(L"InstallLauncherAllUsers", &includeLauncherAllUsers))) { + includeLauncherAllUsers = -1; + } + + if (includeLauncherAllUsers == -1) { + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", 0); + } else if (detectedLauncher) { + _engine->SetVariableString(L"InstallLauncherAllUsersState", L"disable"); + } + + if (includeLauncher == -1) { if (BOOTSTRAPPER_ACTION_LAYOUT == _command.action || (BOOTSTRAPPER_ACTION_INSTALL == _command.action && !_upgrading)) { - // When installing/downloading, we want to include the launcher - // by default. + // When installing/downloading, we include the launcher _engine->SetVariableNumeric(L"Include_launcher", 1); } else { - // Any other action, if we didn't detect the MSI then we want to - // keep it excluded + // Any other action, don't include by default _engine->SetVariableNumeric(L"Include_launcher", 0); _engine->SetVariableNumeric(L"AssociateFiles", 0); } + } else if (detectedLauncher) { + _engine->SetVariableString(L"Include_launcherState", L"disable"); } } diff --git a/Tools/msi/bundle/bundle.wxs b/Tools/msi/bundle/bundle.wxs index 9b4f072152d5c00..387df307c44e057 100644 --- a/Tools/msi/bundle/bundle.wxs +++ b/Tools/msi/bundle/bundle.wxs @@ -28,7 +28,7 @@ - + From 62c0218be957cedbb17dcd0237c854e9300e1c8f Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 22 Feb 2024 22:57:11 +0000 Subject: [PATCH 2/5] Improved logic --- Tools/msi/bundle/Default.wxl | 1 + .../PythonBootstrapperApplication.cpp | 171 ++++++++++-------- Tools/msi/bundle/bundle.wxs | 4 +- 3 files changed, 97 insertions(+), 79 deletions(-) diff --git a/Tools/msi/bundle/Default.wxl b/Tools/msi/bundle/Default.wxl index 1540f050159a548..0014204e89d1bb7 100644 --- a/Tools/msi/bundle/Default.wxl +++ b/Tools/msi/bundle/Default.wxl @@ -88,6 +88,7 @@ Select Customize to review current options. Install Python [ShortVersion] for &all users for &all users (requires admin privileges) Use admin privi&leges when installing py.exe + Python Launcher is already installed &Precompile standard library Download debugging &symbols Download debu&g binaries (requires VS 2017 or later) diff --git a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp index d53545e383f925a..369d0f1e2f7e4a6 100644 --- a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp +++ b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp @@ -442,6 +442,14 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { ThemeControlElevates(_theme, ID_INSTALL_BUTTON, elevated); ThemeControlElevates(_theme, ID_INSTALL_SIMPLE_BUTTON, elevated); ThemeControlElevates(_theme, ID_INSTALL_UPGRADE_BUTTON, elevated); + + LONGLONG blockedLauncher; + if (SUCCEEDED(BalGetNumericVariable(L"BlockedLauncher", &blockedLauncher)) && blockedLauncher) { + LOC_STRING *pLocString = nullptr; + if (SUCCEEDED(LocGetString(_wixLoc, L"#(loc.ShortInstallLauncherBlockedLabel)", &pLocString)) && pLocString) { + ThemeSetTextControl(_theme, ID_INSTALL_LAUNCHER_ALL_USERS_CHECKBOX, pLocString->wzText); + } + } } void Custom1Page_Show() { @@ -718,24 +726,44 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { __in DWORD64 /*dw64Version*/, __in BOOTSTRAPPER_RELATED_OPERATION operation ) { - if ((CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_AllUsers", -1) || - CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_JustForMe", -1))) { - BalLog(BOOTSTRAPPER_LOG_LEVEL_INFO, "Detected existing launcher install"); + // Only check launcher_AllUsers because we'll find the same packages + // twice if we check launcher_JustForMe as well. + if (CSTR_EQUAL == ::CompareStringW(LOCALE_NEUTRAL, 0, wzPackageId, -1, L"launcher_AllUsers", -1)) { + BalLog(BOOTSTRAPPER_LOG_LEVEL_STANDARD, "Detected existing launcher install"); - LONGLONG detectedLauncher; + LONGLONG blockedLauncher, detectedLauncher; + if (FAILED(BalGetNumericVariable(L"BlockedLauncher", &blockedLauncher))) { + blockedLauncher = 0; + } if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { detectedLauncher = -1; } - if (detectedLauncher == 1) { - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Multiple launcher installs have been detected."); - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "No launcher will be installed or upgraded until one has been removed."); - _engine->SetVariableNumeric(L"Include_launcher", 0); - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); - } - else { - _engine->SetVariableNumeric(L"DetectedLauncher", 1); - LONGLONG includeLauncher, includeLaucherAllUsers; + _engine->SetVariableNumeric(L"DetectedLauncher", 1); + + if (blockedLauncher) { + // Nothing else to do, we're already blocking + } + else if (BOOTSTRAPPER_RELATED_OPERATION_DOWNGRADE == operation) { + // Found a higher version, so we can't install ours. + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Higher version launcher has been detected."); + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Launcher will not be installed"); + _engine->SetVariableNumeric(L"BlockedLauncher", 1); + } + else if (detectedLauncher == 1) { + if (!blockedLauncher) { + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Multiple launcher installs have been detected."); + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "No launcher will be installed or upgraded until one has been removed."); + _engine->SetVariableNumeric(L"BlockedLauncher", 1); + } + } + else if (BOOTSTRAPPER_RELATED_OPERATION_MAJOR_UPGRADE == operation) { + // Found an older version, so let's run the equivalent as an upgrade + // This overrides "unknown" all users options, but will leave alone + // any that have already been set/detected. + // User can deselect the option to include the launcher, but cannot + // change it from the current per user/machine setting. + LONGLONG includeLauncher, includeLauncherAllUsers; if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher))) { includeLauncher = -1; } @@ -743,50 +771,16 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { includeLauncherAllUsers = -1; } - if (includeLauncher == 1) { - // We already want to install, so check if settings are consistent - LONGLONG perMachine; - if (includeLauncherAllUsers == -1) { - // Unspecified value, so update to match what was detected - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); - } else if (includeLauncherAllUsers != (fPerMachine ? 1 : 0)) { - // Inconsistent preference, so disable launcher install - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Existing launcher is inconsistent with install request - disabling install"); - _engine->SetVariableNumeric(L"Include_launcher", 0); - } - } - else if (includeLauncher == 0) { - // Launch requested no launcher, but we found one. This is - // fine as long as the user leaves the setting unchanged, - // which ought to be handled in OnDetectComplete because - // Include_launcher != -1 and DetectedLauncher == 1 - } - else if (BOOTSTRAPPER_RELATED_OPERATION_DOWNGRADE == operation) { - // Found a higher version, so we can't install ours. - _engine->SetVariableNumeric(L"Include_launcher", 0); - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); - } - else if (BOOTSTRAPPER_RELATED_OPERATION_MAJOR_UPGRADE == operation) { - // Found an older version, so let's run the equivalent as an upgrade - // This overrides "unknown" all users options, but will leave alone - // any that have already been set/detected. - // User can deselect the option to include the launcher, but cannot - // change it from the current per user/machine setting. + if (includeLauncher < 0) { _engine->SetVariableNumeric(L"Include_launcher", 1); - if (installLauncherAllUsers < 0) { - _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); - } - _engine->SetVariableNumeric(L"DetectedOldLauncher", 1); } - - auto hr = LoadAssociateFilesStateFromKey(_engine, fPerMachine ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER); - LONGLONG associateFiles = 1; - if (FAILED(hr)) { - BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Failed to load AssociateFiles state: error code 0x%08X", hr); - } else if (hr == S_FALSE) { - associateFiles = 0; + if (includeLauncherAllUsers < 0) { + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", fPerMachine ? 1 : 0); + } else if (includeLauncherAllUsers != fPerMachine ? 1 : 0) { + // Requested AllUsers option is inconsistent, so block + _engine->SetVariableNumeric(L"BlockedLauncher", 1); } - _engine->SetVariableNumeric(L"AssociateFiles", associateFiles); + _engine->SetVariableNumeric(L"DetectedOldLauncher", 1); } } return CheckCanceled() ? IDCANCEL : IDNOACTION; @@ -854,35 +848,56 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { // The code in OnDetectRelatedMsiPackage is responsible for figuring // out whether existing installs are compatible with the settings in // place during detection. - LONGLONG detectedLauncher, includeLauncher, includeLauncherAllUsers; - if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { - detectedLauncher = 0; - } - if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher))) { - includeLauncher = -1; - } - if (FAILED(BalGetNumericVariable(L"InstallLauncherAllUsers", &includeLauncherAllUsers))) { - includeLauncherAllUsers = -1; - } - - if (includeLauncherAllUsers == -1) { + LONGLONG blockedLauncher; + if (SUCCEEDED(BalGetNumericVariable(L"BlockedLauncher", &blockedLauncher)) + && blockedLauncher) { + _engine->SetVariableNumeric(L"Include_launcher", 0); _engine->SetVariableNumeric(L"InstallLauncherAllUsers", 0); - } else if (detectedLauncher) { _engine->SetVariableString(L"InstallLauncherAllUsersState", L"disable"); + _engine->SetVariableString(L"Include_launcherState", L"disable"); } + else { + LONGLONG includeLauncher, includeLauncherAllUsers, associateFiles; - if (includeLauncher == -1) { - if (BOOTSTRAPPER_ACTION_LAYOUT == _command.action || - (BOOTSTRAPPER_ACTION_INSTALL == _command.action && !_upgrading)) { - // When installing/downloading, we include the launcher - _engine->SetVariableNumeric(L"Include_launcher", 1); - } else { - // Any other action, don't include by default - _engine->SetVariableNumeric(L"Include_launcher", 0); - _engine->SetVariableNumeric(L"AssociateFiles", 0); + if (FAILED(BalGetNumericVariable(L"Include_launcher", &includeLauncher))) { + includeLauncher = -1; + } + if (FAILED(BalGetNumericVariable(L"InstallLauncherAllUsers", &includeLauncherAllUsers))) { + includeLauncherAllUsers = -1; + } + if (FAILED(BalGetNumericVariable(L"AssociateFiles", &associateFiles))) { + associateFiles = -1; + } + + if (includeLauncherAllUsers == -1) { + includeLauncherAllUsers = 0; + _engine->SetVariableNumeric(L"InstallLauncherAllUsers", includeLauncherAllUsers); + } + + if (includeLauncher == -1) { + if (BOOTSTRAPPER_ACTION_LAYOUT == _command.action || + (BOOTSTRAPPER_ACTION_INSTALL == _command.action && !_upgrading)) { + // When installing/downloading, we include the launcher + _engine->SetVariableNumeric(L"Include_launcher", 1); + } else { + // Any other action, don't include by default + _engine->SetVariableNumeric(L"Include_launcher", 0); + _engine->SetVariableNumeric(L"AssociateFiles", 0); + } + } + + if (associateFiles == -1) { + auto hr = LoadAssociateFilesStateFromKey( + _engine, + includeLauncherAllUsers ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER + ); + if (FAILED(hr)) { + BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Failed to load AssociateFiles state: error code 0x%08X", hr); + } else if (hr == S_OK) { + associateFiles = 1; + } + _engine->SetVariableNumeric(L"AssociateFiles", associateFiles); } - } else if (detectedLauncher) { - _engine->SetVariableString(L"Include_launcherState", L"disable"); } } diff --git a/Tools/msi/bundle/bundle.wxs b/Tools/msi/bundle/bundle.wxs index 387df307c44e057..95fd13a85bba9f3 100644 --- a/Tools/msi/bundle/bundle.wxs +++ b/Tools/msi/bundle/bundle.wxs @@ -32,6 +32,7 @@ + @@ -91,10 +92,11 @@ + - + From 02126f053b20a7164e2d43693241ad85da4fe9d2 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 22 Feb 2024 23:35:35 +0000 Subject: [PATCH 3/5] Improved NEWS --- .../Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst index f8bbdeb7666bf88..ec6691e5d0681b5 100644 --- a/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst +++ b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst @@ -1,4 +1,6 @@ The installer now has more strict rules about updating :ref:`launcher`. In general, most users only have a single launcher installed and will see no difference. When multiple launchers have been installed, the option to -install the launcher may be disabled until all but one have been removed. +install the launcher is disabled until all but one have been removed. +Downgrading the launcher (which was never allowed) is now more obviously +blocked. From d52ee26b7707f02a177828c2f7d0070f0f9f5d60 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Wed, 28 Feb 2024 16:55:22 +0000 Subject: [PATCH 4/5] Changes from self code review --- .../bootstrap/PythonBootstrapperApplication.cpp | 17 +++++++++++------ Tools/msi/bundle/bundle.wxs | 2 +- 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp index 369d0f1e2f7e4a6..e9d87717e94ca64 100644 --- a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp +++ b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp @@ -736,10 +736,12 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { blockedLauncher = 0; } if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { - detectedLauncher = -1; + detectedLauncher = 0; } - _engine->SetVariableNumeric(L"DetectedLauncher", 1); + if (!detectedLauncher) { + _engine->SetVariableNumeric(L"DetectedLauncher", 1); + } if (blockedLauncher) { // Nothing else to do, we're already blocking @@ -869,24 +871,27 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { associateFiles = -1; } - if (includeLauncherAllUsers == -1) { + if (includeLauncherAllUsers < 0) { includeLauncherAllUsers = 0; _engine->SetVariableNumeric(L"InstallLauncherAllUsers", includeLauncherAllUsers); } - if (includeLauncher == -1) { + if (includeLauncher < 0) { if (BOOTSTRAPPER_ACTION_LAYOUT == _command.action || (BOOTSTRAPPER_ACTION_INSTALL == _command.action && !_upgrading)) { // When installing/downloading, we include the launcher + // (though downloads should ignore this setting anyway) _engine->SetVariableNumeric(L"Include_launcher", 1); } else { - // Any other action, don't include by default + // Any other action, we should have detected an existing + // install (e.g. on remove/modify), so if we didn't, we + // assume it's not selected. _engine->SetVariableNumeric(L"Include_launcher", 0); _engine->SetVariableNumeric(L"AssociateFiles", 0); } } - if (associateFiles == -1) { + if (associateFiles < 0) { auto hr = LoadAssociateFilesStateFromKey( _engine, includeLauncherAllUsers ? HKEY_LOCAL_MACHINE : HKEY_CURRENT_USER diff --git a/Tools/msi/bundle/bundle.wxs b/Tools/msi/bundle/bundle.wxs index 95fd13a85bba9f3..abfeb88784890c8 100644 --- a/Tools/msi/bundle/bundle.wxs +++ b/Tools/msi/bundle/bundle.wxs @@ -30,7 +30,7 @@ - + From c20c1c99ddf79e8438ea7606a091032bed6e2c30 Mon Sep 17 00:00:00 2001 From: Steve Dower Date: Thu, 29 Feb 2024 21:45:51 +0000 Subject: [PATCH 5/5] Review feedback --- .../Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst | 4 ++-- .../msi/bundle/bootstrap/PythonBootstrapperApplication.cpp | 7 +++++-- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst index ec6691e5d0681b5..b3c078b578205e6 100644 --- a/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst +++ b/Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst @@ -1,5 +1,5 @@ -The installer now has more strict rules about updating :ref:`launcher`. In -general, most users only have a single launcher installed and will see no +The installer now has more strict rules about updating the :ref:`launcher`. +In general, most users only have a single launcher installed and will see no difference. When multiple launchers have been installed, the option to install the launcher is disabled until all but one have been removed. Downgrading the launcher (which was never allowed) is now more obviously diff --git a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp index e9d87717e94ca64..e0e179e3aede6d9 100644 --- a/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp +++ b/Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp @@ -735,10 +735,13 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { if (FAILED(BalGetNumericVariable(L"BlockedLauncher", &blockedLauncher))) { blockedLauncher = 0; } + + // Get the prior DetectedLauncher value so we can see if we've + // detected more than one, and then update the stored variable + // (we use the original value later on via the local). if (FAILED(BalGetNumericVariable(L"DetectedLauncher", &detectedLauncher))) { detectedLauncher = 0; } - if (!detectedLauncher) { _engine->SetVariableNumeric(L"DetectedLauncher", 1); } @@ -752,7 +755,7 @@ class PythonBootstrapperApplication : public CBalBaseBootstrapperApplication { BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Launcher will not be installed"); _engine->SetVariableNumeric(L"BlockedLauncher", 1); } - else if (detectedLauncher == 1) { + else if (detectedLauncher) { if (!blockedLauncher) { BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "Multiple launcher installs have been detected."); BalLog(BOOTSTRAPPER_LOG_LEVEL_ERROR, "No launcher will be installed or upgraded until one has been removed.");