bpo-29514: Check magic number for bugfix release #54

appeltel · 2017-02-12T19:10:17Z

Add a test to check the current MAGIC_NUMBER against the
expected number for the release if the current release is
at candidate or final level. On test failure, describe to
the developer the procedure for changing the magic number.

https://bugs.python.org/issue29514

Add a dict importlib.util.EXPECTED_MAGIC_NUMBERS which details the initial and expected pyc magic number for each minor release. This gives a mechanism for users to check if the magic number has changed within a release and for a test to ensure procedure is followed if a change is necessary. Add a test to check the current MAGIC_NUMBER against the expected number for the release if the current release is at candidate or final level. On test failure, describe to the developer the procedure for changing the magic number.

ncoghlan

The general approach in the test case looks like it will give affected maintainers useful guidance, but the impact can be reduced by moving the EXPECTED_MAGIC_NUMBERS table into the test case itself (the test case will complain when it needs updating, so it's OK to have it separated from the list of actual magic numbers in the importlib code)

ncoghlan · 2017-02-12T20:42:28Z

Lib/importlib/_bootstrap_external.py

+    (2, 7): 62211,
+    (3, 5): 3350,
+    (3, 6): 3379
+}


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.

Simplifed to a sigle entry in the test case in 9ea7c52

ncoghlan · 2017-02-12T20:42:55Z

Lib/importlib/util.py

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


With the table moved to the test suite, this won't be needed any more.

removed in 9ea7c52

ncoghlan · 2017-02-12T20:44:20Z

Lib/test/test_importlib/test_release.py

+
+        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


dev-python -> python-dev

fixed in 9ea7c52

ambv · 2017-02-13T04:46:03Z

Lib/importlib/_bootstrap_external.py

+
+EXPECTED_MAGIC_NUMBERS = {
+    (2, 7): 62211,
+    (3, 5): 3350,


Does this mean we're reverting the magic number bump from 3.5.3?

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.

Simplify the magic number release test by removing EXPECTED_MAGIC_NUMBERS table and making the expected magic number self-contained within the test. BPO: 29514

ncoghlan

This is looking pretty close now, just a couple of suggestions around details of the test execution.

ncoghlan · 2017-03-12T10:14:16Z

Lib/test/test_importlib/test_util.py

@@ -757,5 +757,49 @@ def test_source_from_cache_path_like_arg(self):
 ) = util.test_both(PEP3147Tests, util=importlib_util)


+class MagicNumberTests:


Inherit from unittest.TestCase here

I initially avoided this and followed the test machinery as I wasn't sure about directly importing importlib.util within this test module, but after looking at the way the test specialization works I don't see how this is an issue. Changed in 1e32a1b.

Yeah, this module does some interesting things to create "real" test classes in different configurations, but we don't need those kinds of tricks for the new test.

ncoghlan · 2017-03-12T10:16:29Z

Lib/test/test_importlib/test_util.py

+        adjustment to this test case.
+        """
+        if sys.version_info.releaselevel not in ('final', 'candidate'):
+            return


This can change to a unittest.skipUnless decorator on the testmethod:

@unittest.skipUnless(sys.version_info.release in ('final', 'release'), 'Only check magic number stability for release candidates and later')

Done in 1e32a1b

ncoghlan · 2017-03-12T10:19:07Z

Lib/test/test_importlib/test_util.py

+        actual = int.from_bytes(self.util.MAGIC_NUMBER[:2], 'little')
+
+        msg = (
+            "Candidate and final releases require the current  "


I'd start this message with "To avoid breaking backwards compatibility with cached bytecode files that can't be automatically regenerated by the current user, ..."

Added in 1e32a1b

Improve the execution of the magic number test by using skipUnless for alpha and beta releases, and directly inheriting from unittest.TestCase rather than using the machinery for the other tests. Also improve the error message to explain the reason for caution in changing the magic number. BPO: 29514

codecov · 2017-03-12T14:08:50Z

Codecov Report

Merging #54 into master will decrease coverage by <.01%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master      #54      +/-   ##
==========================================
- Coverage   82.37%   82.37%   -0.01%     
==========================================
  Files        1427     1428       +1     
  Lines      350948   350968      +20     
==========================================
+ Hits       289093   289102       +9     
- Misses      61855    61866      +11

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c22bfaa...1e32a1b. Read the comment docs.

serhiy-storchaka · 2017-04-02T13:31:32Z

Lib/test/test_importlib/test_util.py

+        actual = int.from_bytes(importlib.util.MAGIC_NUMBER[:2], 'little')
+
+        msg = (
+            "To avoid breaking backwards compatibility with cached bytecode "


Wouldn't be better to use a multiline string literal if you want such verbose error message?

But I think that "broken *.pyc files compatibility" would be enough.

Then you'd have to dedent it, etc. This message is perfectly legible to me.

serhiy-storchaka · 2017-04-02T13:33:31Z

Lib/test/test_importlib/test_util.py

+    )
+    def test_magic_number(self):
+        """
+        Each python minor release should generally have a MAGIC_NUMBER


I think it would be better to write this as a comment rather than a docstring. If you want, add a one-line docstring, but most test methods don't have docstrings.

miss-islington · 2017-09-07T00:12:18Z

🐍🍒⛏🤖 Thanks @appeltel for the PR, and @ambv for merging it 🌮🎉.I'm working now to backport this PR to: 2.7, 3.6.

miss-islington · 2017-09-07T00:12:19Z

Sorry @appeltel and @ambv, I had trouble checking out the 3.6 backport branch.
Please backport using cherry_picker on command line.

miss-islington · 2017-09-07T00:12:22Z

Sorry, @appeltel and @ambv, I could not cleanly backport this to 2.7 due to a conflict.
Please backport using cherry_picker on command line.

ncoghlan · 2017-09-07T00:36:17Z

This was backported a while back, but I missed updating the labels:

2.7:#2160
3.5: #2158
3.6: #2157

The versions checked here are 20 years old. Also dllwrap has started to emit a deprecation warning in the latest release spamming the build logs. Fixes python#54

the-knights-who-say-ni added the CLA signed label Feb 12, 2017

ncoghlan requested changes Feb 12, 2017

View reviewed changes

ambv reviewed Feb 13, 2017

View reviewed changes

Simplify magic number release test

9ea7c52

Simplify the magic number release test by removing EXPECTED_MAGIC_NUMBERS table and making the expected magic number self-contained within the test. BPO: 29514

ncoghlan requested changes Mar 12, 2017

View reviewed changes

ncoghlan approved these changes Mar 13, 2017

View reviewed changes

ncoghlan added needs backport to 2.7 labels Mar 13, 2017

serhiy-storchaka reviewed Apr 2, 2017

View reviewed changes

ambv merged commit d6d344d into python:master Apr 17, 2017

Mariatta added needs backport to 3.6 and removed needs backport to 3.6 labels Sep 7, 2017

ncoghlan removed needs backport to 2.7 labels Sep 7, 2017

mgmacias95 pushed a commit to mgmacias95/cpython that referenced this pull request Mar 27, 2023

Merge pull request python#54 from sunmy2019/handle-invalid-expressions

30ea09f

oraluben pushed a commit to oraluben/cpython that referenced this pull request Jul 12, 2023

feat(ci): cache llvm installs across CI (python#54)

454b349

gvanrossum mentioned this pull request Aug 22, 2023

heap-use-after-free in _PyFunction_LookupByVersion #108253

Closed

LinanV mentioned this pull request Oct 23, 2023

Objects/typeobject.c: No such file or directory. #111203

Closed

kcatss mentioned this pull request Feb 20, 2024

Use After Free at _heapreplace_max #115706

Closed

ngoldbaum mentioned this pull request Sep 23, 2024

Crash running PyO3 tests with --test-threads=1000 #124375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-29514: Check magic number for bugfix release #54

bpo-29514: Check magic number for bugfix release #54

appeltel commented Feb 12, 2017 •

edited by ncoghlan

Loading

ncoghlan left a comment

ncoghlan Feb 12, 2017

appeltel Mar 11, 2017

ncoghlan Feb 12, 2017

appeltel Mar 11, 2017

ncoghlan Feb 12, 2017

appeltel Mar 11, 2017

ambv Feb 13, 2017

ncoghlan Feb 13, 2017

ncoghlan left a comment

ncoghlan Mar 12, 2017

appeltel Mar 12, 2017

ncoghlan Mar 13, 2017

ncoghlan Mar 12, 2017

appeltel Mar 12, 2017

ncoghlan Mar 12, 2017

appeltel Mar 12, 2017

codecov bot commented Mar 12, 2017

serhiy-storchaka Apr 2, 2017

ambv Apr 17, 2017

serhiy-storchaka Apr 2, 2017

miss-islington commented Sep 7, 2017

miss-islington commented Sep 7, 2017

miss-islington commented Sep 7, 2017

ncoghlan commented Sep 7, 2017

		@@ -757,5 +757,49 @@ def test_source_from_cache_path_like_arg(self):
		) = util.test_both(PEP3147Tests, util=importlib_util)


		class MagicNumberTests:

bpo-29514: Check magic number for bugfix release #54

bpo-29514: Check magic number for bugfix release #54

Conversation

appeltel commented Feb 12, 2017 • edited by ncoghlan Loading

ncoghlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ncoghlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Mar 12, 2017

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miss-islington commented Sep 7, 2017

miss-islington commented Sep 7, 2017

miss-islington commented Sep 7, 2017

ncoghlan commented Sep 7, 2017

appeltel commented Feb 12, 2017 •

edited by ncoghlan

Loading