Add a tag to the cachedir

Since there are now two files to make when a cachedir is created,
use the temporary-dir -> rename technique.

CacheDir tests no longer pre-create the cache directory, they should
be verifying it's created properly upon request (one unit test still
makes sure passing an existing empty directory works, too).

Signed-off-by: Mats Wichmann <mats@linux.com>

2 files changed, 131 insertions, 73 deletions

diff --git a/SCons/CacheDir.py b/SCons/CacheDir.py
index 0174793..7f8deb5 100644
--- a/SCons/CacheDir.py
+++ b/SCons/CacheDir.py
@@ -29,6 +29,7 @@ import json
 import os
 import stat
 import sys
+import tempfile
 import uuid
 
 import SCons.Action
@@ -36,6 +37,12 @@ import SCons.Errors
 import SCons.Warnings
 import SCons.Util
 
+CACHE_PREFIX_LEN = 2  # first two characters used as subdirectory name
+CACHE_TAG = (
+    b"Signature: 8a477f597d28d172789f06886806bc55\n"
+    b"# SCons cache directory - see https://bford.info/cachedir/\n"
+)
+
 cache_enabled = True
 cache_debug = False
 cache_force = False
@@ -67,20 +74,20 @@ def CacheRetrieveFunc(target, source, env) -> int:
         fs.chmod(t.get_internal_path(), stat.S_IMODE(st[stat.ST_MODE]) | stat.S_IWRITE)
     return 0
 
-def CacheRetrieveString(target, source, env) -> None:
+def CacheRetrieveString(target, source, env) -> str:
     t = target[0]
     fs = t.fs
     cd = env.get_CacheDir()
     cachedir, cachefile = cd.cachepath(t)
     if t.fs.exists(cachefile):
         return "Retrieved `%s' from cache" % t.get_internal_path()
-    return None
+    return ""
 
 CacheRetrieve = SCons.Action.Action(CacheRetrieveFunc, CacheRetrieveString)
 
 CacheRetrieveSilent = SCons.Action.Action(CacheRetrieveFunc, None)
 
-def CachePushFunc(target, source, env):
+def CachePushFunc(target, source, env) -> None:
     if cache_readonly:
         return
 
@@ -134,8 +141,7 @@ CachePush = SCons.Action.Action(CachePushFunc, None)
 class CacheDir:
 
     def __init__(self, path) -> None:
-        """
-        Initialize a CacheDir object.
+        """Initialize a CacheDir object.
 
         The cache configuration is stored in the object. It
         is read from the config file in the supplied path if
@@ -147,53 +153,97 @@ class CacheDir:
         self.path = path
         self.current_cache_debug = None
         self.debugFP = None
-        self.config = dict()
-        if path is None:
-            return
-
-        self._readconfig(path)
+        self.config = {}
+        if path is not None:
+            self._readconfig(path)
+
+    def _add_config(self, path: str) -> None:
+        """Create the cache config file in *path*.
+
+        Locking isn't necessary in the normal case - when the cachedir is
+        being created - because it's written to a unique directory first,
+        before the directory is renamed. But it is legal to call CacheDir
+        with an existing directory, which may be missing the config file,
+        and in that case we do need locking. Simpler to always lock.
+        """
+        config_file = os.path.join(path, 'config')
+        # TODO: this breaks the "unserializable config object" test which
+        #   does some crazy stuff, so for now don't use setdefault. It does
+        #   seem like it would be better to preserve an exisiting value.
+        # self.config.setdefault('prefix_len', CACHE_PREFIX_LEN)
+        self.config['prefix_len'] = CACHE_PREFIX_LEN
+        with SCons.Util.FileLock(config_file, timeout=5, writer=True), open(
+            config_file, "x"
+        ) as config:
+            try:
+                json.dump(self.config, config)
+            except Exception:
+                msg = "Failed to write cache configuration for " + path
+                raise SCons.Errors.SConsEnvironmentError(msg)
 
+        # Add the tag file "carelessly" - the contents are not used by SCons
+        # so we don't care about the chance of concurrent writes.
+        try:
+            tagfile = os.path.join(path, "CACHEDIR.TAG")
+            with open(tagfile, 'xb') as cachedir_tag:
+                cachedir_tag.write(CACHE_TAG)
+        except FileExistsError:
+            pass
 
-    def _readconfig(self, path):
-        """
-        Read the cache config.
+    def _mkdir_atomic(self, path: str) -> bool:
+        """Create cache directory at *path*.
 
-        If directory or config file do not exist, create.  Take advantage
-        of Py3 capability in os.makedirs() and in file open(): just try
-        the operation and handle failure appropriately.
+        Uses directory renaming to avoid races.  If we are actually
+        creating the dir, populate it with the metadata files at the
+        same time as that's the safest way. But it's not illegal to point
+        CacheDir at an existing directory that wasn't a cache previously,
+        so we may have to do that elsewhere, too.
 
-        Omit the check for old cache format, assume that's old enough
-        there will be none of those left to worry about.
+        Returns:
+            ``True`` if it we created the dir, ``False`` if already existed,
 
-        :param path: path to the cache directory
+        Raises:
+            SConsEnvironmentError: if we tried and failed to create the cache.
         """
-        config_file = os.path.join(path, 'config')
+        directory = os.path.abspath(path)
+        if os.path.exists(directory):
+            return False
+
         try:
-            # still use a try block even with exist_ok, might have other fails
-            os.makedirs(path, exist_ok=True)
-        except OSError:
+            tempdir = tempfile.TemporaryDirectory(dir=os.path.dirname(directory))
+        except OSError as e:
             msg = "Failed to create cache directory " + path
-            raise SCons.Errors.SConsEnvironmentError(msg)
+            raise SCons.Errors.SConsEnvironmentError(msg) from e
+        self._add_config(tempdir.name)
+        with tempdir:
+            try:
+                os.rename(tempdir.name, directory)
+                return True
+            except Exception as e:
+                # did someone else get there first?
+                if os.path.isdir(directory):
+                    return False
+                msg = "Failed to create cache directory " + path
+                raise SCons.Errors.SConsEnvironmentError(msg) from e
+
+    def _readconfig(self, path: str) -> None:
+        """Read the cache config from *path*.
 
+        If directory or config file do not exist, create and populate.
+        """
+        config_file = os.path.join(path, 'config')
+        created = self._mkdir_atomic(path)
+        if not created and not os.path.isfile(config_file):
+            # Could have been passed an empty directory
+            self._add_config(path)
         try:
-            with SCons.Util.FileLock(config_file, timeout=5, writer=True), open(
-                config_file, "x"
+            with SCons.Util.FileLock(config_file, timeout=5, writer=False), open(
+                config_file
             ) as config:
-                self.config['prefix_len'] = 2
-                try:
-                    json.dump(self.config, config)
-                except Exception:
-                    msg = "Failed to write cache configuration for " + path
-                    raise SCons.Errors.SConsEnvironmentError(msg)
-        except FileExistsError:
-            try:
-                with SCons.Util.FileLock(config_file, timeout=5, writer=False), open(
-                    config_file
-                ) as config:
-                    self.config = json.load(config)
-            except (ValueError, json.decoder.JSONDecodeError):
-                msg = "Failed to read cache configuration for " + path
-                raise SCons.Errors.SConsEnvironmentError(msg)
+                self.config = json.load(config)
+        except (ValueError, json.decoder.JSONDecodeError):
+            msg = "Failed to read cache configuration for " + path
+            raise SCons.Errors.SConsEnvironmentError(msg)
 
     def CacheDebug(self, fmt, target, cachefile) -> None:
         if cache_debug != self.current_cache_debug:
@@ -252,7 +302,7 @@ class CacheDir:
     def is_readonly(self) -> bool:
         return cache_readonly
 
-    def get_cachedir_csig(self, node):
+    def get_cachedir_csig(self, node) -> str:
         cachedir, cachefile = self.cachepath(node)
         if cachefile and os.path.exists(cachefile):
             return SCons.Util.hash_file_signature(cachefile, SCons.Node.FS.File.hash_chunksize)
diff --git a/SCons/CacheDirTests.py b/SCons/CacheDirTests.py
index 3fbab4e..0ecd502 100644
--- a/SCons/CacheDirTests.py
+++ b/SCons/CacheDirTests.py
@@ -31,6 +31,7 @@ import stat
 from TestCmd import TestCmd, IS_WINDOWS, IS_ROOT
 
 import SCons.CacheDir
+import SCons.Node.FS
 
 built_it = None
 
@@ -62,15 +63,11 @@ class Environment:
         return self.cachedir
 
 class BaseTestCase(unittest.TestCase):
-    """
-    Base fixtures common to our other unittest classes.
-    """
+    """Base fixtures common to our other unittest classes."""
+
     def setUp(self) -> None:
         self.test = TestCmd(workdir='')
-
-        import SCons.Node.FS
         self.fs = SCons.Node.FS.FS()
-
         self._CacheDir = SCons.CacheDir.CacheDir('cache')
 
     def File(self, name, bsig=None, action=Action()):
@@ -83,13 +80,11 @@ class BaseTestCase(unittest.TestCase):
         return node
 
     def tearDown(self) -> None:
-        os.remove(os.path.join(self._CacheDir.path, 'config'))
-        os.rmdir(self._CacheDir.path)
+        shutil.rmtree(self._CacheDir.path)
 
 class CacheDirTestCase(BaseTestCase):
-    """
-    Test calling CacheDir code directly.
-    """
+    """Test calling CacheDir code directly."""
+
     def test_cachepath(self) -> None:
         """Test the cachepath() method"""
 
@@ -97,6 +92,7 @@ class CacheDirTestCase(BaseTestCase):
         # of the file in cache.
         def my_collect(list, hash_format=None):
             return list[0]
+
         save_collect = SCons.Util.hash_collect
         SCons.Util.hash_collect = my_collect
 
@@ -111,6 +107,21 @@ class CacheDirTestCase(BaseTestCase):
         finally:
             SCons.Util.hash_collect = save_collect
 
+class CacheDirExistsTestCase(unittest.TestCase):
+    """Test passing an existing but not setup cache directory."""
+
+    def setUp(self) -> None:
+        self.test = TestCmd(workdir='')
+        self.test.subdir('ex-cache')  # force an empty dir
+        cache = self.test.workpath('ex-cache')
+        self.fs = SCons.Node.FS.FS()
+        self._CacheDir = SCons.CacheDir.CacheDir(cache)
+
+    def test_existing_cachedir(self) -> None:
+        """Test the setup happened even though cache already existed."""
+        assert os.path.exists(self.test.workpath('ex-cache', 'config'))
+        assert os.path.exists(self.test.workpath('ex-cache', 'CACHEDIR.TAG'))
+
 class ExceptionTestCase(unittest.TestCase):
     """Test that the correct exceptions are thrown by CacheDir."""
 
@@ -147,13 +158,14 @@ class ExceptionTestCase(unittest.TestCase):
         test.writable(privileged_dir, True)
         shutil.rmtree(privileged_dir)
 
-
     def test_throws_correct_when_failed_to_write_configfile(self) -> None:
+        """Test for correct error if cache config file cannot be created."""
+
         class Unserializable:
-            """A class which the JSON should not be able to serialize"""
+            """A class which the JSON module should not be able to serialize."""
 
             def __init__(self, oldconfig) -> None:
-                self.something = 1 # Make the object unserializable
+                self.something = 1  # Make the object unserializable
                 # Pretend to be the old config just enough
                 self.__dict__["prefix_len"] = oldconfig["prefix_len"]
 
@@ -168,16 +180,17 @@ class ExceptionTestCase(unittest.TestCase):
 
         oldconfig = self._CacheDir.config
         self._CacheDir.config = Unserializable(oldconfig)
+
         # Remove the config file that got created on object creation
         # so that _readconfig* will try to rewrite it
         old_config = os.path.join(self._CacheDir.path, "config")
         os.remove(old_config)
-
-        try:
+        with self.assertRaises(SCons.Errors.SConsEnvironmentError) as cm:
             self._CacheDir._readconfig(self._CacheDir.path)
-            assert False, "Should have raised exception and did not"
-        except SCons.Errors.SConsEnvironmentError as e:
-            assert str(e) == "Failed to write cache configuration for {}".format(self._CacheDir.path)
+        self.assertEqual(
+            str(cm.exception),
+            "Failed to write cache configuration for " + self._CacheDir.path,
+        )
 
     def test_raise_environment_error_on_invalid_json(self) -> None:
         config_file = os.path.join(self._CacheDir.path, "config")
@@ -188,17 +201,16 @@ class ExceptionTestCase(unittest.TestCase):
         with open(config_file, "w") as cfg:
             cfg.write(content)
 
-        try:
-            # Construct a new cache dir that will try to read the invalid config
+        with self.assertRaises(SCons.Errors.SConsEnvironmentError) as cm:
+            # Construct a new cachedir that will try to read the invalid config
             new_cache_dir = SCons.CacheDir.CacheDir(self._CacheDir.path)
-            assert False, "Should have raised exception and did not"
-        except SCons.Errors.SConsEnvironmentError as e:
-            assert str(e) == "Failed to read cache configuration for {}".format(self._CacheDir.path)
+        self.assertEqual(
+            str(cm.exception),
+            "Failed to read cache configuration for " + self._CacheDir.path,
+        )
 
 class FileTestCase(BaseTestCase):
-    """
-    Test calling CacheDir code through Node.FS.File interfaces.
-    """
+    """Test calling CacheDir code through Node.FS.File interfaces."""
     # These tests were originally in Nodes/FSTests.py and got moved
     # when the CacheDir support was refactored into its own module.
     # Look in the history for Node/FSTests.py if any of this needs
@@ -274,9 +286,7 @@ class FileTestCase(BaseTestCase):
 
     def test_CachePush(self) -> None:
         """Test the CachePush() function"""
-
         save_CachePush = SCons.CacheDir.CachePush
-
         SCons.CacheDir.CachePush = self.push
 
         try:
@@ -309,7 +319,6 @@ class FileTestCase(BaseTestCase):
 
     def test_warning(self) -> None:
         """Test raising a warning if we can't copy a file to cache."""
-
         test = TestCmd(workdir='')
 
         save_copy2 = shutil.copy2
@@ -337,7 +346,6 @@ class FileTestCase(BaseTestCase):
 
     def test_no_strfunction(self) -> None:
         """Test handling no strfunction() for an action."""
-
         save_CacheRetrieveSilent = SCons.CacheDir.CacheRetrieveSilent
 
         f8 = self.File("cd.f8", 'f8_bsig')