gh-122188: Move magic number to its own file #122243

mdboom · 2024-07-24T19:35:36Z

Issue: Move opcode magic number out of Lib/importlib/_bootstrap_external.py #122188

📚 Documentation preview 📚: https://cpython-previews--122243.org.readthedocs.build/

Doc/library/sys.rst

mdboom · 2024-07-25T13:12:24Z

Include/internal/pycore_magic_number.h

+can be named after the Python major version of the magic number bump, but
+it can really be anything, as long as it's different than anything else
+that's come before.  The tags are included in the following table, starting
+with Python 3.2a0.


I just moved this comment here from the other file, but I did notice this last sentence seems to be no longer true. There are two 3.2 magic tags mentioned below but that's it. I think it's just entirely based on version number now. Does it make sense to remove this paragraph?

brettcannon · 2024-07-25T20:40:13Z

Python/sysmodule.c

+    value = PyLong_FromLong(PYC_MAGIC_NUMBER);
+    if (value == NULL)
+        goto error;
+    res = PyDict_SetItemString(impl_info, "pyc_magic_number", value);


I'm wondering if sys.implementation is the best place for this? https://docs.python.org/3/library/sys.html#sys.implementation has all attributes being required for all implementations and bytecode is not necessary to implement Python. Should it just be a private attribute in sys instead?

According to those docs, this can't be added as sys.implementation.pyc_magic_number without a PEP. sys.implementation._pyc_magic_number (left undocumented) would be fine, though.

I've made this private.

bedevere-bot · 2024-07-26T12:06:16Z

🤖 New build scheduled with the buildbot fleet by @mdboom for commit 2cc27f0 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot · 2024-07-26T12:10:01Z

🤖 New build scheduled with the buildbot fleet by @mdboom for commit 4fd9ea6 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-bot · 2024-07-26T14:34:39Z

🤖 New build scheduled with the buildbot fleet by @mdboom for commit ef89747 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

bedevere-app · 2024-07-29T17:47:27Z

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

ericsnowcurrently

Thanks for moving that.

There's one small thing left to do.

Lib/importlib/_bootstrap_external.py

bedevere-app · 2024-07-29T19:10:45Z

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

bedevere-bot · 2024-07-30T14:01:34Z

🤖 New build scheduled with the buildbot fleet by @mdboom for commit 398988d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-buildbots label again.

mdboom · 2024-07-30T14:33:38Z

I have made the requested changes; please review again

Note, I've added a test to make sure that the new C-side calculation of _pyc_magic_number_token matches the old _RAW_MAGIC_NUMBER, and run it over the buildbots to make sure it gets some testing on big-endian machines (since endianness matters here).

bedevere-app · 2024-07-30T14:33:44Z

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

ericsnowcurrently

LGTM

I've left a small number of minor suggestions related to formatting and tests. I'm approving the PR assuming you will address those (one way or another).

Include/internal/pycore_magic_number.h

Lib/test/test_import/__init__.py

Python/import.c

Co-authored-by: Eric Snow <[email protected]>

serhiy-storchaka · 2024-07-31T07:29:49Z

Lib/importlib/_bootstrap_external.py

-MAGIC_NUMBER = (3603).to_bytes(2, 'little') + b'\r\n'
-
-_RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little')  # For import.c
+MAGIC_NUMBER = (_imp.pyc_magic_number).to_bytes(2, 'little') + b'\r\n'


It can be defined as:

MAGIC_NUMBER = _imp.pyc_magic_number_token.to_bytes(4, 'little')

And _imp.pyc_magic_number will no longer be used, it can be removed. See #122503.

* pythongh-122188: Move magic number to its own file * Add versionadded directive * Do work in C * Integrate launcher.c * Make _pyc_magic_number private * Remove metadata * Move sys.implementation -> _imp * Modernize comment * Move _RAW_MAGIC_NUMBER to the C side as well * _pyc_magic_number -> pyc_magic_number * Remove unused import * Update docs * Apply suggestions from code review Co-authored-by: Eric Snow <[email protected]> * Fix typo in tests --------- Co-authored-by: Eric Snow <[email protected]>

pythongh-122188: Move magic number to its own file

dd6cd9e

mdboom requested review from brettcannon, ericsnowcurrently, ncoghlan and warsaw as code owners July 24, 2024 19:35

bedevere-app bot mentioned this pull request Jul 24, 2024

Move opcode magic number out of Lib/importlib/_bootstrap_external.py #122188

Closed

bedevere-app bot added the awaiting core review label Jul 24, 2024

Eclips4 reviewed Jul 24, 2024

View reviewed changes

Doc/library/sys.rst Outdated Show resolved Hide resolved

Add versionadded directive

5e69ccb

mdboom added the skip news label Jul 25, 2024

mdboom commented Jul 25, 2024

View reviewed changes

mdboom added 2 commits July 25, 2024 11:47

Do work in C

833f851

Integrate launcher.c

ef9b4a1

mdboom requested review from a team and kumaraditya303 as code owners July 25, 2024 16:46

brettcannon reviewed Jul 25, 2024

View reviewed changes

Make _pyc_magic_number private

2cc27f0

mdboom added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

mdboom added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

Merge remote-tracking branch 'upstream/main' into move-magic-number

f7d8a42

mdboom force-pushed the move-magic-number branch from 4fd9ea6 to f7d8a42 Compare July 26, 2024 13:48

mdboom added 2 commits July 26, 2024 09:49

Remove metadata

2294fb7

Merge remote-tracking branch 'upstream/main' into move-magic-number

ef89747

mdboom added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2024

bedevere-app bot added awaiting change review and removed awaiting changes labels Jul 29, 2024

bedevere-app bot requested a review from ericsnowcurrently July 29, 2024 17:47

ericsnowcurrently requested changes Jul 29, 2024

View reviewed changes

Lib/importlib/_bootstrap_external.py Outdated Show resolved Hide resolved

bedevere-app bot added awaiting changes and removed awaiting change review labels Jul 29, 2024

mdboom added 4 commits July 30, 2024 08:51

Move _RAW_MAGIC_NUMBER to the C side as well

33c9d36

_pyc_magic_number -> pyc_magic_number

3b79b94

Remove unused import

9bfce1d

Update docs

398988d

mdboom added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 30, 2024

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 30, 2024

bedevere-app bot added awaiting change review and removed awaiting changes labels Jul 30, 2024

bedevere-app bot requested a review from ericsnowcurrently July 30, 2024 14:33

ericsnowcurrently approved these changes Jul 30, 2024

View reviewed changes

Include/internal/pycore_magic_number.h Outdated Show resolved Hide resolved

Lib/test/test_import/__init__.py Outdated Show resolved Hide resolved

Python/import.c Outdated Show resolved Hide resolved

bedevere-app bot added awaiting merge and removed awaiting change review labels Jul 30, 2024

mdboom and others added 3 commits July 30, 2024 12:20

Apply suggestions from code review

d6c54ee

Co-authored-by: Eric Snow <[email protected]>

Fix typo in tests

e05847e

Merge remote-tracking branch 'upstream/main' into move-magic-number

cf01f9d

mdboom merged commit af0a00f into python:main Jul 30, 2024
35 checks passed

bedevere-app bot removed the awaiting merge label Jul 30, 2024

serhiy-storchaka reviewed Jul 31, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

gh-122188: Move magic number to its own file #122243

gh-122188: Move magic number to its own file #122243

mdboom commented Jul 24, 2024 •

edited by github-actions bot

Loading

mdboom Jul 25, 2024

brettcannon Jul 25, 2024

zware Jul 25, 2024

mdboom Jul 26, 2024

bedevere-bot commented Jul 26, 2024

bedevere-bot commented Jul 26, 2024

bedevere-bot commented Jul 26, 2024

bedevere-app bot commented Jul 29, 2024

ericsnowcurrently left a comment •

edited

Loading

bedevere-app bot commented Jul 29, 2024

bedevere-bot commented Jul 30, 2024

mdboom commented Jul 30, 2024

bedevere-app bot commented Jul 30, 2024

ericsnowcurrently left a comment

serhiy-storchaka Jul 31, 2024

gh-122188: Move magic number to its own file #122243

gh-122188: Move magic number to its own file #122243

Conversation

mdboom commented Jul 24, 2024 • edited by github-actions bot Loading

mdboom Jul 25, 2024

Choose a reason for hiding this comment

brettcannon Jul 25, 2024

Choose a reason for hiding this comment

zware Jul 25, 2024

Choose a reason for hiding this comment

mdboom Jul 26, 2024

Choose a reason for hiding this comment

bedevere-bot commented Jul 26, 2024

bedevere-bot commented Jul 26, 2024

bedevere-bot commented Jul 26, 2024

bedevere-app bot commented Jul 29, 2024

ericsnowcurrently left a comment • edited Loading

Choose a reason for hiding this comment

bedevere-app bot commented Jul 29, 2024

bedevere-bot commented Jul 30, 2024

mdboom commented Jul 30, 2024

bedevere-app bot commented Jul 30, 2024

ericsnowcurrently left a comment

Choose a reason for hiding this comment

serhiy-storchaka Jul 31, 2024

Choose a reason for hiding this comment

mdboom commented Jul 24, 2024 •

edited by github-actions bot

Loading

ericsnowcurrently left a comment •

edited

Loading