GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — gh-109515: Allow a large number of frozen modules on Windows by rghe · Pull Request #109516 · python/cpython

Conversation

rghe commented Sep 17, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

This PR changes freezing modules compilation on windows by writing freezing module paths into a file instead of the command line.
Non-windows builds are unchanged

Issue: freeze_modules.py on windows hits command line limits when number of frozen modules grows #109515

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rghe and others added 2 commits

September 9, 2023 18:23


          fix windows command line limitations when freezing many modules

97c9b8c


          Merge branch 'python:main' into allow-large-number-of-frozen-modules-…

5d640cb

…vstudio

ghost commented Sep 17, 2023 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

All commit authors signed the Contributor License Agreement.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot added the awaiting review label

Sep 17, 2023

bedevere-app Bot commented Sep 17, 2023

Copy link

Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-app Bot mentioned this pull request

Sep 17, 2023

freeze_modules.py on windows hits command line limits when number of frozen modules grows #109515

Closed


          fix spacing

ddf9082

bedevere-app Bot commented Sep 17, 2023

Copy link

Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

blurb-it Bot and others added 2 commits

September 17, 2023 15:17


          📜🤖 Added by blurb_it.

9058b8d


          missing regen

e98c3d0

rghe requested a review from a team as a code owner

September 17, 2023 16:33

rghe added 3 commits

September 17, 2023 18:47


          add generated file

86d1818


          add buildpath-dependent generated files to .gitignore

ca98aa1


          try relative paths

5463e79

AA-Turner changed the title ~~gh-109515 - Allow large number of frozen modules even on windows~~ gh-109515: Allow a large number of frozen modules on Windows

Sep 21, 2023

AA-Turner added the skip news label

Sep 21, 2023


          fill mappings file with relative paths from vcxproj

7456fe5

zooba reviewed

Sep 21, 2023

View reviewed changes

Comment thread

PCbuild/_freeze_module.vcxproj Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

PCbuild/_freeze_module.vcxproj Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Misc/NEWS.d/next/Build/2023-09-17-15-17-53.gh-issue-109515.zpEECA.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

rghe and others added 4 commits

September 23, 2023 18:31


          Implement S.Dower review suggestions

031ea3e


          Merge branch 'main' into allow-large-number-of-frozen-modules-vstudio

e6fb7f5


          clean gitignore

b78bb97


          Merge branch 'main' into allow-large-number-of-frozen-modules-vstudio

93750fa

zooba reviewed

Sep 26, 2023

View reviewed changes

Comment thread

Tools/build/freeze_modules.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

zooba commented Sep 26, 2023

Copy link

Copy Markdown

Member

We always squash merge PRs and rewrite the commit messages, don't worry about the change history.

rghe reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Update Tools/build/freeze_modules.py

d584d2d

Co-authored-by: Steve Dower <[email protected]>

AA-Turner reviewed

Sep 26, 2023

View reviewed changes

Comment thread

Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/build/freeze_modules.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

rghe and others added 4 commits

September 26, 2023 19:28


          Update Tools/build/deepfreeze.py

48112ab

Co-authored-by: Adam Turner <[email protected]>


          Update Tools/build/deepfreeze.py

74a255a

Co-authored-by: Adam Turner <[email protected]>


          Update Tools/build/deepfreeze.py

ecc334f

Co-authored-by: Adam Turner <[email protected]>


          expand \t and align vcxfile by AA-Turner suggestion

bb42f32


          typo

4cc3458

zooba approved these changes

Sep 26, 2023

View reviewed changes

zooba left a comment

Copy link

Copy Markdown

Member

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'm okay with this, but I'm not the maintainer of this code. Not sure who cares about it most, but they can merge it (or I'll get it in a couple of weeks if nobody cares)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

PCbuild/_freeze_module.vcxproj Outdated

Comment on lines +377 to +378

		<ItemGroup>
		<!-- BEGIN freeze mappings -->

zooba Sep 26, 2023

Copy link

Copy Markdown

Member

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 would be nice to fix the indentation on these lines. I don't see where it is though.

Otherwise anyone who edits this file manually is likely to fix it and trigger regeneration.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-app Bot added awaiting core review and removed awaiting review labels

Sep 26, 2023

AA-Turner requested review from gvanrossum and kumaraditya303

September 26, 2023 20:48

AA-Turner commented Sep 26, 2023

Copy link

Copy Markdown

Member

Devguide lists Guido & Kumar as deepfreeze experts.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

AA-Turner reviewed

Sep 27, 2023

View reviewed changes

Comment thread

Tools/build/freeze_modules.py Outdated

		filterlines.append(' </None>')
		deepfreezerules.append(f'\t\t "$(PySourcePath){header}:{src.frozenid}" ^')
		deepfreezerules.append('\t\t "-o" "$(PySourcePath)Python\\deepfreeze\\deepfreeze.c"\'/>' )
		deepfreezemappings.append(f' <FrozenModule Include="$(PySourcePath)\\{header}" FrozenId="{src.frozenid}" />\n')

AA-Turner Sep 27, 2023

Copy link

Copy Markdown

Member

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 this would solve Steve's point re indentation

Suggested change

	deepfreezemappings.append(f' <FrozenModule Include="$(PySourcePath)\\{header}" FrozenId="{src.frozenid}" />\n')
	deepfreezemappings.append(f' <FrozenModule Include="$(PySourcePath)\\{header}" FrozenId="{src.frozenid}" />\n')

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

rghe Sep 27, 2023

Copy link

Copy Markdown

Contributor Author

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.

reformatted the vcxproj, now everything seems aligned

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          reformat vcxproj

1c20fcd

gvanrossum reviewed

Sep 27, 2023

View reviewed changes

gvanrossum left a comment

Copy link

Copy Markdown

Member

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.

Changes to deepfreeze.py LGTM (one nit for the help message). I'll leave the rest to Steve and Adam!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Tools/build/deepfreeze.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Tools/build/deepfreeze.py

5620f94

Co-authored-by: Guido van Rossum <[email protected]>

rghe commented Oct 28, 2023

Copy link

Copy Markdown

Contributor Author

I'm okay with this, but I'm not the maintainer of this code. Not sure who cares about it most, but they can merge it (or I'll get it in a couple of weeks if nobody cares)

ahem, ping?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba commented Oct 30, 2023

Copy link

Copy Markdown

Member

Thanks for the ping!

rghe reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

zooba merged commit 8eaa206 into python:main

Oct 30, 2023

bedevere-app Bot removed the awaiting core review label

Oct 30, 2023

ericsnowcurrently commented Oct 30, 2023

Copy link

Copy Markdown

Member

There are a bunch of buildbot failures, but they may be due to gh-110764.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rghe commented Oct 30, 2023

Copy link

Copy Markdown

Contributor Author

There are a bunch of buildbot failures, but they may be due to gh-110764.

well, I don´t think it's due to this change, since it affects only the pre-build frozen module generator on windows builds and all error messages seem to be "define _Py_ThreadId for this platform"

zooba and ericsnowcurrently reacted with thumbs up emoji

All reactions

👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

rghe deleted the allow-large-number-of-frozen-modules-vstudio branch

October 30, 2023 17:42

FullteaR pushed a commit to FullteaR/cpython that referenced this pull request

Nov 3, 2023


          pythongh-109515: When generating deep frozen modules on Windows, use …

af4ffa1

…a list file instead of arguments (pythonGH-109516)

aisk pushed a commit to aisk/cpython that referenced this pull request

Feb 11, 2024


          pythongh-109515: When generating deep frozen modules on Windows, use …

0cbe9f0

…a list file instead of arguments (pythonGH-109516)

Glyphack pushed a commit to Glyphack/cpython that referenced this pull request

Sep 2, 2024


          pythongh-109515: When generating deep frozen modules on Windows, use …

25d9fa0

…a list file instead of arguments (pythonGH-109516)

Reviewers

gvanrossum

gvanrossum left review comments

AA-Turner

AA-Turner left review comments

zooba

zooba approved these changes

kumaraditya303

Awaiting requested review from kumaraditya303

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

rghe commented Sep 17, 2023 • edited by bedevere-app Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Sep 17, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bedevere-app Bot commented Sep 17, 2023

Uh oh!

bedevere-app Bot commented Sep 17, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zooba commented Sep 26, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zooba left a comment

Choose a reason for hiding this comment

Uh oh!

zooba Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

AA-Turner commented Sep 26, 2023

Uh oh!

AA-Turner Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

rghe Sep 27, 2023

Choose a reason for hiding this comment

Uh oh!

gvanrossum left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rghe commented Oct 28, 2023

Uh oh!

zooba commented Oct 30, 2023

Uh oh!

ericsnowcurrently commented Oct 30, 2023

Uh oh!

rghe commented Oct 30, 2023

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rghe commented Sep 17, 2023 •

edited by bedevere-app Bot

Loading

Uh oh!

There was an error while loading. Please reload this page.

ghost commented Sep 17, 2023 •

edited by ghost

Loading

Uh oh!

There was an error while loading. Please reload this page.