Skip to content

gh-109515: Allow a large number of frozen modules on Windows#109516

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
zooba merged 21 commits into from
Oct 30, 2023
Merged

gh-109515: Allow a large number of frozen modules on Windows #109516

zooba merged 21 commits into from
Oct 30, 2023

Conversation

@rghe
Copy link
Contributor

@rgherghe commented Sep 17, 2023•
edited by bedevere-app bot
Loading

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

@ghost
Copy link

ghost commented Sep 17, 2023•
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-app
Copy link

bedevere-appbot commented Sep 17, 2023

Most changes to Python require a NEWS entry.

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

@bedevere-appbedevere-appbot mentioned this pull request Sep 17, 2023
Closed
@bedevere-app
Copy link

bedevere-appbot commented Sep 17, 2023

Most changes to Python require a NEWS entry.

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

@rgherghe requested a review from a team as a code ownerSeptember 17, 2023 16:33
@AA-TurnerAA-Turner changed the title gh-109515 - Allow large number of frozen modules even on windowsgh-109515: Allow a large number of frozen modules on WindowsSep 21, 2023
zooba
zooba reviewed Sep 21, 2023
View reviewed changes
zooba
zooba reviewed Sep 26, 2023
View reviewed changes
@zooba
Copy link
Member

zooba commented Sep 26, 2023

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

AA-Turner
AA-Turner reviewed Sep 26, 2023
View reviewed changes
zooba
zooba approved these changes Sep 26, 2023
View reviewed changes
Copy link
Member

@zoobazooba left a 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)

PCbuild/_freeze_module.vcxproj Outdated
Comment on lines 377 to 378
<ItemGroup>
<!-- BEGIN freeze mappings -->
Copy link
Member

@zoobazoobaSep 26, 2023

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.

@AA-Turner
Copy link
Member

AA-Turner commented Sep 26, 2023

Devguide lists Guido & Kumar as deepfreeze experts.

A

AA-Turner
AA-Turner reviewed Sep 27, 2023
View reviewed changes
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')
Copy link
Member

@AA-TurnerAA-TurnerSep 27, 2023

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')

Copy link
ContributorAuthor

@rghergheSep 27, 2023

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

gvanrossum
gvanrossum reviewed Sep 27, 2023
View reviewed changes
Copy link
Member

@gvanrossumgvanrossum left a 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!

@rghe@gvanrossum
Update Tools/build/deepfreeze.py
5620f94
Co-authored-by: Guido van Rossum <[email protected]>
@rghe
Copy link
ContributorAuthor

rghe commented Oct 28, 2023

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?

@zooba
Copy link
Member

zooba commented Oct 30, 2023

Thanks for the ping!

@zoobazooba merged commit 8eaa206 into python:mainOct 30, 2023
@ericsnowcurrently
Copy link
Member

ericsnowcurrently commented Oct 30, 2023

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

@rghe
Copy link
ContributorAuthor

rghe commented Oct 30, 2023

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"

@rgherghe 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
@rghe@FullteaR
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
@rghe@aisk
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
@rghe@Glyphack
pythongh-109515: When generating deep frozen modules on Windows, use …
25d9fa0
…a list file instead of arguments (pythonGH-109516)
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossumgvanrossumgvanrossum left review comments

@AA-TurnerAA-TurnerAA-Turner left review comments

@zoobazoobazooba approved these changes

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@rghe@zooba@AA-Turner@ericsnowcurrently@gvanrossum