From 5716119a44fe274385e329dadfea4a2470928500 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 27 Feb 2020 00:40:47 -0800 Subject: [PATCH 1/5] bpo-39769: Fix compileall ddir for subpkgs. Fixes compileall.compile_dir's ddir parameter and compileall command line flag `-d` to no longer write the wrong pathname to the generated pyc file for submodules beneath the root of the directory tree being compiled. This fixes a regression introduced with Python 3.5. --- Doc/library/compileall.rst | 4 +-- Lib/compileall.py | 11 +++++-- Lib/test/test_compileall.py | 30 +++++++++++++++++++ .../2020-02-27-00-40-21.bpo-39769.hJmxu4.rst | 4 +++ 4 files changed, 45 insertions(+), 4 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2020-02-27-00-40-21.bpo-39769.hJmxu4.rst diff --git a/Doc/library/compileall.rst b/Doc/library/compileall.rst index 394d60634f1e0d..b1ae9d60e8ae14 100644 --- a/Doc/library/compileall.rst +++ b/Doc/library/compileall.rst @@ -143,7 +143,7 @@ runtime. Public functions ---------------- -.. function:: compile_dir(dir, maxlevels=sys.getrecursionlimit(), ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, workers=1, invalidation_mode=None, stripdir=None, prependdir=None, limit_sl_dest=None) +.. function:: compile_dir(dir, maxlevels=sys.getrecursionlimit(), ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, workers=1, invalidation_mode=None, \*, stripdir=None, prependdir=None, limit_sl_dest=None) Recursively descend the directory tree named by *dir*, compiling all :file:`.py` files along the way. Return a true value if all the files compiled successfully, @@ -221,7 +221,7 @@ Public functions .. versionchanged:: 3.9 Added *stripdir*, *prependdir* and *limit_sl_dest* arguments. -.. function:: compile_file(fullname, ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, invalidation_mode=None) +.. function:: compile_file(fullname, ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, invalidation_mode=None, \*, stripdir=None, prependdir=None, limit_sl_dest=None) Compile the file with path *fullname*. Return a true value if the file compiled successfully, and a false value otherwise. diff --git a/Lib/compileall.py b/Lib/compileall.py index 8cfde5b7b3e0aa..8eb41226f6ddec 100644 --- a/Lib/compileall.py +++ b/Lib/compileall.py @@ -46,7 +46,7 @@ def _walk_dir(dir, maxlevels, quiet=0): def compile_dir(dir, maxlevels=None, ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, workers=1, - invalidation_mode=None, stripdir=None, + invalidation_mode=None, *, stripdir=None, prependdir=None, limit_sl_dest=None): """Byte-compile all modules in the given directory tree. @@ -72,6 +72,13 @@ def compile_dir(dir, maxlevels=None, ddir=None, force=False, the defined path """ ProcessPoolExecutor = None + if ddir is not None and (stripdir is not None or prependdir is not None): + raise ValueError(("Destination dir (ddir) cannot be used " + "in combination with stripdir or prependdir")) + if ddir: + stripdir = dir + prependdir = ddir + ddir = None if workers < 0: raise ValueError('workers must be greater or equal to 0') if workers != 1: @@ -111,7 +118,7 @@ def compile_dir(dir, maxlevels=None, ddir=None, force=False, def compile_file(fullname, ddir=None, force=False, rx=None, quiet=0, legacy=False, optimize=-1, - invalidation_mode=None, stripdir=None, prependdir=None, + invalidation_mode=None, *, stripdir=None, prependdir=None, limit_sl_dest=None): """Byte-compile one file. diff --git a/Lib/test/test_compileall.py b/Lib/test/test_compileall.py index 2ebcb424095d8b..a1088158761b69 100644 --- a/Lib/test/test_compileall.py +++ b/Lib/test/test_compileall.py @@ -2,6 +2,7 @@ import compileall import importlib.util import test.test_importlib.util +import marshal import os import pathlib import py_compile @@ -212,6 +213,35 @@ def test_compile_dir_maxlevels(self): compileall.compile_dir(self.directory, quiet=True, maxlevels=depth) self.assertTrue(os.path.isfile(pyc_filename)) + def test_ddir_only(self): + """Recursive compile_dir ddir must contain package paths; bpo39769.""" + fullpath = ["test", "foo"] + path = self.directory + mods = [] + for subdir in fullpath: + path = os.path.join(path, subdir) + os.mkdir(path) + script_helper.make_script(path, "__init__", "") + mods.append(script_helper.make_script(path, "mod", + "def fn(): 1/0\nfn()\n")) + ddir = "" + compileall.compile_dir(self.directory, quiet=True, ddir=ddir) + assert mods + for mod in mods: + assert mod.startswith(self.directory) + modcode = importlib.util.cache_from_source(mod) + modpath = mod[len(self.directory+os.sep):] + _, _, err = script_helper.assert_python_failure(modcode) + expected_in = os.path.join(ddir, modpath) + with open(modcode, 'rb') as mod_pyc_f: + mod_pyc_f.seek(16) # Update if pyc header size ever changes. + mod_code_obj = marshal.load(mod_pyc_f) + self.assertEqual(mod_code_obj.co_filename, expected_in) + self.assertIn( + f'"{expected_in}"', + str(err, encoding=sys.getdefaultencoding()) + ) + def test_strip_only(self): fullpath = ["test", "build", "real", "path"] path = os.path.join(self.directory, *fullpath) diff --git a/Misc/NEWS.d/next/Library/2020-02-27-00-40-21.bpo-39769.hJmxu4.rst b/Misc/NEWS.d/next/Library/2020-02-27-00-40-21.bpo-39769.hJmxu4.rst new file mode 100644 index 00000000000000..9b564bd10d3b3b --- /dev/null +++ b/Misc/NEWS.d/next/Library/2020-02-27-00-40-21.bpo-39769.hJmxu4.rst @@ -0,0 +1,4 @@ +The :func:`compileall.compile_dir` function's *ddir* parameter and the +compileall command line flag `-d` no longer write the wrong pathname to the +generated pyc file for submodules beneath the root of the directory tree +being compiled. This fixes a regression introduced with Python 3.5. From 34e354b671534a7d93227f3fbdfc489a16aabc08 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 27 Feb 2020 13:45:11 -0800 Subject: [PATCH 2/5] Add a function to read code from a pyc to test_importlib.util. --- Lib/test/test_importlib/util.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/Lib/test/test_importlib/util.py b/Lib/test/test_importlib/util.py index 5aaf277e1f3725..de6e0b02560184 100644 --- a/Lib/test/test_importlib/util.py +++ b/Lib/test/test_importlib/util.py @@ -7,6 +7,7 @@ from importlib import machinery, util, invalidate_caches from importlib.abc import ResourceReader import io +import marshal import os import os.path from pathlib import Path, PurePath @@ -118,6 +119,16 @@ def submodule(parent, name, pkg_dir, content=''): return '{}.{}'.format(parent, name), path +def get_code_from_pyc(pyc_path): + """Reads a pyc file and returns the unmarshalled code object within. + + No header validation is performed. + """ + with open(pyc_path, 'rb') as pyc_f: + pyc_f.seek(16) + return marshal.load(pyc_f) + + @contextlib.contextmanager def uncache(*names): """Uncache a module from sys.modules. From 6b5d576243905e2cb3f9068e0c45d1fea4fc4760 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 27 Feb 2020 13:45:56 -0800 Subject: [PATCH 3/5] Test both single and parallel workers. --- Lib/test/test_compileall.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/Lib/test/test_compileall.py b/Lib/test/test_compileall.py index a1088158761b69..e758963613f024 100644 --- a/Lib/test/test_compileall.py +++ b/Lib/test/test_compileall.py @@ -2,7 +2,6 @@ import compileall import importlib.util import test.test_importlib.util -import marshal import os import pathlib import py_compile @@ -213,7 +212,7 @@ def test_compile_dir_maxlevels(self): compileall.compile_dir(self.directory, quiet=True, maxlevels=depth) self.assertTrue(os.path.isfile(pyc_filename)) - def test_ddir_only(self): + def _test_ddir_only(self, *, parallel=True): """Recursive compile_dir ddir must contain package paths; bpo39769.""" fullpath = ["test", "foo"] path = self.directory @@ -225,7 +224,9 @@ def test_ddir_only(self): mods.append(script_helper.make_script(path, "mod", "def fn(): 1/0\nfn()\n")) ddir = "" - compileall.compile_dir(self.directory, quiet=True, ddir=ddir) + compileall.compile_dir( + self.directory, quiet=True, ddir=ddir, + workers=2 if parallel else 1) assert mods for mod in mods: assert mod.startswith(self.directory) @@ -233,15 +234,21 @@ def test_ddir_only(self): modpath = mod[len(self.directory+os.sep):] _, _, err = script_helper.assert_python_failure(modcode) expected_in = os.path.join(ddir, modpath) - with open(modcode, 'rb') as mod_pyc_f: - mod_pyc_f.seek(16) # Update if pyc header size ever changes. - mod_code_obj = marshal.load(mod_pyc_f) + mod_code_obj = test.test_importlib.util.get_code_from_pyc(modcode) self.assertEqual(mod_code_obj.co_filename, expected_in) self.assertIn( f'"{expected_in}"', str(err, encoding=sys.getdefaultencoding()) ) + def test_ddir_only_one_worker(self): + """Recursive compile_dir ddir must contain package paths; bpo39769.""" + return self._test_ddir_only(parallel=False) + + def test_ddir_only_multiple_workers(self): + """Recursive compile_dir ddir must contain package paths; bpo39769.""" + return self._test_ddir_only(parallel=True) + def test_strip_only(self): fullpath = ["test", "build", "real", "path"] path = os.path.join(self.directory, *fullpath) From dd244d36eb71eaaebbd4612e74315d504fe1cac9 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 27 Feb 2020 21:46:18 -0800 Subject: [PATCH 4/5] Address code review comments. --- Lib/compileall.py | 2 +- Lib/test/test_compileall.py | 9 +++------ 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/Lib/compileall.py b/Lib/compileall.py index 8eb41226f6ddec..1831ad749f2b17 100644 --- a/Lib/compileall.py +++ b/Lib/compileall.py @@ -75,7 +75,7 @@ def compile_dir(dir, maxlevels=None, ddir=None, force=False, if ddir is not None and (stripdir is not None or prependdir is not None): raise ValueError(("Destination dir (ddir) cannot be used " "in combination with stripdir or prependdir")) - if ddir: + if ddir is not None: stripdir = dir prependdir = ddir ddir = None diff --git a/Lib/test/test_compileall.py b/Lib/test/test_compileall.py index e758963613f024..d9c9a6d97a55f5 100644 --- a/Lib/test/test_compileall.py +++ b/Lib/test/test_compileall.py @@ -227,19 +227,16 @@ def _test_ddir_only(self, *, parallel=True): compileall.compile_dir( self.directory, quiet=True, ddir=ddir, workers=2 if parallel else 1) - assert mods + self.assertTrue(mods) for mod in mods: - assert mod.startswith(self.directory) + self.assertTrue(mod.startswith(self.directory), mod) modcode = importlib.util.cache_from_source(mod) modpath = mod[len(self.directory+os.sep):] _, _, err = script_helper.assert_python_failure(modcode) expected_in = os.path.join(ddir, modpath) mod_code_obj = test.test_importlib.util.get_code_from_pyc(modcode) self.assertEqual(mod_code_obj.co_filename, expected_in) - self.assertIn( - f'"{expected_in}"', - str(err, encoding=sys.getdefaultencoding()) - ) + self.assertIn(f'"{expected_in}"', os.fsdecode(err)) def test_ddir_only_one_worker(self): """Recursive compile_dir ddir must contain package paths; bpo39769.""" From 4e3543fb2cdcc5f59cfd10eeef1735d3d8f617e6 Mon Sep 17 00:00:00 2001 From: "Gregory P. Smith" Date: Thu, 27 Feb 2020 22:54:46 -0800 Subject: [PATCH 5/5] explicitly test ddir='' --- Lib/test/test_compileall.py | 21 ++++++++++++++------- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/Lib/test/test_compileall.py b/Lib/test/test_compileall.py index d9c9a6d97a55f5..cb59fd71b38254 100644 --- a/Lib/test/test_compileall.py +++ b/Lib/test/test_compileall.py @@ -212,7 +212,7 @@ def test_compile_dir_maxlevels(self): compileall.compile_dir(self.directory, quiet=True, maxlevels=depth) self.assertTrue(os.path.isfile(pyc_filename)) - def _test_ddir_only(self, *, parallel=True): + def _test_ddir_only(self, *, ddir, parallel=True): """Recursive compile_dir ddir must contain package paths; bpo39769.""" fullpath = ["test", "foo"] path = self.directory @@ -223,7 +223,6 @@ def _test_ddir_only(self, *, parallel=True): script_helper.make_script(path, "__init__", "") mods.append(script_helper.make_script(path, "mod", "def fn(): 1/0\nfn()\n")) - ddir = "" compileall.compile_dir( self.directory, quiet=True, ddir=ddir, workers=2 if parallel else 1) @@ -239,12 +238,20 @@ def _test_ddir_only(self, *, parallel=True): self.assertIn(f'"{expected_in}"', os.fsdecode(err)) def test_ddir_only_one_worker(self): - """Recursive compile_dir ddir must contain package paths; bpo39769.""" - return self._test_ddir_only(parallel=False) + """Recursive compile_dir ddir= contains package paths; bpo39769.""" + return self._test_ddir_only(ddir="", parallel=False) - def test_ddir_only_multiple_workers(self): - """Recursive compile_dir ddir must contain package paths; bpo39769.""" - return self._test_ddir_only(parallel=True) + def test_ddir_multiple_workers(self): + """Recursive compile_dir ddir= contains package paths; bpo39769.""" + return self._test_ddir_only(ddir="", parallel=True) + + def test_ddir_empty_only_one_worker(self): + """Recursive compile_dir ddir='' contains package paths; bpo39769.""" + return self._test_ddir_only(ddir="", parallel=False) + + def test_ddir_empty_multiple_workers(self): + """Recursive compile_dir ddir='' contains package paths; bpo39769.""" + return self._test_ddir_only(ddir="", parallel=True) def test_strip_only(self): fullpath = ["test", "build", "real", "path"]