Adjust tests in case running as root

Although validation tests are not normally run as root, there may be
cicrumstances when it happens - one known case is when the test suite
is run as part of a particular Linux distros package construction.
It isn't too hard to avoid the few places where we counted on something
failing because of permissions, which don't if the user is root -
added a few skips.

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

9 files changed, 125 insertions, 47 deletions

diff --git a/CHANGES.txt b/CHANGES.txt
index b9467a1..defd0e3 100644
--- a/CHANGES.txt
+++ b/CHANGES.txt
@@ -60,6 +60,8 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
     - Change long-standing irritant in Environment tests - instead of using
       a while loop to pull test info from a list of tests and then delete
       the test, structure the test data as a list of tuples and iterate it.
+    - Skip running a few validation tests if the user is root and the test is
+      not designed to work for the root user.
 
   From Alex James:
     - On Darwin, PermissionErrors are now handled while trying to access
diff --git a/RELEASE.txt b/RELEASE.txt
index 76ae1d5..2e1769e 100644
--- a/RELEASE.txt
+++ b/RELEASE.txt
@@ -37,17 +37,22 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
 FIXES
 -----
 
+- List fixes of outright bugs
+
 - PackageVariable now does what the documentation always said it does
   if the variable is used on the command line with one of the enabling
   string as the value: the variable's default value is produced (previously
   it always produced True in this case).
+
 - Temporary files created by TempFileMunge() are now cleaned up on
   scons exit, instead of at the time they're used.  Fixes #4595.
+
 - AddOption now correctly adds short (single-character) options.
   Previously an added short option would always report as unknown,
   while long option names for the same option worked. Short options
   that take a value require the user to specify the value immediately
   following the option, with no spaces (e.g. -j5 and not -j 5).
+
 - Fix a problem with compilation_db component initialization - the
   entries for assembler files were not being set up correctly.
 - On Darwin, PermissionErrors are now handled while trying to access
@@ -58,6 +63,9 @@ FIXES
 
 - Fix nasm test for missing include file, cleanup.
 
+- Skip running a few validation tests if the user is root and the test is
+  not designed to work for the root user.
+
 IMPROVEMENTS
 ------------
 
diff --git a/SCons/CacheDirTests.py b/SCons/CacheDirTests.py
index 6ec9e84..e904788 100644
--- a/SCons/CacheDirTests.py
+++ b/SCons/CacheDirTests.py
@@ -34,6 +34,13 @@ import SCons.CacheDir
 
 built_it = None
 
+IS_WINDOWS = sys.platform == 'win32'
+try:
+    IS_ROOT = os.geteuid() == 0
+except AttributeError:
+    IS_ROOT = False
+
+
 class Action:
     def __call__(self, targets, sources, env, **kw) -> int:
         global built_it
@@ -85,7 +92,6 @@ class BaseTestCase(unittest.TestCase):
     def tearDown(self) -> None:
         os.remove(os.path.join(self._CacheDir.path, 'config'))
         os.rmdir(self._CacheDir.path)
-        # Should that be shutil.rmtree?
 
 class CacheDirTestCase(BaseTestCase):
     """
@@ -124,20 +130,29 @@ 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:
diff --git a/SCons/Node/FSTests.py b/SCons/Node/FSTests.py
index 2036f92..0f20cd8 100644
--- a/SCons/Node/FSTests.py
+++ b/SCons/Node/FSTests.py
@@ -44,6 +44,11 @@ built_it = None
 
 scanner_count = 0
 
+try:
+    IS_ROOT = os.geteuid() == 0
+except AttributeError:
+    IS_ROOT = False
+
 
 class Scanner:
     def __init__(self, node=None) -> None:
@@ -958,6 +963,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
 
@@ -1550,27 +1557,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"""
 
diff --git a/SCons/Variables/PathVariableTests.py b/SCons/Variables/PathVariableTests.py
index 7fa8da1..87d6bdf 100644
--- a/SCons/Variables/PathVariableTests.py
+++ b/SCons/Variables/PathVariableTests.py
@@ -30,6 +30,12 @@ import SCons.Variables
 import TestCmd
 from TestCmd import IS_WINDOWS
 
+try:
+    IS_ROOT = os.geteuid() == 0
+except AttributeError:
+    IS_ROOT = False
+
+
 class PathVariableTestCase(unittest.TestCase):
     def test_PathVariable(self) -> None:
         """Test PathVariable creation"""
@@ -93,7 +99,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 +121,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 +152,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',
diff --git a/template/RELEASE.txt b/template/RELEASE.txt
index 439c863..77ce83b 100755
--- a/template/RELEASE.txt
+++ b/template/RELEASE.txt
@@ -6,7 +6,7 @@ Past official release announcements appear at:
 
 ==================================================================
 
-A new SCons release, 4.4.1, is now available on the SCons download page:
+A new SCons release, NEXT_RELEASE, is now available on the SCons download page:
 
     https://scons.org/pages/download.html
 
diff --git a/test/Install/Install.py b/test/Install/Install.py
index 2857c72..65725ba 100644
--- a/test/Install/Install.py
+++ b/test/Install/Install.py
@@ -131,16 +131,21 @@ test.must_match(['work', 'f2.out'], "f2.in\n", mode='r')
 # if a target can not be unlinked before building it:
 test.write(['work', 'f1.in'], "f1.in again again\n")
 
-os.chmod(test.workpath('work', 'export'), 0o555)
-with open(f1_out, 'rb'):
-    expect = [
-        "Permission denied",
-        "The process cannot access the file because it is being used by another process",
-        "Der Prozess kann nicht auf die Datei zugreifen, da sie von einem anderen Prozess verwendet wird",
-    ]
+# This test is not designed to work if running as root
+try:
+    IS_ROOT = os.geteuid() == 0
+except AttributeError:
+    IS_ROOT = False
+if not IS_ROOT:
+    os.chmod(test.workpath('work', 'export'), 0o555)
+    with open(f1_out, 'rb'):
+        expect = [
+            "Permission denied",
+            "The process cannot access the file because it is being used by another process",
+            "Der Prozess kann nicht auf die Datei zugreifen, da sie von einem anderen Prozess verwendet wird",
+        ]
 
     test.run(chdir='work', arguments=f1_out, stderr=None, status=2)
-
     test.must_contain_any_line(test.stderr(), expect)
 
 test.pass_test()
diff --git a/test/VariantDir/errors.py b/test/VariantDir/errors.py
index 26ef4a2..531beea 100644
--- a/test/VariantDir/errors.py
+++ b/test/VariantDir/errors.py
@@ -1,6 +1,8 @@
 #!/usr/bin/env python
 #
-# __COPYRIGHT__
+# MIT License
+#
+# Copyright The SCons Foundation
 #
 # Permission is hereby granted, free of charge, to any person obtaining
 # a copy of this software and associated documentation files (the
@@ -20,9 +22,6 @@
 # LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
 # OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION
 # WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
-#
-
-__revision__ = "__FILE__ __REVISION__ __DATE__ __DEVELOPER__"
 
 """
 Validate successful handling of errors when duplicating things in
@@ -37,6 +36,13 @@ import TestSCons
 
 test = TestSCons.TestSCons()
 
+try:
+    IS_ROOT = os.geteuid() == 0
+except AttributeError:
+    IS_ROOT = False
+if IS_ROOT:
+    test.skip_test('SConscript permissions meaningless when running as root; skipping test.\n')
+
 for dir in ['normal', 'ro-dir', 'ro-SConscript', 'ro-src']:
     test.subdir(dir, [dir, 'src'])
 
@@ -44,7 +50,7 @@ for dir in ['normal', 'ro-dir', 'ro-SConscript', 'ro-src']:
 import os.path
 VariantDir('build', 'src')
 SConscript(os.path.join('build', 'SConscript'))
-""") 
+""")
 
     test.write([dir, 'src', 'SConscript'], """\
 def fake_scan(node, env, target):
diff --git a/testing/framework/TestCmd.py b/testing/framework/TestCmd.py
index cada7be..240aa6c 100644
--- a/testing/framework/TestCmd.py
+++ b/testing/framework/TestCmd.py
@@ -2003,11 +2003,15 @@ class TestCmd:
                     do_chmod(os.path.join(dirpath, name))
             do_chmod(top)
 
-    def writable(self, top, write: bool=True) -> None:
+    def writable(self, top, write: bool = True) -> None:
         """Make the specified directory tree writable or unwritable.
 
         Tree is made writable if `write` evaluates True (the default),
         else it is made not writable.
+
+        Note on Windows the only thing we can do is and/remove the
+        "readable" setting without resorting to PyWin32 - and that,
+        only as Administrator, so this is kind of pointless there.
         """
 
         if sys.platform == 'win32':
@@ -2034,7 +2038,7 @@ class TestCmd:
                     except OSError:
                         pass
                     else:
-                        os.chmod(fname, stat.S_IMODE(st[stat.ST_MODE] | 0o200))
+                        os.chmod(fname, stat.S_IMODE(st[stat.ST_MODE] | stat.S_IWRITE))
             else:
                 def do_chmod(fname) -> None:
                     try:
@@ -2043,7 +2047,7 @@ class TestCmd:
                         pass
                     else:
                         os.chmod(fname, stat.S_IMODE(
-                            st[stat.ST_MODE] & ~0o200))
+                            st[stat.ST_MODE] & ~stat.S_IWRITE))
 
         if os.path.isfile(top):
             do_chmod(top)