-
-
Notifications
You must be signed in to change notification settings - Fork 32.1k
bpo-29514: Check magic number for bugfix release #54
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -248,6 +248,21 @@ def _write_atomic(path, data, mode=0o666): | |
# | ||
# Whenever MAGIC_NUMBER is changed, the ranges in the magic_values array | ||
# in PC/launcher.c must also be updated. | ||
# | ||
# In general, the MAGIC_NUMBER should not change for maintenance releases | ||
# although this may be required under exceptional circumstances. When | ||
# each release reaches candidate status, an entry in EXPECTED_MAGIC_NUMBERS | ||
# should be added for this release. This value is tested against the actual | ||
# MAGIC_NUMBER to ensure that if a change is required within a minor | ||
# release, the exception is first discussed with python-dev and relevant | ||
# community stakeholders such as OS distribution package maintainers | ||
# are properly informed of the change. | ||
|
||
EXPECTED_MAGIC_NUMBERS = { | ||
(2, 7): 62211, | ||
(3, 5): 3350, | ||
(3, 6): 3379 | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'd put this table in the test suite rather than in the code - there's no need to have it other than the bytecode stability test case, and if it hasn't been updated when it needs to be we'll get a test failure anyway. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Simplifed to a sigle entry in the test case in 9ea7c52 |
||
|
||
MAGIC_NUMBER = (3390).to_bytes(2, 'little') + b'\r\n' | ||
_RAW_MAGIC_NUMBER = int.from_bytes(MAGIC_NUMBER, 'little') # For import.c | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
from ._bootstrap import _resolve_name | ||
from ._bootstrap import spec_from_loader | ||
from ._bootstrap import _find_spec | ||
from ._bootstrap_external import EXPECTED_MAGIC_NUMBERS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the table moved to the test suite, this won't be needed any more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed in 9ea7c52 |
||
from ._bootstrap_external import MAGIC_NUMBER | ||
from ._bootstrap_external import cache_from_source | ||
from ._bootstrap_external import decode_source | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
import importlib.util as util | ||
import sys | ||
import unittest | ||
|
||
|
||
class ImportModuleReleaseTests(unittest.TestCase): | ||
""" | ||
Test release compatibility issues relating to importlib | ||
""" | ||
def test_magic_number(self): | ||
""" | ||
Each python minor release should generally have a MAGIC_NUMBER | ||
that does not change once the release reaches candidate status. | ||
|
||
Once a release reaches candidate status, an entry should be | ||
added to EXPECTED_MAGIC_NUMBERS. This test will then check that | ||
the actual MAGIC_NUMBER matches the expected value for the | ||
release. | ||
|
||
In exceptional cases, it may be required to change the MAGIC_NUMBER | ||
for a maintenance release. In this case the change should be | ||
discussed in dev-python. If a change is required, community | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. dev-python -> python-dev There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed in 9ea7c52 |
||
stakeholders such as OS package maintainers must be notified | ||
in advance. Such exceptional releases will then require an | ||
adjustment to this test case. | ||
""" | ||
if sys.version_info.releaselevel not in ('final', 'candidate'): | ||
return | ||
|
||
release = (sys.version_info.major, sys.version_info.minor) | ||
|
||
msg = ( | ||
"Candidate and final releases require an entry in " | ||
"importlib.util.EXPECTED_MAGIC_NUMBERS. Set the expected " | ||
"magic number to the current MAGIC_NUMBER to continue " | ||
"with the release." | ||
) | ||
self.assertIn(release, util.EXPECTED_MAGIC_NUMBERS, msg=msg) | ||
expected = util.EXPECTED_MAGIC_NUMBERS[release] | ||
actual = int.from_bytes(util.MAGIC_NUMBER[:2], 'little') | ||
|
||
# Adjust for exceptional releases | ||
if release == (3, 5): | ||
expected = 3351 # Changed in 3.5.3 issue 27286 | ||
|
||
msg = ( | ||
"The MAGIC_NUMBER {0} does not match the expected value " | ||
"{1} for release {2}. Changing the MAGIC_NUMBER for a " | ||
"maintenance release requires discussion in dev-python and " | ||
"notification of community stakeholders." | ||
.format(actual, expected, release) | ||
) | ||
self.assertEqual(expected, actual, msg) | ||
|
||
|
||
if __name__ == '__main__': | ||
unittest.main() |
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.
Does this mean we're reverting the magic number bump from 3.5.3?
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.
No, but the current special case from the test should just be moved directly into the table of expected magic numbers with a suitable comment.