GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45949: Pure Python freeze module for cross builds (GH-29899) by tiran · Pull Request #29899 · python/cpython

Conversation

@tiran

@tiran tiran commented Dec 2, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Use _bootstrap_python interpreter and pure Python implementation of
freeze_module to generate frozen byte code files. Only importlib
bootstrap files are generated with Programs/_freeze_module.

This simplifies cross building, as the build system no longer needs a
_freeze_module binary. A standard Python installation with same
version is sufficient.

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue45949

Sorry, something went wrong.

Uh oh!

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

All reactions
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Dec 2, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Dec 2, 2021
@gvanrossum

gvanrossum commented Dec 2, 2021

Copy link
Copy Markdown
Member

Where would the frozen importlib files come from? Are you adding those back to git? (Otherwise I don't see how this helps.)

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Dec 2, 2021

Copy link
Copy Markdown
Member Author

Where would the frozen importlib files come from? Are you adding those back to git? (Otherwise I don't see how this helps.)

  • For non-cross builds
    • Python/frozen_modules/importlib._bootstrap* are generated with Program/_freeze_module. The files are needed to create _bootstrap_python.
    • other Python/frozen_modules/*.h are generated with _bootstrap_python and freeze_module.py
  • For cross builds all Python/frozen_modules/*.h are generated with an host Python interpreter (must have same version) and freeze_module.py. Cross builds can neither use ./_bootstrap_python nor Program/_freeze_module from the current build because all files are for a different architecture or platform.
All reactions

Sorry, something went wrong.

Uh oh!

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

@gvanrossum

gvanrossum commented Dec 2, 2021

Copy link
Copy Markdown
Member

Cross builds don't just need the same version -- they need to use the same commit, unless we're in beta or beyond.

I'm not sure what's the point of ever using freeze_module.py for non-cross builds. Is it just so that freeze_module.py gets exercised regularly?

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Dec 2, 2021

Copy link
Copy Markdown
Member Author

Cross builds don't just need the same version -- they need to use the same commit, unless we're in beta or beyond.

Good point, I'll update the documentation.

I'm not sure what's the point of ever using freeze_module.py for non-cross builds. Is it just so that freeze_module.py gets exercised regularly?

Yes, you are correct. It is not necessary to use freeze_module.py for standard builds. It is the easiest way to test that the code does not regress. I'll add a comment.

gvanrossum 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.

@tiran

tiran commented Dec 2, 2021 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

The main point of the PR is to get rid of ./configure --with-freeze-module=Programs/_freeze_module again. It makes cross compiling more complicated. Users need to provide the helper tool for the build platform.

This patch removes the dependency. Users will be able to cross build a 3.11 interpreter with a 3.11 host Python (once byte code and API has stabilized in beta).

All reactions

Sorry, something went wrong.

Uh oh!

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

@kumaraditya303

kumaraditya303 commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

How about committing the importlib bootstrap related files to git and generate the rest with pure python version on both standard and cross builds provided the bytecode is same, this would simplify things a lot ?

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Dec 3, 2021

Copy link
Copy Markdown
Member Author

How about committing the importlib bootstrap related files to git and generate the rest with pure python version on both standard and cross builds provided the bytecode is same, this would simplify things a lot ?

In the past we had the importlib bootstrap files in git. The approach has the downside that it increases the size of git history a lot. Really a lot a lot.

The files are fairly large, about 350k in total. They also must be regenerated every time the source Python files are updated or any aspect of the byte code generator and optimizer is changed. Even a tiny change results in a large diff, often affecting all lines of the header files.

All reactions

Sorry, something went wrong.

Uh oh!

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

@kumaraditya303

kumaraditya303 commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

In the past we had the importlib bootstrap files in git. The approach has the downside that it increases the size of git history a lot. Really a lot a lot.

Yes, it will increase the size of git history but does anyone clones the repo in full ?, for builds most would do a shallow clone.

The files are fairly large, about 350k in total. They also must be regenerated every time the source Python files are updated or any aspect of the byte code generator and optimizer is changed. Even a tiny change results in a large diff, often affecting all lines of the header files.

For diff, these files can be marked as linguist-generated then github would suppress them in PRs

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Dec 3, 2021

Copy link
Copy Markdown
Member Author

Yes, it will increase the size of git history but does anyone clones the repo in full ?, for builds most would do a shallow clone.

I have a full clone and usually everybody else with a local checkout has a full clone as well.

erlend-aasland 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.

@arhadthedev

arhadthedev commented Dec 3, 2021

Copy link
Copy Markdown
Member

For diff, these files can be marked as linguist-generated then github would suppress them in PRs

It opens a great possibility to introduce a vulnerability into the Python codebase. Huge (especially suppressed) diffs are hard to analyze so they allow alterations of a generated file that will go unnoticed.

In addition, GitHub is just a part of a development conveyor. Before it there ara local non-Github tools (git diff/gitk, TortoiseGit, Sublime Merge, and so on) that do not support linguist-generated=true extension.

[...] but does anyone clones the repo in full?

Actually this is the very purpose of Git as a distributed version control system, to distribute full clones and work with them offline.

All reactions

Sorry, something went wrong.

Uh oh!

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

@kumaraditya303

kumaraditya303 commented Dec 3, 2021

Copy link
Copy Markdown
Contributor

It opens a great possibility to introduce a vulnerability into the Python codebase. Huge (especially suppressed) diffs are hard to analyze so they allow alterations of a generated file that will go unnoticed.

If generated files were to be added to the repo then it would have been verified on CI that the generated files and files generated on CI are equal.

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran

tiran commented Dec 3, 2021

Copy link
Copy Markdown
Member Author

If generated files were to be added to the repo then it would have been verified on CI that the generated files and files generated on CI are equal.

You would also have to make sure that non of the validating tools rely on the generated files.

All reactions

Sorry, something went wrong.

Uh oh!

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

@arhadthedev

arhadthedev commented Dec 3, 2021

Copy link
Copy Markdown
Member

If generated files were to be added to the repo then it would have been verified on CI that the generated files and files generated on CI are equal.

If bots are/will be involved, then I agree and retract my note on vulnerability planting.

In fact, I am aware that bootstrapping in a bare environment with no predecessor (so _bootstrap_python minimal interpreter is introduced) is a can of worms that force hardly bearable compromises.

All reactions

Sorry, something went wrong.

Uh oh!

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

@arhadthedev

arhadthedev commented Dec 3, 2021

Copy link
Copy Markdown
Member

Indeed, maintainability of the diffs is still a question. Even with suppression, unanalysable black boxes carried along with actual proposed changed is not a good idea.

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran
tiran force-pushed the bpo-45949-pyfreeze branch from 7d9cda1 to dce68d0 Compare December 3, 2021 15:25
@tiran
tiran marked this pull request as ready for review December 3, 2021 15:25
@tiran

tiran commented Dec 3, 2021

Copy link
Copy Markdown
Member Author

I have added more comments to explain the bootstrap process.

Sorry for the squash merge + force push. I ran into merge conflicts and eventually gave up. Merge was less work.

All reactions

Sorry, something went wrong.

Uh oh!

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

@tiran
tiran force-pushed the bpo-45949-pyfreeze branch 2 times, most recently from f7e07fd to 2a44284 Compare December 3, 2021 18:36
@tiran tiran changed the title bpo-45949: Pure Python freeze module bpo-45949: Pure Python freeze module for cross builds Dec 5, 2021
@tiran
tiran force-pushed the bpo-45949-pyfreeze branch 4 times, most recently from 454514c to 55e1527 Compare December 9, 2021 14:26
@tiran
tiran requested a review from ericsnowcurrently December 9, 2021 14:30
ericsnowcurrently
ericsnowcurrently approved these changes Dec 10, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

Mostly LGTM.

I've left a few minor comments. The only matter of consequence is about make rule dependencies in the cross-compiling case, as well as running make regen-frozen as one of the steps for a cross-compiled build.

I'm approving the PR under the assumption that you'll resolve my concerns before merging. 🙂

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Makefile.pre.in Resolved

Uh oh!

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

Comment thread Makefile.pre.in Resolved

Uh oh!

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

Comment thread Makefile.pre.in Resolved

Uh oh!

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

Comment thread Makefile.pre.in Outdated
Comment on lines 1119 to 1138

@ericsnowcurrently ericsnowcurrently Dec 10, 2021

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.

Instead of "manually", can this be generated now? (added to Tools/scripts/freeze_modules.py)

Sorry, something went wrong.

Uh oh!

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

All reactions

@tiran tiran Dec 11, 2021

Copy link
Copy Markdown
Member 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.

getpath is not like the others. It would need special handling in Tools/scripts/freeze_modules.py, too. Let's tackle the getpath target in another PR. I only moved existing block to a different location so it is closer to other early bootstrap blocks.

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
Comment thread Tools/scripts/freeze_modules.py Resolved

Uh oh!

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

Comment thread Programs/_freeze_module.py Outdated

@ericsnowcurrently ericsnowcurrently Dec 10, 2021

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.

Would you mind adding a short note here (or even a TODO) about the relationship with Tools/freeze/freeze.py?

Sorry, something went wrong.

Uh oh!

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

All reactions

@tiran tiran Dec 11, 2021

Copy link
Copy Markdown
Member 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.

Good idea!

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Programs/_freeze_module.py Resolved

Uh oh!

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

Comment thread Programs/_freeze_module.py Resolved

Uh oh!

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

Comment thread Programs/_freeze_module.py Resolved

Uh oh!

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

@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Dec 10, 2021
@ericsnowcurrently

ericsnowcurrently commented Dec 10, 2021

Copy link
Copy Markdown
Member

and sorry for the delay in getting you a review!

All reactions

Sorry, something went wrong.

Uh oh!

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

tiran added 2 commits December 11, 2021 10:47
@tiran
bpo-45949: Pure Python freeze module for cross builds
85dd6ec
Use `_bootstrap_python` interpreter and pure Python implementation of
`freeze_module` to generate frozen byte code files. Only importlib
bootstrap files are generated with `Programs/_freeze_module`.

This simplifies cross building, as the build system no longer needs a
`_freeze_module` binary. A standard Python installation with same
version is sufficient.

Signed-off-by: Christian Heimes <[email protected]>
@tiran
Move script next to _freeze_module.c
df657a9
@tiran
tiran force-pushed the bpo-45949-pyfreeze branch from 55e1527 to 4fd065f Compare December 11, 2021 16:41
@tiran
Address review comments
0d328a9
- Fix typo
- Remove confusing comment
- Improve dependency handling for freezing
@tiran
tiran force-pushed the bpo-45949-pyfreeze branch from 4fd065f to 0d328a9 Compare December 11, 2021 16:42
@tiran

tiran commented Dec 13, 2021

Copy link
Copy Markdown
Member Author

@ericsnowcurrently Are you satisfied with the current state of the patch?

All reactions

Sorry, something went wrong.

Uh oh!

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

ericsnowcurrently
ericsnowcurrently approved these changes Dec 13, 2021
View reviewed changes

@ericsnowcurrently ericsnowcurrently 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.

LGTM

Sorry, something went wrong.

Uh oh!

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

All reactions
@tiran tiran changed the title bpo-45949: Pure Python freeze module for cross builds bpo-45949: Pure Python freeze module for cross builds (GH-29899) Dec 13, 2021
@tiran
tiran merged commit eb483c4 into python:main Dec 13, 2021
@tiran
tiran deleted the bpo-45949-pyfreeze branch December 13, 2021 19:48
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 13, 2021
@bedevere-bot

bedevere-bot commented Dec 13, 2021

Copy link
Copy Markdown

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

Hi! The buildbot AMD64 Windows8.1 Non-Debug 3.x has failed when building commit eb483c4.

What do you need to do:

  1. Don't panic.
  2. Check the 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/405/builds/831) and take a look at the build logs.
  4. Check if the failure is related to this commit (eb483c4) 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/405/builds/831

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

== Tests result: ENV CHANGED ==

400 tests OK.

10 slowest tests:

  • test_peg_generator: 2 min 30 sec
  • test_mmap: 2 min 25 sec
  • test_io: 2 min
  • test_multiprocessing_spawn: 1 min 59 sec
  • test_concurrent_futures: 1 min 43 sec
  • test_largefile: 1 min 38 sec
  • test_mailbox: 1 min 13 sec
  • test_asyncio: 59.8 sec
  • test_unparse: 45.5 sec
  • test_socket: 43.0 sec

1 test altered the execution environment:
test_asyncio

30 tests skipped:
test_curses test_dbm_gnu test_dbm_ndbm test_devpoll test_epoll
test_fcntl test_fork1 test_gdb test_grp test_ioctl test_kqueue
test_multiprocessing_fork test_multiprocessing_forkserver test_nis
test_openpty test_ossaudiodev test_pipes test_poll test_posix
test_pty test_pwd test_readline test_resource test_spwd
test_syslog test_threadsignals test_wait3 test_wait4
test_xxtestfuzz test_zipfile64

Total duration: 10 min 8 sec

Click to see traceback logs
Traceback (most recent call last):
  File "D:\buildarea\3.x.ware-win81-release.nondebug\build\Lib\asyncio\proactor_events.py", line 116, in __del__
    self.close()
    ^^^^^^^^^^^^
  File "D:\buildarea\3.x.ware-win81-release.nondebug\build\Lib\asyncio\proactor_events.py", line 108, in close
    self._loop.call_soon(self._call_connection_lost, None)
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "D:\buildarea\3.x.ware-win81-release.nondebug\build\Lib\asyncio\base_events.py", line 745, in call_soon
    self._check_closed()
    ^^^^^^^^^^^^^^^^^^^^
  File "D:\buildarea\3.x.ware-win81-release.nondebug\build\Lib\asyncio\base_events.py", line 506, in _check_closed
    raise RuntimeError('Event loop is closed')
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
RuntimeError: Event loop is closed
k
All reactions

Sorry, something went wrong.

Uh oh!

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

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

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

None yet

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.

7 participants

@tiran @gvanrossum @kumaraditya303 @arhadthedev @ericsnowcurrently @bedevere-bot @the-knights-who-say-ni
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.