Issue #6074: Apply an appropriate fix for importlib based imports

2 files changed, 110 insertions, 49 deletions

diff --git a/Lib/importlib/_bootstrap.py b/Lib/importlib/_bootstrap.py
index 8a20c5e..26d9250 100644
--- a/Lib/importlib/_bootstrap.py
+++ b/Lib/importlib/_bootstrap.py
@@ -1048,6 +1048,9 @@ class SourceFileLoader(FileLoader, SourceLoader):
             mode = _os.stat(source_path).st_mode
         except OSError:
             mode = 0o666
+        # We always ensure write access so we can update cached files
+        # later even when the source files are read-only on Windows (#6074)
+        mode |= 0o200
         return self.set_data(bytecode_path, data, _mode=mode)
 
     def set_data(self, path, data, *, _mode=0o666):
diff --git a/Lib/test/test_import.py b/Lib/test/test_import.py
index 0746411..8cf96d8 100644
--- a/Lib/test/test_import.py
+++ b/Lib/test/test_import.py
@@ -14,6 +14,8 @@ import unittest
 import textwrap
 import errno
 import shutil
+import contextlib
+import time
 
 import test.support
 from test.support import (
@@ -33,6 +35,24 @@ def remove_files(name):
     rmtree('__pycache__')
 
 
+@contextlib.contextmanager
+def _ready_to_import(name=None, source=""):
+    # sets up a temporary directory and removes it
+    # creates the module file
+    # temporarily clears the module from sys.modules (if any)
+    name = name or "spam"
+    with script_helper.temp_dir() as tempdir:
+        path = script_helper.make_script(tempdir, name, source)
+        old_module = sys.modules.pop(name, None)
+        try:
+            sys.path.insert(0, tempdir)
+            yield name, path
+            sys.path.remove(tempdir)
+        finally:
+            if old_module is not None:
+                sys.modules[name] = old_module
+
+
 class ImportTests(unittest.TestCase):
 
     def setUp(self):
@@ -101,54 +121,6 @@ class ImportTests(unittest.TestCase):
         finally:
             del sys.path[0]
 
-    @unittest.skipUnless(os.name == 'posix',
-                         "test meaningful only on posix systems")
-    def test_creation_mode(self):
-        mask = 0o022
-        with temp_umask(mask):
-            sys.path.insert(0, os.curdir)
-            try:
-                fname = TESTFN + os.extsep + "py"
-                create_empty_file(fname)
-                fn = imp.cache_from_source(fname)
-                unlink(fn)
-                importlib.invalidate_caches()
-                __import__(TESTFN)
-                if not os.path.exists(fn):
-                    self.fail("__import__ did not result in creation of "
-                              "either a .pyc or .pyo file")
-                s = os.stat(fn)
-                # Check that the umask is respected, and the executable bits
-                # aren't set.
-                self.assertEqual(oct(stat.S_IMODE(s.st_mode)), oct(0o666 & ~mask))
-            finally:
-                del sys.path[0]
-                remove_files(TESTFN)
-                unload(TESTFN)
-
-    @unittest.skipUnless(os.name == 'posix',
-                         "test meaningful only on posix systems")
-    def test_cached_mode_issue_2051(self):
-        mode = 0o600
-        source = TESTFN + ".py"
-        with script_helper.temp_dir() as tempdir:
-            path = script_helper.make_script(tempdir, TESTFN,
-                                             "key='top secret'")
-            os.chmod(path, mode)
-            compiled = imp.cache_from_source(path)
-            sys.path.insert(0, tempdir)
-            try:
-                __import__(TESTFN)
-            finally:
-                sys.path.remove(tempdir)
-
-            if not os.path.exists(compiled):
-                self.fail("__import__ did not result in creation of "
-                          "either a .pyc or .pyo file")
-            stat_info = os.stat(compiled)
-
-        self.assertEqual(oct(stat.S_IMODE(stat_info.st_mode)), oct(mode))
-
     def test_bug7732(self):
         source = TESTFN + '.py'
         os.mkdir(source)
@@ -345,6 +317,92 @@ class ImportTests(unittest.TestCase):
             self.fail("fromlist must allow bogus names")
 
 
+class FilePermissionTests(unittest.TestCase):
+    # tests for file mode on cached .pyc/.pyo files
+
+    @unittest.skipUnless(os.name == 'posix',
+                         "test meaningful only on posix systems")
+    def test_creation_mode(self):
+        mask = 0o022
+        with temp_umask(mask), _ready_to_import() as (name, path):
+            cached_path = imp.cache_from_source(path)
+            module = __import__(name)
+            if not os.path.exists(cached_path):
+                self.fail("__import__ did not result in creation of "
+                          "either a .pyc or .pyo file")
+            stat_info = os.stat(cached_path)
+
+        # Check that the umask is respected, and the executable bits
+        # aren't set.
+        self.assertEqual(oct(stat.S_IMODE(stat_info.st_mode)),
+                         oct(0o666 & ~mask))
+
+    @unittest.skipUnless(os.name == 'posix',
+                         "test meaningful only on posix systems")
+    def test_cached_mode_issue_2051(self):
+        # permissions of .pyc should match those of .py, regardless of mask
+        mode = 0o600
+        with temp_umask(0o022), _ready_to_import() as (name, path):
+            cached_path = imp.cache_from_source(path)
+            os.chmod(path, mode)
+            __import__(name)
+            if not os.path.exists(cached_path):
+                self.fail("__import__ did not result in creation of "
+                          "either a .pyc or .pyo file")
+            stat_info = os.stat(cached_path)
+
+        self.assertEqual(oct(stat.S_IMODE(stat_info.st_mode)), oct(mode))
+
+    @unittest.skipUnless(os.name == 'posix',
+                         "test meaningful only on posix systems")
+    def test_cached_readonly(self):
+        mode = 0o400
+        with temp_umask(0o022), _ready_to_import() as (name, path):
+            cached_path = imp.cache_from_source(path)
+            os.chmod(path, mode)
+            __import__(name)
+            if not os.path.exists(cached_path):
+                self.fail("__import__ did not result in creation of "
+                          "either a .pyc or .pyo file")
+            stat_info = os.stat(cached_path)
+
+        expected = mode | 0o200 # Account for fix for issue #6074
+        self.assertEqual(oct(stat.S_IMODE(stat_info.st_mode)), oct(expected))
+
+    def test_pyc_always_writable(self):
+        # Initially read-only .pyc files on Windows used to cause problems
+        # with later updates, see issue #6074 for details
+        with _ready_to_import() as (name, path):
+            # Write a Python file, make it read-only and import it
+            with open(path, 'w') as f:
+                f.write("x = 'original'\n")
+            # Tweak the mtime of the source to ensure pyc gets updated later
+            s = os.stat(path)
+            os.utime(path, (s.st_atime, s.st_mtime-100000000))
+            os.chmod(path, 0o400)
+            m = __import__(name)
+            self.assertEqual(m.x, 'original')
+            # Change the file and then reimport it
+            os.chmod(path, 0o600)
+            with open(path, 'w') as f:
+                f.write("x = 'rewritten'\n")
+            unload(name)
+            importlib.invalidate_caches()
+            m = __import__(name)
+            self.assertEqual(m.x, 'rewritten')
+            # Now delete the source file and check the pyc was rewritten
+            unlink(path)
+            unload(name)
+            importlib.invalidate_caches()
+            if __debug__:
+                bytecode_only = path + "c"
+            else:
+                bytecode_only = path + "o"
+            os.rename(imp.cache_from_source(path), bytecode_only)
+            m = __import__(name)
+            self.assertEqual(m.x, 'rewritten')
+
+
 class PycRewritingTests(unittest.TestCase):
     # Test that the `co_filename` attribute on code objects always points
     # to the right file, even when various things happen (e.g. both the .py
@@ -945,7 +1003,7 @@ class ImportTracebackTests(unittest.TestCase):
 
 
 def test_main(verbose=None):
-    run_unittest(ImportTests, PycacheTests,
+    run_unittest(ImportTests, PycacheTests, FilePermissionTests,
                  PycRewritingTests, PathsTests, RelativeImportTests,
                  OverridingImportBuiltinTests,
                  ImportlibBootstrapTests,