Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-101135: Windows launcher: Add backwards compatibility for older 32-bit versions#101138

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
zooba merged 11 commits intopython:mainfromboimart1:fix-issue-101135
Jan 24, 2023

Conversation

@boimart1
Copy link
Contributor

@boimart1boimart1 commentedJan 18, 2023
edited
Loading

py -0p output, with this fix:

image

For 32-bit installs, these versions did not contain the "-32" in their registry name, so the 32 and 64-bit installs were treated equal.

Additionally, there were a few related bugs in the code to add nodes to the EnvironmentInfo tree:

  • All nodes'parents were alwaysNULL
  • If two nodes compared equal, the one one was "chosen" (the sort key condition was reversed). However, due to the NULL parent problem, nodes were never actually replaced in the tree.
  • Even when a node was meant to be replaced, there was nofreeEnvironmentInfo() on the removed node.

Finally,_listAllEnvironments now displays-32 in the tag for PythonCore 32-bit versions which don't have the suffix already.

An alternative fix would have been to add-32 to the tag name for PythonCore 32-bit versions which don't have it in the name. I can go back and implement it that way instead if this approach seems too complicated -- however, that would involve adding a few functions to properly update the tag without leaking.

For 32-bit installs, these versions did not contain the "-32" in their registry name, so the 32 and 64-bit installs were treated equal. Additionally, the code to replace a node with one with a lower sort key was buggy (wrong node chosen, replace never happened since parent was always NULL, replaced node never freed, etc)
@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@ghost
Copy link

ghost commentedJan 18, 2023
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@zooba
Copy link
Member

An alternative fix would have been to add -32 to the tag name for PythonCore 32-bit versions which don't have it in the name. I can go back and implement it that way instead if this approach seems too complicated -- however, that would involve adding a few functions to properly update the tag without leaking.

I'd prefer this fix. Ideally this would be easy to remove when we decide that weactually don't care about end-of-life versions, since apparently it's too soon for that. Use theallocSearchInfoBuffer function to allocate a new buffer - these get attached to theSearchInfo structure and won't leak.

@zooba
Copy link
Member

env.architecture isn't actually a good enough check if you care about EOL'd versions, you need to addGetBinaryType calls for when searching the HKCU registry key and assume 64-bit/32-bit based on the WOW6432Node selection for HKLM keys. It also should definitely be constrained to versions that definitely don't have-32 in the registry key, which IIRC is 3.4 and earlier? Possibly 3.5 as well, but I think I changed it in 3.5. Anything since then should follow the PEP 514 rules excluding back-compat allowances.

@zooba
Copy link
Member

Theparent fixes are good though, thanks! I thought I had tests that would ensure it was handling duplicated nodes properly, but I guess not.

@boimart1
Copy link
ContributorAuthor

boimart1 commentedJan 18, 2023
edited
Loading

env.architecture isn't actually a good enough check if you care about EOL'd versions, you need to add GetBinaryType calls for when searching the HKCU registry key and assume 64-bit/32-bit based on the WOW6432Node selection for HKLM keys

For the HKLM entries, I believe this is already covered by thefallbackArch argument, which setsenv.architecture to the correct value based on which registry section was searched.

cpython/PC/launcher2.c

Lines 1489 to 1491 ine9d5bb9

if (!env->architecture&&env->executablePath&&fallbackArch) {
copyWstr(&env->architecture,fallbackArch);
}

Good point on the HKCU entries though. I'll see where I can fit that in.

So, to recap my next changes:

  • add detection of 32bit vs 64bit to HKCU entries,
  • add "-32" to the tag name of the 32-bit entries which don't have it,
  • constrain these two checks to PythonCore 2.X and 3.5 and lower,
  • remove_compareArch, since architecture comparison will be implicit with the "-32" suffix always being present in the tag name,
  • also remove the new code in_listAllEnvironments which appended "-32".

@zooba
Copy link
Member

That sounds good.

Also verify that a command likepy -2.7 will select2.7-32 if that's the only option. Itshould, but my confidence in all my contributions over the last few months has been pretty well shattered, so check my work :)

boimart1 reacted with laugh emoji

@boimart1
Copy link
ContributorAuthor

boimart1 commentedJan 23, 2023
edited
Loading

Did a bunch of manual testing with 3.4 (fully pre-PEP 514) and 3.5 (includes executablePath and the "-32" in the tag for 32bit installations, but not full PEP 514), plus a few tests with 3.6 (follows PEP 514). I made sure to have the following behavior, as written inPEP 397:

  • py -X.Y uses the 64-bit installation if there is one, 32-bit otherwise
  • py -X.Y-64 uses the 64-bit installation, fails if there is none
  • py -X.Y-32 uses the 32-bit installation, fails if there is none

In all cases, if the same version+architecture exists in both HKCU and HKLM, the HKCU entry is preferred.

zooba reacted with thumbs up emoji

@zooba
Copy link
Member

Excellent! Thanks for the great work here.

boimart1 reacted with rocket emoji

@zoobazooba merged commitdaec3a4 intopython:mainJan 24, 2023
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-101290 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelJan 24, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestJan 24, 2023
…older 32-bit versions (pythonGH-101138)Python 2.x and up to 3.4 did not contain the "-32" in their registry name, so the 32 and 64-bit installs were treated equal. Since 3.5/PEP 514 this is no longer true, but we still want to detect the EOL versions correctly in case people are still using them.Additionally, the code to replace a node with one with a lower sort key was buggy (wrong node chosen, replace never happened since parent was always NULL, replaced node never freed, etc)(cherry picked from commitdaec3a4)Co-authored-by: Martin Boisvert <martin.boisvert@optelgroup.com>
@boimart1boimart1 deleted the fix-issue-101135 branchJanuary 24, 2023 16:39
miss-islington added a commit that referenced this pull requestJan 24, 2023
…32-bit versions (GH-101138)Python 2.x and up to 3.4 did not contain the "-32" in their registry name, so the 32 and 64-bit installs were treated equal. Since 3.5/PEP 514 this is no longer true, but we still want to detect the EOL versions correctly in case people are still using them.Additionally, the code to replace a node with one with a lower sort key was buggy (wrong node chosen, replace never happened since parent was always NULL, replaced node never freed, etc)(cherry picked from commitdaec3a4)Co-authored-by: Martin Boisvert <martin.boisvert@optelgroup.com>
@bedevere-bot
Copy link

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

Hi! The buildbots390x Fedora LTO 3.x has failed when building commitdaec3a4.

What do you need to do:

  1. Don't panic.
  2. Checkthe buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/55/builds/2908) and take a look at the build logs.
  4. Check if the failure is related to this commit (daec3a4) or if it is a false positive.
  5. 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/55/builds/2908

Failed tests:

  • test_asyncio

Failed subtests:

  • test_subprocess_consistent_callbacks - test.test_asyncio.test_subprocess.SubprocessPidfdWatcherTests.test_subprocess_consistent_callbacks

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

== Tests result: FAILURE then FAILURE ==

413 tests OK.

10 slowest tests:

  • test_gdb: 2 min 19 sec
  • test_concurrent_futures: 2 min 18 sec
  • test_tools: 2 min 14 sec
  • test_multiprocessing_spawn: 1 min 32 sec
  • test_signal: 1 min 13 sec
  • test_multiprocessing_forkserver: 1 min 10 sec
  • test_asyncio: 1 min 8 sec
  • test_multiprocessing_fork: 1 min 5 sec
  • test_socket: 41.4 sec
  • test_math: 35.8 sec

1 test failed:
test_asyncio

19 tests skipped:
test_check_c_globals test_devpoll test_ioctl test_kqueue
test_launcher test_msilib test_nis test_peg_generator
test_perf_profiler test_readline test_startfile test_tix
test_tkinter test_ttk test_winconsoleio test_winreg test_winsound
test_wmi test_zipfile64

1 re-run test:
test_asyncio

Total duration: 5 min 23 sec

Click to see traceback logs
Traceback (most recent call last):  File"/home/dje/cpython-buildarea/3.x.edelsohn-fedora-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line771, intest_subprocess_consistent_callbacksself.loop.run_until_complete(main())  File"/home/dje/cpython-buildarea/3.x.edelsohn-fedora-z.lto/build/Lib/asyncio/base_events.py", line664, inrun_until_completereturn future.result()^^^^^^^^^^^^^^^  File"/home/dje/cpython-buildarea/3.x.edelsohn-fedora-z.lto/build/Lib/test/test_asyncio/test_subprocess.py", line763, inmainself.assertEqual(events, [AssertionError:Lists differ: [('pi[69 chars]), 'process_exited', 'pipe_connection_lost', '[17 chars]ost'] != [('pi[69 chars]), 'pipe_connection_lost', 'pipe_connection_lo[17 chars]ted']

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

@zoobazooba

Labels

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@boimart1@bedevere-bot@zooba@miss-islington

[8]ページ先頭

©2009-2025 Movatter.jp