FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-29514: Check magic number for bugfix release by appeltel · Pull Request #54 · python/cpython · GitHub

Repository navigation

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

Merged
ambv merged 3 commits into
python:masterfrom
appeltel:bpo-29514-magic-numbers
Apr 17, 2017
Merged

ambv merged 3 commits into
python:masterfrom
appeltel:bpo-29514-magic-numbers

Conversation

appeltel commented Feb 12, 2017 •
edited by ncoghlan
Loading

Copy link
Copy Markdown
Contributor

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 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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)

Comment thread Lib/importlib/_bootstrap_external.py Outdated
(2, 7): 62211,
(3, 5): 3350,
(3, 6): 3379
}

ghost Feb 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ghost Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/importlib/util.py Outdated
from ._bootstrap import _resolve_name
from ._bootstrap import spec_from_loader
from ._bootstrap import _find_spec
from ._bootstrap_external import EXPECTED_MAGIC_NUMBERS

ghost Feb 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ghost Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

removed in 9ea7c52

Comment thread Lib/test/test_importlib/test_release.py Outdated

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

ghost Feb 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

dev-python -> python-dev

ghost Mar 11, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

fixed in 9ea7c52

Comment thread Lib/importlib/_bootstrap_external.py Outdated

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

ghost Feb 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ghost Feb 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ghost left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Comment thread Lib/test/test_importlib/test_util.py Outdated
) = util.test_both(PEP3147Tests, util=importlib_util)


class MagicNumberTests:

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Inherit from unittest.TestCase here

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ghost Mar 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

Comment thread Lib/test/test_importlib/test_util.py Outdated
adjustment to this test case.
"""
if sys.version_info.releaselevel not in ('final', 'candidate'):
return

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Done in 1e32a1b

Comment thread Lib/test/test_importlib/test_util.py Outdated
actual = int.from_bytes(self.util.MAGIC_NUMBER[:2], 'little')

msg = (
"Candidate and final releases require the current "

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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, ..."

ghost Mar 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

ghost commented Mar 12, 2017

Copy link
Copy Markdown

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.

actual = int.from_bytes(importlib.util.MAGIC_NUMBER[:2], 'little')

msg = (
"To avoid breaking backwards compatibility with cached bytecode "

ghost Apr 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

ghost Apr 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

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

ghost Apr 2, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

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

ghost commented Sep 7, 2017

Copy link
Copy Markdown
Contributor

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

ghost commented Sep 7, 2017

Copy link
Copy Markdown
Contributor

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

ghost commented Sep 7, 2017

Copy link
Copy Markdown
Contributor

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.

ghost commented Sep 7, 2017

Copy link
Copy Markdown
Contributor

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

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

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants


Back | FazBrowse Home | New Git URL