Merge pull request #4734 from mwichmann/test/node-unc

FS unit test fixes for UNC paths

Author: bdbaddog

CHANGES.txt

@@ -113,6 +113,9 @@ RELEASE  VERSION/DATE TO BE FILLED IN LATER
       of a list of actions. Recognition only happens if the special
       action is first in the list.  Initial suggestion from Julien Pommier.
       Fixes #4731.
+    - Fix a test problem on Windows where UNC tests failed due to incorrect
+      path munging if a non-default %TEMP% was defined (as in moving to
+      a Dev Drive). Also some cleanup.
 
 
 RELEASE 4.9.1 -  Thu, 27 Mar 2025 11:40:20 -0700

RELEASE.txt

@@ -73,6 +73,9 @@ FIXES
   of a list of actions. Recognition only happens if the special
   action is first in the list.
 
+- Fix a test problem on Windows where UNC tests failed due to incorrect path
+  munging if a non-default %TEMP% was defined (as in moving to a Dev Drive).
+
 IMPROVEMENTS
 ------------
 

SCons/Node/FS.py

@@ -145,17 +145,29 @@ def initialize_do_splitdrive() -> None:
     do_splitdrive = bool(os.path.splitdrive('X:/foo')[0])
 
     if do_splitdrive:
+        # Note there's a little dilemma here - we should be able to use the
+        # Python os.path.splitdrive, which is well debugged over the years.
+        # For normal usage it should work, but we also want to as much as
+        # possible run the full testsuite on whichever platform, even if it's
+        # "wrong" for some feature. POSIX splitdrive doesn't do what we want
+        # when running the drive and UNC path tests, and the NT one isn't
+        # designed for use on non-win32.  So for now stuck on our private one.
+        # _my_splitdrive = os.path.splitdrive
+
         def _my_splitdrive(p):
             if p[1:2] == ':':
                 return p[:2], p[2:]
             if p[0:2] == '//':
-                # Note that we leave a leading slash in the path
-                # because UNC paths are always absolute.
+                # We leave a leading slash in the path because UNC paths
+                # are always absolute.
+                #
+                # TODO: returning "//" for the drive part is actually
+                #   completely wrong. UNC paths must have minimum three
+                #   slashes (if using '//server/share/path' style), or five
+                #   ('//?/UNC/server/share/path)' and we should return the
+                #   part up to but not including the next slash.
                 return '//', p[1:]
             return '', p
-    # TODO: the os routine should work and be better debugged than ours,
-    #   but unit test test_unc_path fails on POSIX platforms. Resolve someday.
-    # _my_splitdrive = os.path.splitdrive
 
     # Keep some commonly used values in global variables to skip to
     # module look-up costs.
@@ -1011,7 +1023,7 @@ def __init__(self, name, directory, fs) -> None:
     def diskcheck_match(self) -> None:
         pass
 
-    def disambiguate(self, must_exist=None):
+    def disambiguate(self, must_exist=False):
         """
         """
         if self.isfile():
@@ -1069,7 +1081,7 @@ def get_text_contents(self) -> str:
         system, we check to see into what sort of subclass we should
         morph this Entry."""
         try:
-            self = self.disambiguate(must_exist=1)
+            self = self.disambiguate(must_exist=True)
         except SCons.Errors.UserError:
             # There was nothing on disk with which to disambiguate
             # this entry.  Leave it as an Entry, but return a null

SCons/Node/FSTests.py

@@ -1053,7 +1053,7 @@ def test_runTest(self):
 
         seps = [os.sep]
         if os.sep != '/':
-            seps = seps + ['/']
+            seps.append('/')
 
         drive, path = os.path.splitdrive(os.getcwd())
 
@@ -1089,21 +1089,16 @@ def strip_slash(p, drive=drive):
             else:
                 dir_up_path = dir.up().get_internal_path()
 
-            assert dir.name == name, \
-                "dir.name %s != expected name %s" % \
-                (dir.name, name)
-            assert dir.get_internal_path() == path, \
-                "dir.path %s != expected path %s" % \
-                (dir.get_internal_path(), path)
-            assert str(dir) == path, \
-                "str(dir) %s != expected path %s" % \
-                (str(dir), path)
-            assert dir.get_abspath() == abspath, \
-                "dir.abspath %s != expected absolute path %s" % \
-                (dir.get_abspath(), abspath)
-            assert dir_up_path == up_path, \
-                "dir.up().path %s != expected parent path %s" % \
-                (dir_up_path, up_path)
+            with self.subTest():
+                self.assertEqual(dir.name, name)
+            with self.subTest():
+                self.assertEqual(dir.get_internal_path(), path)
+            with self.subTest():
+                self.assertEqual(str(dir), path)
+            with self.subTest():
+                self.assertEqual(dir.get_abspath(), abspath)
+            with self.subTest():
+                self.assertEqual(dir_up_path, up_path)
 
         for sep in seps:
 
@@ -1583,44 +1578,40 @@ def test_remove_fail(self) -> None:
 
     def test_drive_letters(self) -> None:
         """Test drive-letter look-ups"""
-
         test = self.test
-
         test.subdir('sub', ['sub', 'dir'])
 
+        seps = [os.sep]
+        if os.sep != '/':
+            seps.append('/')
+
         def drive_workpath(dirs, test=test):
             x = test.workpath(*dirs)
             drive, path = os.path.splitdrive(x)
             return 'X:' + path
 
         wp = drive_workpath([''])
-
-        if wp[-1] in (os.sep, '/'):
+        if wp[-1] in seps:
             tmp = os.path.split(wp[:-1])[0]
         else:
             tmp = os.path.split(wp)[0]
-
         parent_tmp = os.path.split(tmp)[0]
         if parent_tmp == 'X:':
             parent_tmp = 'X:' + os.sep
-
         tmp_foo = os.path.join(tmp, 'foo')
-
         foo = drive_workpath(['foo'])
         foo_bar = drive_workpath(['foo', 'bar'])
-        sub = drive_workpath(['sub', ''])
-        sub_dir = drive_workpath(['sub', 'dir', ''])
-        sub_dir_foo = drive_workpath(['sub', 'dir', 'foo', ''])
-        sub_dir_foo_bar = drive_workpath(['sub', 'dir', 'foo', 'bar', ''])
-        sub_foo = drive_workpath(['sub', 'foo', ''])
+        # The following are not used, but kept for reference.
+        # sub = drive_workpath(['sub', ''])
+        # sub_dir = drive_workpath(['sub', 'dir', ''])
+        # sub_dir_foo = drive_workpath(['sub', 'dir', 'foo', ''])
+        # sub_dir_foo_bar = drive_workpath(['sub', 'dir', 'foo', 'bar', ''])
+        # sub_foo = drive_workpath(['sub', 'foo', ''])
 
         fs = SCons.Node.FS.FS()
 
-        seps = [os.sep]
-        if os.sep != '/':
-            seps = seps + ['/']
-
         def _do_Dir_test(lpath, path_, up_path_, sep, fileSys=fs) -> None:
+            """Test the Dir constructor with a given path and expected values."""
             dir = fileSys.Dir(lpath.replace('/', sep))
 
             if os.sep != '/':
@@ -1636,18 +1627,14 @@ def strip_slash(p):
             up_path = strip_slash(up_path_)
             name = path.split(os.sep)[-1]
 
-            assert dir.name == name, \
-                "dir.name %s != expected name %s" % \
-                (dir.name, name)
-            assert dir.get_internal_path() == path, \
-                "dir.path %s != expected path %s" % \
-                (dir.get_internal_path(), path)
-            assert str(dir) == path, \
-                "str(dir) %s != expected path %s" % \
-                (str(dir), path)
-            assert dir.up().get_internal_path() == up_path, \
-                "dir.up().path %s != expected parent path %s" % \
-                (dir.up().get_internal_path(), up_path)
+            with self.subTest():
+                self.assertEqual(dir.name, name)
+            with self.subTest():
+                self.assertEqual(dir.get_internal_path(), path)
+            with self.subTest():
+                self.assertEqual(str(dir), path)
+            with self.subTest():
+                self.assertEqual(dir.up().get_internal_path(), up_path)
 
         save_os_path = os.path
         save_os_sep = os.sep
@@ -1658,6 +1645,7 @@ def strip_slash(p):
             SCons.Node.FS.initialize_do_splitdrive()
 
             for sep in seps:
+
                 def Dir_test(lpath, path_, up_path_, sep=sep, func=_do_Dir_test):
                     return func(lpath, path_, up_path_, sep)
 
@@ -1684,59 +1672,73 @@ def Dir_test(lpath, path_, up_path_, sep=sep, func=_do_Dir_test):
 
     def test_unc_path(self) -> None:
         """Test UNC path look-ups"""
-
         test = self.test
-
         test.subdir('sub', ['sub', 'dir'])
 
-        def strip_slash(p):
-            if p[-1] == os.sep and len(p) > 3:
-                p = p[:-1]
-            return p
+        seps = [os.sep]
+        if os.sep != '/':
+            seps.append('/')
 
         def unc_workpath(dirs, test=test):
+            """Return a UNC-style path for the given directory.
+
+            *dirs* is a list of directory names, which will be joined
+            together to derive the base of the path - any "drive letter"
+            bits split off. Makes no effort to create an actual *valid*
+            UNC path - does not add a server or share name, so it will not
+            actually work in real life, just for string testing.
+
+            So that tests will work on non-Windows systems, we use the ntpath
+            module to perform the splitdrive, as that's otherwise a no-op.
+            """
             import ntpath
+
+            # This duplicates the definition in _do_Dir_test below.
+            # It depends on the monkeypatching of os.sep so we just have two.
+            def strip_slash(p):
+                if p[-1] in seps and len(p) > 3:
+                    p = p[:-1]
+                return p
+
             x = test.workpath(*dirs)
             drive, path = ntpath.splitdrive(x)
-            try:
-                unc, path = ntpath.splitunc(path)
-            except AttributeError:
-                # could be python 3.7 or newer, make sure splitdrive can do UNC
-                assert ntpath.splitdrive(r'\\split\drive\test')[0] == r'\\split\drive'
             path = strip_slash(path)
             return '//' + path[1:]
 
         wp = unc_workpath([''])
-
-        if wp[-1] in (os.sep, '/'):
-            tmp = os.path.split(wp[:-1])[0]
-        else:
-            tmp = os.path.split(wp)[0]
-
-        parent_tmp = os.path.split(tmp)[0]
-
+        tmp = os.path.split(wp)[0]
+        # parent_tmp = os.path.split(tmp)[0]
         tmp_foo = os.path.join(tmp, 'foo')
-
         foo = unc_workpath(['foo'])
         foo_bar = unc_workpath(['foo', 'bar'])
-        sub = unc_workpath(['sub', ''])
-        sub_dir = unc_workpath(['sub', 'dir', ''])
-        sub_dir_foo = unc_workpath(['sub', 'dir', 'foo', ''])
-        sub_dir_foo_bar = unc_workpath(['sub', 'dir', 'foo', 'bar', ''])
-        sub_foo = unc_workpath(['sub', 'foo', ''])
+        # The following are not used, but kept for reference.
+        # sub = unc_workpath(['sub', ''])
+        # sub_dir = unc_workpath(['sub', 'dir', ''])
+        # sub_dir_foo = unc_workpath(['sub', 'dir', 'foo', ''])
+        # sub_dir_foo_bar = unc_workpath(['sub', 'dir', 'foo', 'bar', ''])
+        # sub_foo = unc_workpath(['sub', 'foo', ''])
 
         fs = SCons.Node.FS.FS()
 
-        seps = [os.sep]
-        if os.sep != '/':
-            seps = seps + ['/']
-
-        def _do_Dir_test(lpath, path, up_path, sep, fileSys=fs) -> None:
+        def _do_Dir_test(lpath, path_, up_path_, sep, fileSys=fs) -> None:
+            """Test the Dir constructor with a given path and expected values."""
+            # before = time.perf_counter()
             dir = fileSys.Dir(lpath.replace('/', sep))
+            # after = time.perf_counter()
+            # print(f"TIME: dir creation for {lpath!r} (as {dir.path!r}) took {after - before:.6f} seconds")
 
+            # convert to os-native paths
             if os.sep != '/':
-                path = path.replace('/', os.sep)
-                up_path = up_path.replace('/', os.sep)
+                path_ = path_.replace('/', os.sep)
+                up_path_ = up_path_.replace('/', os.sep)
+
+            def strip_slash(p):
+                if p[-1] in os.sep and len(p) > 3:
+                    p = p[:-1]
+                return p
+
+            path = strip_slash(path_)
+            up_path = strip_slash(up_path_)
 
             if path == os.sep + os.sep:
                 name = os.sep + os.sep
@@ -1748,23 +1750,23 @@ def _do_Dir_test(lpath, path, up_path, sep, fileSys=fs) -> None:
             else:
                 dir_up_path = dir.up().get_internal_path()
 
-            assert dir.name == name, \
-                "dir.name %s != expected name %s" % \
-                (dir.name, name)
-            assert dir.get_internal_path() == path, \
-                "dir.path %s != expected path %s" % \
-                (dir.get_internal_path(), path)
-            assert str(dir) == path, \
-                "str(dir) %s != expected path %s" % \
-                (str(dir), path)
-            assert dir_up_path == up_path, \
-                "dir.up().path %s != expected parent path %s" % \
-                (dir.up().get_internal_path(), up_path)
+            with self.subTest():
+                self.assertEqual(dir.name, name)
+            with self.subTest():
+                self.assertEqual(dir.get_internal_path(), path)
+            with self.subTest():
+                self.assertEqual(str(dir), path)
+            with self.subTest():
+                self.assertEqual(dir_up_path, up_path)
 
         save_os_path = os.path
         save_os_sep = os.sep
         try:
             import ntpath
+
+            # monkeypatch some things to look like Windows and force the
+            # splitdrive stuff in the Node package to re-init; we'll put
+            # it back later if we happened to be on a non-Windows platform.
             os.path = ntpath
             os.sep = '\\'
             SCons.Node.FS.initialize_do_splitdrive()
@@ -1773,6 +1775,16 @@ def _do_Dir_test(lpath, path, up_path, sep, fileSys=fs) -> None:
                 def Dir_test(lpath, path_, up_path_, sep=sep, func=_do_Dir_test):
                     return func(lpath, path_, up_path_, sep)
 
+                # Note that if any of the UNC paths eventually makes it to
+                # Windows, and looks syntactically valid (\\server\share), the
+                # mkdir takes a *long* time to fail with "network path not
+                # found".  If it's not valid syntax, it fails immediately.
+                # Thus, some of these tests can be very slow on the first pass.
+                # Second pass they all "hit" on the Node lookup cache,
+                # so there's no further slowdown. Tried doing this passing
+                # create=False to the Dir() function, but that causes a hard
+                # fail if the dir doesn't already exist, so not right either.
+
                 Dir_test('//foo', '//foo', '//')
                 Dir_test('//foo/bar', '//foo/bar', '//foo')
                 Dir_test('//', '//', '//')