bpo-31650: PEP 552 (Deterministic pycs) implementation by benjaminp · Pull Request #4575 · python/cpython

benjaminp · 2017-11-26T22:39:17Z

Python now supports checking bytecode cache up-to-dateness with a hash of the
source contents rather than volatile source metadata. See the PEP for details.

While a fairly straightforward idea, quite a lot of code had to be modified due
to the pervasiveness of pyc implementation details in the codebase. Changes in
this commit include:

The core changes to importlib to understand how to read, validate, and
regenerate hash-based pycs.
Support for generating hash-based pycs in py_compile and compileall.
Modifications to our siphash implementation to support passing a custom
key. We then expose it to importlib through _imp.
Updates to all places in the interpreter, standard library, and tests that
manually generate or parse pyc files to grok the new format.
Support in the interpreter command line code for long options like
--check-hash-based-pycs.
Tests and documentation for all of the above.

https://bugs.python.org/issue31650

gpshead · 2017-11-28T01:20:45Z

Doc/library/py_compile.rst

+      against the source at runtime to determine if the pyc needs to be
+      regenerated.
+
+   .. attribute:: UNCHECKED_HASH


The text below merely says "a hash is included but not validated" but makes no mention of what is validated in this situation. the pyc-invalidation docs cover this in more detail. It seems worth repeating that this just means a hash is generated when generating a pyc but is not checked by default here.

It'd also be good to include a statement of when this might be useful (packaging distribution installed pre-generated pyc files where something else is guaranteeing they cannot go out of sync for example?). Though perhaps that description belongs in import.rst's pyc-invalidation section.

The text below merely says "a hash is included but not validated" but makes no mention of what is validated in this situation. the pyc-invalidation docs cover this in more detail. It seems worth repeating that this just means a hash is generated when generating a pyc but is not checked by default here.

Yeah, I was trying to funnel everything into import.rst. I will beef up this part a big, though.

It'd also be good to include a statement of when this might be useful (packaging distribution installed pre-generated pyc files where something else is guaranteeing they cannot go out of sync for example?). Though perhaps that description belongs in import.rst's pyc-invalidation section.

I put some commentary about why you might want this in the what's new? entry. Maybe, it should go somewhere more "permanent", too, though.

gpshead · 2017-11-28T01:29:52Z

Lib/compileall.py

    if args.workers is not None:
        args.workers = args.workers or None

+    ivl_mode = args.invalidation_mode.replace('-', '_').upper()


Since you use the names in choices= above to generate the mode name, perhaps use a similar reverse transformation to generate a (sorted/stable-ordered) list of names in the choices tuple above?

choices=sorted(mode.lower().replace('_', '-') for mode in args.invalidation_mode),

or similar?

gpshead · 2017-11-28T01:34:12Z

Lib/importlib/_bootstrap_external.py

-def _validate_bytecode_header(data, source_stats=None, name=None, path=None):
-    """Validate the header of the passed-in bytecode against source_stats (if
-    given) and returning the bytecode that can be compiled by compile().
+def _classify_pyc(data, name, exc_details):


Can you document what the arguments are?
name is not obvious - could it be given a better variable name? is it the pyc file's basename or the full pathname?
exc_details is unobvious.

A similar comment applies to all other new function def's below.

gpshead · 2017-11-28T01:38:25Z

Lib/importlib/_bootstrap_external.py

+    return flags
+
+
+def _validate_timestamp_pyc(data, source_stats, name, exc_details):


It seems suspicious that this raises no error when the source_stats dict-like object fails to contain mtime or size members.

If this is intentional, document why.

Yeah, I'll clean this up.

gpshead · 2017-11-28T01:42:03Z

Lib/importlib/_bootstrap_external.py

+
+def _code_to_timestamp_pyc(code, mtime=0, source_size=0):
+    "Produce the data for a timestamp-based pyc."
    data = bytearray(MAGIC_NUMBER)


It is unfortunate that the constant is called MAGIC_NUMBER when in fact it is a bytes object containing the binary magic data. Can it be renamed as this is "just" _bootstrap_external or would that break some API somewhere?

bytearray(MAGIC_NUMBER) looks suspicious if a reader naively assumes that MAGIC_NUMBER is an integer...

Yeah, it turns out the integer version is actually called _RAW_MAGIC_NUMBER, which a priori sounds more likely to be a bytearray. oops

At any rate, I think it's correct that this constant is exposed as some bytes not a number, since the only valid operation is to write it into a file or compare it to some bytes from a file.

gpshead · 2017-11-28T02:05:04Z

Lib/importlib/_bootstrap_external.py

-                                source_stats=st, name=fullname,
-                                path=bytecode_path)
+                        flags = _classify_pyc(data, fullname, exc_details)
+                        bytes_data = data[16:]


Consider using memoryview(data)[16:] to avoid a large copy assuming _compile_bytecode is happy with that (it appears to be, marshal.loads() accepts memoryview)

gpshead · 2017-11-28T02:08:45Z

Lib/importlib/_bootstrap_external.py

+            'path': path,
+        }
+        _classify_pyc(data, fullname, exc_details)
+        return _compile_bytecode(data[16:], name=fullname, bytecode_path=path)


memoryview(data)[16:] if you also do this in the other place i made this comment.

gpshead · 2017-11-28T02:09:30Z

Lib/importlib/util.py

@@ -5,18 +5,25 @@
 from ._bootstrap import spec_from_loader
 from ._bootstrap import _find_spec
 from ._bootstrap_external import MAGIC_NUMBER


This is a public API exposing MAGIC_NUMBER so it's probably best to ignore my previous comment about changing the name of the thing to avoid confusion over its type.

gpshead · 2017-11-28T02:10:01Z

Lib/modulefinder.py

                self.msgout(2, "raise ImportError: " + str(exc), pathname)
                raise
-            co = marshal.loads(marshal_data)
+            co = marshal.loads(data[16:])


memoryview(data)[16:]

gpshead · 2017-11-28T02:16:23Z

Lib/py_compile.py

+        bytecode = importlib._bootstrap_external._code_to_hash_pyc(
+            code,
+            source_hash,
+            invalidation_mode == PycInvalidationMode.CHECKED_HASH,


A == test to get a boolean value in a list of args is easy to misread as a typo for a keyword argument being passed. Disambiguate by adding ()s around it or passing this by name?

benjaminp

Thanks for the comments! Update incoming.

brettcannon

Pretty much all of my comments are minor, so no need to have to double-check any changes made based on my feedback.

brettcannon · 2017-12-01T20:27:51Z

Doc/library/compileall.rst

+.. cmdoption:: --invalidation-mode [timestamp|checked-hash|unchecked-hash]
+
+   Control how the generated pycs will be invalidated at runtime. The default
+   setting, ``timestamp``, means that pyc files with the source timestamp and


I just wanted to double-check that we are referring to the files as "pyc" and "pycs" throughout the rest of the docs instead of ".pyc files" or something? Basically just checking we're being consistent.

brettcannon · 2017-12-01T20:29:10Z

Doc/library/importlib.rst


+.. function:: source_hash(source_bytes)
+
+   Return the hash of *source_bytes* as byte string. A hash-based pyc embeds the


"as byte string" -> "as bytes".

brettcannon · 2017-12-01T20:30:58Z

Doc/reference/import.rst

+metadata.
+
+Python also supports "hash-based" cache files, which store a hash of a source
+file contents rather than its metadata. There are two variants of hash-based


"source file contents" -> "source file's contents"

brettcannon · 2017-12-01T20:31:47Z

Doc/reference/import.rst

+hash in the cache file. If a checked hash-based cache file is found to be
+invalid, Python regenerates it and writes a new checked hash-based cache
+file. For unchecked hash-based pycs, Python simply assumes the cache file is
+valid if it exists. Hash-based pyc validation behavior may be override with the


"override" -> "overridden"

brettcannon · 2017-12-01T20:36:31Z

Lib/importlib/_bootstrap_external.py

-    elif len(raw_timestamp) != 4:
-        message = 'reached EOF while reading timestamp in {!r}'.format(name)
+    if len(data) < 16:
+        message = 'reached EOF while reading pyc header of {!r}'.format(name)


Might as well change this to an f-string.

brettcannon · 2017-12-01T20:36:39Z

Lib/importlib/_bootstrap_external.py

+    flags = _r_long(data[4:8])
+    # Only the first two flags are defined.
+    if flags & ~0b11:
+        raise ImportError('invalid flags {!r} in {!r}'.format(flags, name),


brettcannon · 2017-12-01T20:36:47Z

Lib/importlib/_bootstrap_external.py

+
+    """
+    if _r_long(data[8:12]) != (source_mtime & 0xFFFFFFFF):
+        message = 'bytecode is stale for {!r}'.format(name)


brettcannon · 2017-12-01T20:37:03Z

Lib/importlib/_bootstrap_external.py

+        raise ImportError(message, **exc_details)
+    if (source_size is not None and
+        _r_long(data[12:16]) != (source_size & 0xFFFFFFFF)):
+        raise ImportError('bytecode is stale for {!r}'.format(name),


brettcannon · 2017-12-01T20:43:00Z

Lib/test/test_importlib/source/test_file_loader.py

        with util.create_modules('_temp') as mapping:
            bc_path = self.manipulate_bytecode('_temp', mapping,
-                                                lambda bc: bc[:12],
+                                                lambda bc: bc[:16],


I should have made these constants instead of using a literal when I originally wrote this code. Sorry about that. :(

brettcannon · 2017-12-01T20:45:01Z

Python/import.c

    if (d == NULL)
        goto failure;
+    PyObject *pyc_mode = PyUnicode_FromString(_Py_CheckHashBasedPycsMode);
+    if (pyc_mode == NULL)


Missing curly braces.

gpshead

some minor comments to address but overall i believe this is good to go in when you are ready.

thanks!

gpshead · 2017-12-09T01:59:13Z

Modules/main.c

 -x     : skip first line of source, allowing use of non-Unix forms of #!cmd\n\
 -X opt : set implementation-specific option\n\
+--check-hash-based-pycs always|default|never:\n\
+    control how Python invalidates hash-based .pcy files\n\


gpshead · 2017-12-09T02:00:12Z

Modules/perfmap.c

+};
+
+PyDoc_STRVAR(module_doc,
+"perfmap module.");


where'd this file come from?

eek, now you know what crud I have sitting about in my checkout.

gpshead · 2017-12-09T02:01:20Z

Modules/zipimport.c

+    if (flags != 0) {
+        // Hash-based pyc. We currently refuse to handle checked hash-based
+        // pycs. We could validate hash-based pycs against the source, but it
+        // seems likely that most people putting hash-based pycs in a zipfile


Python now supports checking bytecode cache up-to-dateness with a hash of the source contents rather than volatile source metadata. See the PEP for details. While a fairly straightforward idea, quite a lot of code had to be modified due to the pervasiveness of pyc implementation details in the codebase. Changes in this commit include: - The core changes to importlib to understand how to read, validate, and regenerate hash-based pycs. - Support for generating hash-based pycs in py_compile and compileall. - Modifications to our siphash implementation to support passing a custom key. We then expose it to importlib through _imp. - Updates to all places in the interpreter, standard library, and tests that manually generate or parse pyc files to grok the new format. - Support in the interpreter command line code for long options like --check-hash-based-pycs. - Tests and documentation for all of the above.

benjaminp · 2017-12-09T18:27:10Z

Thanks again for the reviews, Brett and Greg!

benjaminp requested a review from a team November 26, 2017 22:39

the-knights-who-say-ni added the CLA signed label Nov 26, 2017

bedevere-bot added the awaiting merge label Nov 26, 2017

benjaminp force-pushed the benjamin-pep-552 branch 4 times, most recently from 780bf48 to cd28db3 Compare November 27, 2017 07:31

benjaminp requested review from Yhg1s, brettcannon and gpshead November 27, 2017 07:55

gpshead reviewed Nov 28, 2017

View reviewed changes

benjaminp commented Nov 28, 2017

View reviewed changes

benjaminp force-pushed the benjamin-pep-552 branch from cd28db3 to 737f925 Compare November 28, 2017 05:47

gpshead assigned gpshead and brettcannon Nov 30, 2017

brettcannon reviewed Dec 1, 2017

View reviewed changes

benjaminp force-pushed the benjamin-pep-552 branch from 737f925 to b3a7070 Compare December 3, 2017 07:08

gpshead approved these changes Dec 9, 2017

View reviewed changes

benjaminp added 12 commits December 9, 2017 00:34

add parens for clarity

89fae57

use memoryview to avoid copying

3c1f73d

avoid hard-coding argument chocies

0d0a265

improve importlib docstring

0247475

massage PycInvalidationMode docs

dc10730

update importlib

29dcc24

pyc -> .pyc files

efdcf4b

fix grammar

ce3ab9e

fstring the works

09cbb9d

fix spelling

c026928

add --check-hash-based-pycs to usage output

56ccb80

benjaminp added 2 commits December 9, 2017 00:34

add curlies

7298380

fix pyc typo

f498a91

benjaminp force-pushed the benjamin-pep-552 branch from e5d54da to f498a91 Compare December 9, 2017 08:35

benjaminp merged commit 42aa93b into master Dec 9, 2017

bedevere-bot removed the awaiting merge label Dec 9, 2017

benjaminp deleted the benjamin-pep-552 branch December 9, 2017 18:26

tirkarthi mentioned this pull request Oct 15, 2018

WIP: bpo-34990: year 2038 problem in compileall.py #9892

Closed

ammaraskar mentioned this pull request Apr 22, 2020

bpo-34990: Change pyc headers to use 64-bit timestamps #19651

Closed

FFY00 mentioned this pull request Nov 21, 2022

Call to _imp.source_hash with incorrect arguments in (unreachable?) part of SourceLoader.get_code (importlib._bootstrap_external) #99508

Closed

		return flags


		def _validate_timestamp_pyc(data, source_stats, name, exc_details):


		.. function:: source_hash(source_bytes)

		Return the hash of source_bytes as byte string. A hash-based pyc embeds the

Uh oh!

Conversation

benjaminp commented Nov 26, 2017 • edited by bedevere-bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminp left a comment

Choose a reason for hiding this comment

Uh oh!

brettcannon left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gpshead left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjaminp commented Dec 9, 2017

Uh oh!

benjaminp commented Nov 26, 2017 •

edited by bedevere-bot

Loading