gh-115554: Improved logic for handling multiple existing py.exe launcher installs #115793

zooba · 2024-02-21T23:50:02Z

Issue: Python Install Fails Due To Newer Launcher #115554

… launcher installs

gpshead

at a high level I think this makes sense. I can look past the specific api c++ boilerplate for what really seems to just be a way to spell get and set these variables from the whatever-script maybe functionalish installer state machine runtime ultimately running this logic.

My comments are mostly code stuff, I trust that this does what it describes itself as doing. But I'm definitely not a windows installer expert (I last created one over 20 years ago).

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp

gpshead · 2024-02-29T18:41:35Z

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp

+                // 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))) {


The _ in that L" Variable name when most others don't appear use that style it is unfortunate... but i'm guessing this comes from elsewhere in the XML definition or whatnot and is just style would require larger refactoring beyond the scope of this PR better done as its own no-functionality change if ever.

It's actually consistent with the other Include_<name> variables, which all refer to a <name>.msi file. It's just none of the others need to be referenced as much because they're all easy.

Here they are all in a list:

cpython/Tools/msi/bundle/bundle.wxs

Lines 68 to 77 in 41d5391

<Variable Name="Include_core" Value="1" />

<Variable Name="Include_exe" Value="1" bal:Overridable="yes" />

<Variable Name="Include_dev" Value="1" bal:Overridable="yes" />

<Variable Name="Include_lib" Value="1" bal:Overridable="yes" />

<Variable Name="Include_test" Value="1" bal:Overridable="yes" />

<Variable Name="Include_doc" Value="1" bal:Overridable="yes" />

<Variable Name="Include_tools" Value="0" bal:Overridable="yes" />

<Variable Name="Include_tcltk" Value="1" bal:Overridable="yes" />

<Variable Name="Include_pip" Value="1" bal:Overridable="yes" />

<Variable Name="Include_launcher" Value="-1" bal:Overridable="yes" />

itamaro

this seems a bit convoluted and hard to follow (maybe a combination of installer-toolkit-isms and non-trivial business logic) -- it might benefit from a flowchart!

can you clarify what would be the behavior when both 3.12.1 & 3.11.5 are installed, and we attempt to upgrade both to 3.12.2 & 3.11.8? (arbitrary patch releases)
in particular, I'm curious about the scenario of unattended installations that attempt to upgrade both (and can happen in unspecified order on different machines).

Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp

zooba · 2024-02-29T21:31:56Z

this seems a bit convoluted and hard to follow

Agreed! It hurt my head figuring out the right places to put the logic.

can you clarify what would be the behavior when both 3.12.1 & 3.11.5 are installed, and we attempt to upgrade both to 3.12.2 & 3.11.8? (arbitrary patch releases)

So the important aspect for this change isn't which Python version(s) are installed, but which launcher version. There's only one launcher installer, so there should only ever be one version installed. Multiple is a bad enough error that we now refuse to do anything.

If we assume 3.12.1 and 3.11.5 are installed correctly, then launcher 3.12.1 will be installed. If we then install 3.11.8, it'll compare its launcher 3.11.8 to 3.12.1 and decide that's a downgrade, and so will take the downgrade path in the new code (which deselects the launcher and disables the controls).

If we're installing 3.12.2, it'll compare its launcher 3.12.2 to 3.12.1 and decide it's a major upgrade, and so will select the launcher and update the "launcher for all users" option to match the existing install (but only if the option wasn't specified on the command line, in which case we disable the launcher entirely if they conflict).

If a second launcher is installed, this typically means one is per-user and one is per-machine, which we can't handle, so we block the launcher install entirely. This will update the checkbox on the main page to say that the launcher has to be uninstalled in order to update.

If no launchers are detected, we update the default options (-1) to what we want for our standard default (i.e. to include the launcher). It's possible to override these on the command line though, so we want to respect those selections.

It's not much of a flowchart, but you can probably imagine "OnPackageDetected" getting called once for every package that's already installed, and "OnDetectionComplete" being called once at the end.

itamaro

the Windows installer could probably benefit from an end-to-end test suite, with all the complexity and possible scenarios, but I can't imagine writing such a test suite would be easy, and it'll probably be excruciatingly slow and flaky...

zooba · 2024-03-01T00:54:14Z

I can't imagine writing such a test suite would be easy, and it'll probably be excruciatingly slow and flaky...

I just tried getting the smoke tests that are part of the installer build to run more reliably (GitHub installs Python on the build machines, so the installers can't really be tested when they're already installed 😅) and yeah, slow and flaky. But we do run the basics through their paces on every release - just not through every possible combination.

However, most combinations shouldn't have interactions, and the problems almost always come up because of preexisting things on peoples' machines, and not because of the installer itself. So it becomes a case of scripting the setup of broken machines so that we can run the test and ... err... yeah... it's a huge amount of work for not actually a lot of benefit.

miss-islington-app · 2024-03-01T12:58:31Z

Thanks @zooba for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

… launcher installs (pythonGH-115793) (cherry picked from commit 9b7f253) Co-authored-by: Steve Dower <steve.dower@python.org>

miss-islington-app · 2024-03-01T12:58:37Z

Sorry, @zooba, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 9b7f253b55f10df03d43c8a7c2da40ea523ac7a1 3.11

bedevere-app · 2024-03-01T12:58:38Z

GH-116200 is a backport of this pull request to the 3.12 branch.

bedevere-app · 2024-03-01T13:02:41Z

GH-116201 is a backport of this pull request to the 3.11 branch.

… launcher installs (pythonGH-115793)

bedevere-bot · 2024-03-01T13:52:30Z

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot AMD64 Windows11 Non-Debug 3.x has failed when building commit 9b7f253.

What do you need to do:

Don't panic.
Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/914/builds/3574) and take a look at the build logs.
Check if the failure is related to this commit (9b7f253) or if it is a false positive.
If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/914/builds/3574

Failed tests:

test_wmi

Failed subtests:

test_wmi_query_threads - test.test_wmi.WmiTests.test_wmi_query_threads

Summary of the results of the build (if available):

==

Click to see traceback logs

Traceback (most recent call last):
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_wmi.py", line 86, in test_wmi_query_threads
    self.assertRegex(t.result(), "ProcessId=")
                     ~~~~~~~~^^
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\concurrent\futures\_base.py", line 456, in result
    return self.__get_result()
           ~~~~~~~~~~~~~~~~~^^
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\concurrent\futures\_base.py", line 401, in __get_result
    raise self._exception
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\concurrent\futures\thread.py", line 58, in run
    result = self.fn(*self.args, **self.kwargs)
             ~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "b:\uildarea\3.x.ware-win11.nondebug\build\Lib\test\test_wmi.py", line 16, in wmi_exec_query
    return _wmi.exec_query(query)
           ~~~~~~~~~~~~~~~^^^^^^^
BrokenPipeError: [WinError -2147024664] The pipe is being closed

… launcher installs (pythonGH-115793)

…her installs (GH-115793) (cherry picked from commit 9b7f253) Co-authored-by: Steve Dower <steve.dower@python.org>

… launcher installs (pythonGH-115793)

pythongh-115554: Improved logic for handling multiple existing py.exe…

01c8642

… launcher installs

bedevere-app bot mentioned this pull request Feb 21, 2024

Python Install Fails Due To Newer Launcher #115554

Closed

Improved logic

62c0218

zooba marked this pull request as ready for review February 22, 2024 23:34

zooba requested a review from a team as a code owner February 22, 2024 23:34

bedevere-app bot added the awaiting core review label Feb 22, 2024

Improved NEWS

02126f0

zooba added OS-windows needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Feb 23, 2024

Changes from self code review

d52ee26

gpshead reviewed Feb 29, 2024

View reviewed changes

itamaro reviewed Feb 29, 2024

View reviewed changes

Misc/NEWS.d/next/Windows/2024-02-21-23-48-59.gh-issue-115554.02mpQC.rst Outdated Show resolved Hide resolved

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp Show resolved Hide resolved

Tools/msi/bundle/bootstrap/PythonBootstrapperApplication.cpp Show resolved Hide resolved

Review feedback

c20c1c9

itamaro approved these changes Feb 29, 2024

View reviewed changes

zooba merged commit 9b7f253 into python:main Mar 1, 2024

bedevere-app bot removed the awaiting core review label Mar 1, 2024

zooba deleted the gh-115554 branch March 1, 2024 12:58

miss-islington-app bot assigned zooba Mar 1, 2024

bedevere-app bot removed the needs backport to 3.12 only security fixes label Mar 1, 2024

bedevere-app bot removed the needs backport to 3.11 only security fixes label Mar 1, 2024

zooba added a commit to zooba/cpython that referenced this pull request Mar 1, 2024

pythongh-115554: Improved logic for handling multiple existing py.exe…

fb7d560

… launcher installs (pythonGH-115793)

woodruffw pushed a commit to woodruffw-forks/cpython that referenced this pull request Mar 4, 2024

pythongh-115554: Improved logic for handling multiple existing py.exe…

406d3b5

… launcher installs (pythonGH-115793)

zooba added a commit that referenced this pull request Mar 5, 2024

gh-115554: Improved logic for handling multiple existing py.exe launc…

34efd49

…her installs (GH-115793) (cherry picked from commit 9b7f253) Co-authored-by: Steve Dower <steve.dower@python.org>

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024

pythongh-115554: Improved logic for handling multiple existing py.exe…

27d3e4c

… launcher installs (pythonGH-115793)

diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024

pythongh-115554: Improved logic for handling multiple existing py.exe…

5001555

… launcher installs (pythonGH-115793)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

gh-115554: Improved logic for handling multiple existing py.exe launcher installs #115793

gh-115554: Improved logic for handling multiple existing py.exe launcher installs #115793

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

	<Variable Name="Include_core" Value="1" />
	<Variable Name="Include_exe" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_dev" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_lib" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_test" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_doc" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_tools" Value="0" bal:Overridable="yes" />
	<Variable Name="Include_tcltk" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_pip" Value="1" bal:Overridable="yes" />
	<Variable Name="Include_launcher" Value="-1" bal:Overridable="yes" />

Uh oh!

gh-115554: Improved logic for handling multiple existing py.exe launcher installs #115793

gh-115554: Improved logic for handling multiple existing py.exe launcher installs #115793

Uh oh!

Conversation

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Uh oh!

Uh oh!