Merge pull request #4614 from mwichmann/test/fix-root

Adjust tests in case running as root

CHANGES.txt

@@ -68,6 +68,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.
     - Clarify documentation of Repository() in manpage and user guide.
 
 

RELEASE.txt

@@ -37,18 +37,20 @@ CHANGED/ENHANCED EXISTING FUNCTIONALITY
 
 FIXES
 -----
-
 - 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
@@ -59,6 +61,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
 ------------
 
@@ -67,13 +72,13 @@ IMPROVEMENTS
   under which they would be observed), or major code cleanups
 
 - For consistency with the optparse "add_option" method, AddOption accepts
-  an SConsOption object as a single argument (this failed previouly).
+  an SConsOption object as a single argument (this failed previously).
   Calling AddOption with the full set of arguments (option names and
   attributes) to set up the option is still the recommended approach.
 
 - Add clang and clang++ to the default tool search orders for POSIX
   and Windows platforms. These will be searched for after gcc and g++,
-  respectively. Does not affect expliclity requested tool lists.  Note:
+  respectively. Does not affect explicitly requested tool lists.  Note:
   on Windows, SCons currently only has builtin support for clang, not
   for clang-cl, the version of the frontend that uses cl.exe-compatible
   command line switches.

SCons/CacheDirTests.py

@@ -28,7 +28,7 @@
 import tempfile
 import stat
 
-from TestCmd import TestCmd
+from TestCmd import TestCmd, IS_WINDOWS, IS_ROOT
 
 import SCons.CacheDir
 
@@ -85,7 +85,6 @@ def File(self, name, bsig=None, action=Action()):
     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 +123,29 @@ def setUp(self) -> None:
     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:

SCons/Node/FSTests.py

@@ -31,7 +31,7 @@
 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 @@
 
 scanner_count = 0
 
-
 class Scanner:
     def __init__(self, node=None) -> None:
         global scanner_count
@@ -513,7 +512,7 @@ def copy(self, src, dest) -> None:
             # 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 @@ def test_isfile(self) -> None:
         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 @@ def test_runTest(self):
         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 @@ def nonexistent(method, s):
         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 @@ def nonexistent(method, s):
         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 @@ def nonexistent(method, s):
             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 @@ def test_lookup_abs(self) -> None:
         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 @@ def test_lookup_uncpath(self) -> None:
         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 @@ def test_rel_path(self) -> None:
             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')

SCons/Variables/PathVariableTests.py

@@ -28,7 +28,7 @@
 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 @@ def test_PathIsDir(self):
         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 @@ def test_PathIsDirCreate(self):
         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 @@ def test_PathIsDirCreate(self):
         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',

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
 

test/Install/Install.py

@@ -31,6 +31,7 @@
 import time
 
 import TestSCons
+from TestCmd import IS_ROOT
 
 test = TestSCons.TestSCons()
 
@@ -131,17 +132,17 @@ def my_install(dest, source, env):
 # 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",
-    ]
-
-    test.run(chdir='work', arguments=f1_out, stderr=None, status=2)
-
-    test.must_contain_any_line(test.stderr(), expect)
+# This test is not designed to work if running as root
+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()