Merge branch 'master' into jbrill-msvs-tests

Manually resolve conflicts in CHANGES.txt.

7 files changed, 269 insertions, 188 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 6ec9e84..0ecd502 100644
--- a/SCons/CacheDirTests.py
+++ b/SCons/CacheDirTests.py
@@ -28,9 +28,10 @@ import unittest
 import tempfile
 import stat
 
-from TestCmd import TestCmd
+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,14 +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)
-        # Should that be shutil.rmtree?
+        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"""
 
@@ -98,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
 
@@ -112,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."""
 
@@ -124,28 +134,38 @@ class ExceptionTestCase(unittest.TestCase):
     def tearDown(self) -> None:
         shutil.rmtree(self.tmpdir)
 
-    @unittest.skipIf(sys.platform.startswith("win"), "This fixture will not trigger an OSError on Windows")
+    @unittest.skipIf(
+        IS_WINDOWS,
+        "Skip privileged CacheDir test on Windows, cannot change directory rights",
+    )
+    @unittest.skipIf(
+        IS_ROOT,
+        "Skip privileged CacheDir test if running as root.",
+    )
     def test_throws_correct_on_OSError(self) -> None:
-        """Test that the correct error is thrown when cache directory cannot be created."""
+        """Test for correct error when cache directory cannot be created."""
+        test = TestCmd()
         privileged_dir = os.path.join(self.tmpdir, "privileged")
-        try:
-            os.mkdir(privileged_dir)
-            os.chmod(privileged_dir, stat.S_IREAD)
-            cd = SCons.CacheDir.CacheDir(os.path.join(privileged_dir, "cache"))
-            assert False, "Should have raised exception and did not"
-        except SCons.Errors.SConsEnvironmentError as e:
-            assert str(e) == "Failed to create cache directory {}".format(os.path.join(privileged_dir, "cache"))
-        finally:
-            os.chmod(privileged_dir, stat.S_IWRITE | stat.S_IEXEC | stat.S_IREAD)
-            shutil.rmtree(privileged_dir)
-
+        cachedir_path = os.path.join(privileged_dir, "cache")
+        os.makedirs(privileged_dir, exist_ok=True)
+        test.writable(privileged_dir, False)
+        with self.assertRaises(SCons.Errors.SConsEnvironmentError) as cm:
+            cd = SCons.CacheDir.CacheDir(cachedir_path)
+        self.assertEqual(
+            str(cm.exception),
+            "Failed to create cache directory " + cachedir_path
+        )
+        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"]
 
@@ -160,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")
@@ -180,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
@@ -266,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:
@@ -301,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
@@ -329,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')
diff --git a/SCons/Environment.py b/SCons/Environment.py
index 72572db..ad50456 100644
--- a/SCons/Environment.py
+++ b/SCons/Environment.py
@@ -1749,7 +1749,7 @@ class Base(SubstitutionEnvironment):
         Raises:
            ValueError: *format* is not a recognized serialization format.
 
-        .. versionchanged:: NEXT_VERSION
+        .. versionchanged:: NEXT_RELEASE
            *key* is no longer limited to a single construction variable name.
            If *key* is supplied, a formatted dictionary is generated like the
            no-arg case - previously a single *key* displayed just the value.
diff --git a/SCons/Environment.xml b/SCons/Environment.xml
index 97d0343..403d6b8 100644
--- a/SCons/Environment.xml
+++ b/SCons/Environment.xml
@@ -155,7 +155,7 @@ for more information.
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -166,7 +166,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -177,7 +177,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -188,7 +188,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -199,7 +199,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -210,7 +210,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -221,7 +221,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -232,7 +232,7 @@ for more information).
 <summary>
 <para>
 A reserved variable name
-that may not be set or used in a construction environment.
+that may not be set or used in a &consenv;.
 (See the manpage section "Variable Substitution"
 for more information).
 </para>
@@ -279,13 +279,12 @@ for a complete explanation of the arguments and behavior.
 <para>
 Note that the &f-env-Action;
 form of the invocation will expand
-construction variables in any argument strings,
+&consvars; in any argument strings,
 including the
 <parameter>action</parameter>
 argument, at the time it is called
-using the construction variables in the
-<replaceable>env</replaceable>
-construction environment through which
+using the &consvars; in the
+&consenv; through which
 &f-env-Action; was called.
 The &f-Action; global function
 form delays all variable expansion
@@ -862,14 +861,14 @@ for a complete explanation of the arguments and behavior.
 Note that the
 <function>env.Builder</function>()
 form of the invocation will expand
-construction variables in any arguments strings,
+&consvars; in any arguments strings,
 including the
 <parameter>action</parameter>
 argument,
 at the time it is called
-using the construction variables in the
+using the &consvars; in the
 <varname>env</varname>
-construction environment through which
+&consenv; through which
 &f-env-Builder; was called.
 The
 &f-Builder;
@@ -903,12 +902,12 @@ disables derived file caching.
 Calling the environment method
 &f-link-env-CacheDir;
 limits the effect to targets built
-through the specified construction environment.
+through the specified &consenv;.
 Calling the global function
 &f-link-CacheDir;
 sets a global default
 that will be used by all targets built
-through construction environments
+through &consenvs;
 that do not set up environment-specific
 caching by calling &f-env-CacheDir;.
 </para>
@@ -1034,7 +1033,7 @@ either as separate arguments to the
 &f-Clean;
 method, or as a list.
 &f-Clean;
-will also accept the return value of any of the construction environment
+will also accept the return value of any of the &consenv;
 Builder methods.
 Examples:
 </para>
@@ -1327,13 +1326,11 @@ for a complete explanation of the arguments and behavior.
 <summary>
 <para>
 Specifies that all up-to-date decisions for
-targets built through this construction environment
-will be handled by the specified
-<parameter>function</parameter>.
+targets built through this &consenv;
+will be handled by <parameter>function</parameter>.
 <parameter>function</parameter> can be the name of
 a function or one of the following strings
-that specify the predefined decision function
-that will be applied:
+that specify a predefined decider function:
 </para>
 
 <para>
@@ -1440,7 +1437,7 @@ Examples:
 Decider('timestamp-match')
 
 # Use hash content signatures for any targets built
-# with the attached construction environment.
+# with the attached &consenv;.
 env.Decider('content')
 </example_commands>
 
@@ -1746,7 +1743,7 @@ only those keys and their values are serialized.
 </para>
 
 <para>
-<emphasis>Changed in NEXT_VERSION</emphasis>:
+<emphasis>Changed in NEXT_RELEASE</emphasis>:
 More than one <parameter>key</parameter> can be specified.
 The returned string always looks like a dict (or JSON equivalent);
 previously a single key serialized only the value,
@@ -2269,14 +2266,14 @@ is non-zero,
 adds the names of the default builders
 (Program, Library, etc.)
 to the global name space
-so they can be called without an explicit construction environment.
+so they can be called without an explicit &consenv;.
 (This is the default.)
 When
 <parameter>flag</parameter>
 is zero,
 the names of the default builders are removed
 from the global name space
-so that an explicit construction environment is required
+so that an explicit &consenv; is required
 to call all builders.
 </para>
 </summary>
@@ -2334,7 +2331,7 @@ env.Ignore('bar', 'bar/foobar.obj')
 The specified
 <parameter>string</parameter>
 will be preserved as-is
-and not have construction variables expanded.
+and not have &consvars; expanded.
 </para>
 </summary>
 </scons_function>
@@ -2493,7 +2490,7 @@ either as separate arguments to the
 &f-NoCache;
 method, or as a list.
 &f-NoCache;
-will also accept the return value of any of the construction environment
+will also accept the return value of any of the &consenv;
 Builder methods.
 </para>
 
@@ -2544,7 +2541,7 @@ either as separate arguments to the
 &f-NoClean;
 method, or as a list.
 &f-NoClean;
-will also accept the return value of any of the construction environment
+will also accept the return value of any of the &consenv;
 Builder methods.
 </para>
 
@@ -2589,7 +2586,7 @@ is omitted or <constant>None</constant>,
 &f-link-env-MergeFlags; is used.
 By default,
 duplicate values are not
-added to any construction variables;
+added to any &consvars;;
 you can specify
 <parameter>unique=False</parameter>
 to allow duplicate values to be added.
@@ -2613,7 +2610,7 @@ in order to distribute it to appropriate &consvars;.
 &f-env-MergeFlags; uses a separate function to
 do that processing -
 see &f-link-env-ParseFlags; for the details, including a
-a table of options and corresponding construction variables.
+a table of options and corresponding &consvars;.
 To provide alternative processing of the output of
 <parameter>command</parameter>,
 you can suppply a custom
@@ -2696,12 +2693,12 @@ function.
 Parses one or more strings containing
 typical command-line flags for GCC-style tool chains
 and returns a dictionary with the flag values
-separated into the appropriate SCons construction variables.
+separated into the appropriate SCons &consvars;.
 Intended as a companion to the
 &f-link-env-MergeFlags;
 method, but allows for the values in the returned dictionary
 to be modified, if necessary,
-before merging them into the construction environment.
+before merging them into the &consenv;.
 (Note that
 &f-env-MergeFlags;
 will call this method if its argument is not a dictionary,
@@ -2730,7 +2727,7 @@ See &f-link-ParseConfig; for more details.
 
 <para>
 Flag values are translated according to the prefix found,
-and added to the following construction variables:
+and added to the following &consvars;:
 </para>
 
 <example_commands>
@@ -2770,8 +2767,7 @@ and added to the following construction variables:
 Any other strings not associated with options
 are assumed to be the names of libraries
 and added to the
-&cv-LIBS;
-construction variable.
+&cv-LIBS; &consvar;.
 </para>
 
 <para>
@@ -2802,7 +2798,7 @@ selected by <parameter>plat</parameter>
 (defaults to the detected platform for the
 current system)
 that can be used to initialize
-a construction environment by passing it as the
+a &consenv; by passing it as the
 <parameter>platform</parameter> keyword argument to the
 &f-link-Environment; function.
 </para>
@@ -2999,7 +2995,7 @@ env = Environment(
 </arguments>
 <summary>
 <para>
-Replaces construction variables in the Environment
+Replaces &consvars; in the Environment
 with the specified keyword arguments.
 </para>
 
@@ -3019,50 +3015,43 @@ env.Replace(CCFLAGS='-g', FOO='foo.xxx')
 </arguments>
 <summary>
 <para>
-Specifies that
+Sets
 <parameter>directory</parameter>
-is a repository to be searched for files.
+as a repository to be searched for files contributing to the build.
 Multiple calls to
 &f-Repository;
-are legal,
-and each one adds to the list of
-repositories that will be searched.
+are allowed,
+with repositories searched in the given order.
+Repositories specified via command-line option
+have higher priority.
 </para>
 
 <para>
-To
+In
 &scons;,
-a repository is a copy of the source tree,
-from the top-level directory on down,
-which may contain
-both source files and derived files
+a repository is partial or complete copy of the source tree,
+from the top-level directory down,
+containing source files
 that can be used to build targets in
-the local source tree.
-The canonical example would be an
-official source tree maintained by an integrator.
-If the repository contains derived files,
-then the derived files should have been built using
-&scons;,
-so that the repository contains the necessary
-signature information to allow
-&scons;
-to figure out when it is appropriate to
-use the repository copy of a derived file,
-instead of building one locally.
+the current worktree.
+Repositories can also contain derived files.
+An example might be an official source tree maintained by an integrator.
+If a repository contains derived files,
+they should be the result of building with &SCons;,
+so a signature database (sconsign) is present
+in the repository,
+allowing better decisions on whether they are
+up-to-date or not.
 </para>
 
 <para>
 Note that if an up-to-date derived file
 already exists in a repository,
-&scons;
-will
+&scons; will
 <emphasis>not</emphasis>
 make a copy in the local directory tree.
-In order to guarantee that a local copy
-will be made,
-use the
-&f-link-Local;
-method.
+If you need a local copy to be made,
+use the &f-link-Local; method.
 </para>
 </summary>
 </scons_function>
@@ -3267,7 +3256,7 @@ SConsignFile(dbm_module=dbm.gnu)
 </arguments>
 <summary>
 <para>
-Sets construction variables to default values specified with the keyword
+Sets &consvars; to default values specified with the keyword
 arguments if (and only if) the variables are not already set.
 The following statements are equivalent:
 </para>
diff --git a/SCons/EnvironmentTests.py b/SCons/EnvironmentTests.py
index a37e424..62238c7 100644
--- a/SCons/EnvironmentTests.py
+++ b/SCons/EnvironmentTests.py
@@ -3202,7 +3202,7 @@ def generate(env):
         """Test the Dump() method"""
         env = self.TestEnvironment(FOO='foo', FOOFLAGS=CLVar('--bar --baz'))
 
-        # changed in NEXT_VERSION: single arg now displays as a dict,
+        # changed in NEXT_RELEASE: single arg now displays as a dict,
         #   not a bare value; more than one arg is allowed.
         with self.subTest():  # one-arg version
             self.assertEqual(env.Dump('FOO'), "{'FOO': 'foo'}")
@@ -3842,7 +3842,7 @@ class OverrideEnvironmentTestCase(unittest.TestCase,TestEnvironmentFixture):
         """Test deleting variables from an OverrideEnvironment"""
         env, env2, env3 = self.envs
 
-        # changed in NEXT_VERSION: delete does not cascade to underlying envs
+        # changed in NEXT_RELEASE: delete does not cascade to underlying envs
         # XXX is in all three, del from env3 should affect only it
         del env3['XXX']
         with self.subTest():
@@ -3931,7 +3931,7 @@ class OverrideEnvironmentTestCase(unittest.TestCase,TestEnvironmentFixture):
         # test deletion in top override
         del env3['XXX']
         self.assertRaises(KeyError, env3.Dictionary, 'XXX')
-        # changed in NEXT_VERSION: *not* deleted from underlying envs
+        # changed in NEXT_RELEASE: *not* deleted from underlying envs
         assert 'XXX' in env2.Dictionary()
         assert 'XXX' in env.Dictionary()
 
diff --git a/SCons/Node/FSTests.py b/SCons/Node/FSTests.py
index 2036f92..83ceef2 100644
--- a/SCons/Node/FSTests.py
+++ b/SCons/Node/FSTests.py
@@ -31,7 +31,7 @@ import shutil
 import stat
 from typing import Optional
 
-from TestCmd import TestCmd, IS_WINDOWS
+from TestCmd import TestCmd, IS_WINDOWS, IS_ROOT
 
 import SCons.Errors
 import SCons.Node.FS
@@ -44,7 +44,6 @@ built_it = None
 
 scanner_count = 0
 
-
 class Scanner:
     def __init__(self, node=None) -> None:
         global scanner_count
@@ -513,7 +512,7 @@ class VariantDirTestCase(unittest.TestCase):
             # Disable symlink and link for now in win32.
             # We don't have a consistant plan to make these work as yet
             # They are only supported with PY3
-            if sys.platform == 'win32':
+            if IS_WINDOWS:
                 real_symlink = None
                 real_link = None
 
@@ -706,7 +705,7 @@ class BaseTestCase(_tempdirTestCase):
         nonexistent = fs.Entry('nonexistent')
         assert not nonexistent.isfile()
 
-    @unittest.skipUnless(sys.platform != 'win32' and hasattr(os, 'symlink'),
+    @unittest.skipIf(IS_WINDOWS or not hasattr(os, 'symlink'),
                          "symlink is not used on Windows")
     def test_islink(self) -> None:
         """Test the Base.islink() method"""
@@ -958,6 +957,8 @@ class FSTestCase(_tempdirTestCase):
         This test case handles all of the file system node
         tests in one environment, so we don't have to set up a
         complicated directory structure for each test individually.
+        This isn't ideal: normally you want to separate tests a bit
+        more to make it easier to debug and not fail too fast.
         """
         test = self.test
 
@@ -1449,7 +1450,7 @@ class FSTestCase(_tempdirTestCase):
         except SyntaxError:
             assert c == ""
 
-        if sys.platform != 'win32' and hasattr(os, 'symlink'):
+        if not IS_WINDOWS and hasattr(os, 'symlink'):
             os.symlink('nonexistent', test.workpath('dangling_symlink'))
             e = fs.Entry('dangling_symlink')
             c = e.get_contents()
@@ -1541,7 +1542,7 @@ class FSTestCase(_tempdirTestCase):
         assert r, r
         assert not os.path.exists(test.workpath('exists')), "exists was not removed"
 
-        if sys.platform != 'win32' and hasattr(os, 'symlink'):
+        if not IS_WINDOWS and hasattr(os, 'symlink'):
             symlink = test.workpath('symlink')
             os.symlink(test.workpath('does_not_exist'), symlink)
             assert os.path.islink(symlink)
@@ -1550,27 +1551,30 @@ class FSTestCase(_tempdirTestCase):
             assert r, r
             assert not os.path.islink(symlink), "symlink was not removed"
 
-        test.write('can_not_remove', "can_not_remove\n")
-        test.writable(test.workpath('.'), 0)
-        fp = open(test.workpath('can_not_remove'))
-
-        f = fs.File('can_not_remove')
-        exc_caught = 0
-        try:
-            r = f.remove()
-        except OSError:
-            exc_caught = 1
-
-        fp.close()
-
-        assert exc_caught, "Should have caught an OSError, r = " + str(r)
-
         f = fs.Entry('foo/bar/baz')
         assert f.for_signature() == 'baz', f.for_signature()
         assert f.get_string(0) == os.path.normpath('foo/bar/baz'), \
             f.get_string(0)
         assert f.get_string(1) == 'baz', f.get_string(1)
 
+
+    @unittest.skipIf(IS_ROOT, "Skip file removal in protected dir if running as root.")
+    def test_remove_fail(self) -> None:
+        """Test failure when removing a file where permissions don't allow.
+
+        Split from :math:`test_runTest` to be able to skip on root.
+        We want to be able to skip only this one testcase and run the rest.
+        """
+        test = self.test
+        fs = SCons.Node.FS.FS()
+        test.write('can_not_remove', "can_not_remove\n")
+        test.writable(test.workpath('.'), False)
+        with open(test.workpath('can_not_remove')):
+            f = fs.File('can_not_remove')
+            with self.assertRaises(OSError, msg="Should have caught an OSError"):
+                r = f.remove()
+
+
     def test_drive_letters(self) -> None:
         """Test drive-letter look-ups"""
 
@@ -1847,7 +1851,7 @@ class FSTestCase(_tempdirTestCase):
         d = root._lookup_abs('/tmp/foo-nonexistent/nonexistent-dir', SCons.Node.FS.Dir)
         assert d.__class__ == SCons.Node.FS.Dir, str(d.__class__)
 
-    @unittest.skipUnless(sys.platform == "win32", "requires Windows")
+    @unittest.skipUnless(IS_WINDOWS, "requires Windows")
     def test_lookup_uncpath(self) -> None:
         """Testing looking up a UNC path on Windows"""
         test = self.test
@@ -1859,13 +1863,13 @@ class FSTestCase(_tempdirTestCase):
         assert str(f) == r'\\servername\C$\foo', \
             'UNC path %s got looked up as %s' % (path, f)
 
-    @unittest.skipUnless(sys.platform.startswith == "win32", "requires Windows")
+    @unittest.skipUnless(IS_WINDOWS, "requires Windows")
     def test_unc_drive_letter(self) -> None:
         """Test drive-letter lookup for windows UNC-style directories"""
         share = self.fs.Dir(r'\\SERVER\SHARE\Directory')
         assert str(share) == r'\\SERVER\SHARE\Directory', str(share)
 
-    @unittest.skipUnless(sys.platform == "win32", "requires Windows")
+    @unittest.skipUnless(IS_WINDOWS, "requires Windows")
     def test_UNC_dirs_2689(self) -> None:
         """Test some UNC dirs that printed incorrectly and/or caused
         infinite recursion errors prior to r5180 (SCons 2.1)."""
@@ -1928,7 +1932,7 @@ class FSTestCase(_tempdirTestCase):
             d1_d2_f, d3_d4_f, '../../d3/d4/f',
         ]
 
-        if sys.platform in ('win32',):
+        if IS_WINDOWS:
             x_d1 = fs.Dir(r'X:\d1')
             x_d1_d2 = x_d1.Dir('d2')
             y_d1 = fs.Dir(r'Y:\d1')
diff --git a/SCons/Variables/PathVariableTests.py b/SCons/Variables/PathVariableTests.py
index 7fa8da1..b093053 100644
--- a/SCons/Variables/PathVariableTests.py
+++ b/SCons/Variables/PathVariableTests.py
@@ -28,7 +28,7 @@ import SCons.Errors
 import SCons.Variables
 
 import TestCmd
-from TestCmd import IS_WINDOWS
+from TestCmd import IS_WINDOWS, IS_ROOT
 
 class PathVariableTestCase(unittest.TestCase):
     def test_PathVariable(self) -> None:
@@ -93,7 +93,7 @@ class PathVariableTestCase(unittest.TestCase):
         self.assertEqual(str(e), f"Directory path for variable 'X' does not exist: {dne}")
 
     def test_PathIsDirCreate(self):
-        """Test the PathIsDirCreate validator"""
+        """Test the PathIsDirCreate validator."""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
                                           'test build variable help',
@@ -115,6 +115,27 @@ class PathVariableTestCase(unittest.TestCase):
         e = cm.exception
         self.assertEqual(str(e), f"Path for variable 'X' is a file, not a directory: {f}")
 
+
+    @unittest.skipIf(IS_ROOT, "Skip creating bad directory if running as root.")
+    def test_PathIsDirCreate_bad_dir(self):
+        """Test the PathIsDirCreate validator with an uncreatable dir.
+
+        Split from :meth:`test_PathIsDirCreate` to be able to skip on root.
+        We want to be able to skip only this one testcase and run the rest.
+        """
+        opts = SCons.Variables.Variables()
+        opts.Add(
+            SCons.Variables.PathVariable(
+                'test',
+                'test build variable help',
+                '/default/path',
+                SCons.Variables.PathVariable.PathIsDirCreate,
+            )
+        )
+
+        test = TestCmd.TestCmd(workdir='')
+        o = opts.options[0]
+
         # pick a directory path that can't be mkdir'd
         if IS_WINDOWS:
             f = r'\\noserver\noshare\yyy\zzz'
@@ -125,8 +146,9 @@ class PathVariableTestCase(unittest.TestCase):
         e = cm.exception
         self.assertEqual(str(e), f"Path for variable 'X' could not be created: {f}")
 
+
     def test_PathIsFile(self):
-        """Test the PathIsFile validator"""
+        """Test the PathIsFile validator."""
         opts = SCons.Variables.Variables()
         opts.Add(SCons.Variables.PathVariable('test',
                                           'test build variable help',