Improved path handling

This commit fixes a bunch of issues at the same time, as they are all
related to path handling:

- `pathlib.Path.resolve()` or `.absolute()` are replaced by
  `bpathlib.make_absolute()`. The latter does NOT follow symlinks and does
  NOT network mounts from a drive letter to UNC notation. This also has
  advantages on non-Windows sytems, as it allows BAT-packing a directory
  structure with symlinked files (such as a Shaman checkout).
- Better handling of drive letters, and of paths that cross drive
  boundaries.
- Better testing of Windows-specific cases when running the tests on
  Windows, and of POSIX-specific cases on other platforms.

Thanks to @wisaac for starting this patch in D6676.

Thanks to @jbakker for pointing out the drive letter issue. This fixes
T70655.

tests/test_bpathlib.py

@@ -1,8 +1,11 @@
-from pathlib import Path, PurePosixPath
+import os
+from pathlib import Path, PurePath, PurePosixPath, PureWindowsPath
+import platform
+import tempfile
 import unittest
 from unittest import mock
 
-from blender_asset_tracer.bpathlib import BlendPath
+from blender_asset_tracer.bpathlib import BlendPath, make_absolute, strip_root
 
 
 class BlendPathTest(unittest.TestCase):
@@ -29,11 +32,11 @@ class BlendPathTest(unittest.TestCase):
         self.assertEqual("BlendPath(b'//some/file.blend')", repr(p))
 
     def test_to_path(self):
-        self.assertEqual(PurePosixPath('/some/file.blend'),
+        self.assertEqual(PurePath('/some/file.blend'),
                          BlendPath(b'/some/file.blend').to_path())
-        self.assertEqual(PurePosixPath('C:/some/file.blend'),
+        self.assertEqual(PurePath('C:/some/file.blend'),
                          BlendPath(b'C:/some/file.blend').to_path())
-        self.assertEqual(PurePosixPath('C:/some/file.blend'),
+        self.assertEqual(PurePath('C:/some/file.blend'),
                          BlendPath(br'C:\some\file.blend').to_path())
 
         with mock.patch('sys.getfilesystemencoding') as mock_getfse:
@@ -41,7 +44,7 @@ class BlendPathTest(unittest.TestCase):
 
             # \xe9 is Latin-1 for é, and BlendPath should revert to using the
             # (mocked) filesystem encoding when decoding as UTF-8 fails.
-            self.assertEqual(PurePosixPath('C:/some/filé.blend'),
+            self.assertEqual(PurePath('C:/some/filé.blend'),
                              BlendPath(b'C:\\some\\fil\xe9.blend').to_path())
 
         with self.assertRaises(ValueError):
@@ -84,7 +87,8 @@ class BlendPathTest(unittest.TestCase):
         self.assertEqual(BlendPath(b'//root/parent.blend'),
                          BlendPath(b'//root/') / b'parent.blend')
 
-    def test_mkrelative(self):
+    @unittest.skipIf(platform.system() == 'Windows', "POSIX paths cannot be used on Windows")
+    def test_mkrelative_posix(self):
         self.assertEqual(b'//asset.png', BlendPath.mkrelative(
             Path('/path/to/asset.png'),
             PurePosixPath('/path/to/bfile.blend'),
@@ -109,3 +113,140 @@ class BlendPathTest(unittest.TestCase):
             Path('/shallow/asset.png'),
             PurePosixPath('/path/to/very/very/very/very/very/deep/bfile.blend'),
         ))
+
+    @unittest.skipIf(platform.system() != 'Windows', "Windows paths cannot be used on POSIX")
+    def test_mkrelative_windows(self):
+        self.assertEqual(b'//asset.png', BlendPath.mkrelative(
+            Path('Q:/path/to/asset.png'),
+            PureWindowsPath('Q:/path/to/bfile.blend'),
+        ))
+        self.assertEqual(b'//to/asset.png', BlendPath.mkrelative(
+            Path('Q:/path/to/asset.png'),
+            PureWindowsPath('Q:/path/bfile.blend'),
+        ))
+        self.assertEqual(b'//../of/asset.png', BlendPath.mkrelative(
+            Path('Q:/path/of/asset.png'),
+            PureWindowsPath('Q:/path/to/bfile.blend'),
+        ))
+        self.assertEqual(b'//../../path/of/asset.png', BlendPath.mkrelative(
+            Path('Q:/path/of/asset.png'),
+            PureWindowsPath('Q:/some/weird/bfile.blend'),
+        ))
+        self.assertEqual(b'//very/very/very/very/very/deep/asset.png', BlendPath.mkrelative(
+            Path('Q:/path/to/very/very/very/very/very/deep/asset.png'),
+            PureWindowsPath('Q:/path/to/bfile.blend'),
+        ))
+        self.assertEqual(b'//../../../../../../../../shallow/asset.png', BlendPath.mkrelative(
+            Path('Q:/shallow/asset.png'),
+            PureWindowsPath('Q:/path/to/very/very/very/very/very/deep/bfile.blend'),
+        ))
+        self.assertEqual(b'D:/path/to/asset.png', BlendPath.mkrelative(
+            Path('D:/path/to/asset.png'),
+            PureWindowsPath('Q:/path/to/bfile.blend'),
+        ))
+
+
+class MakeAbsoluteTest(unittest.TestCase):
+    def test_relative(self):
+        my_dir = Path(__file__).absolute().parent
+        cwd = os.getcwd()
+        try:
+            os.chdir(my_dir)
+            self.assertEqual(my_dir / 'blendfiles/Cube.btx',
+                             make_absolute(Path('blendfiles/Cube.btx')))
+        except Exception:
+            os.chdir(cwd)
+            raise
+
+    @unittest.skipIf(platform.system() != 'Windows', "This test uses drive letters")
+    def test_relative_drive(self):
+        cwd = os.getcwd()
+        my_drive = Path(f'{Path(cwd).drive}/')
+        self.assertEqual(my_drive / 'blendfiles/Cube.btx',
+                         make_absolute(Path('/blendfiles/Cube.btx')))
+
+    def test_drive_letters(self):
+        """PureWindowsPath should be accepted and work well on POSIX systems too."""
+        in_path = PureWindowsPath('R:/wrongroot/oops/../../path/to/a/file')
+        expect_path = Path('R:/path/to/a/file')
+        self.assertNotEqual(expect_path, in_path, 'pathlib should not automatically resolve ../')
+        self.assertEqual(expect_path, make_absolute(in_path))
+
+    @unittest.skipIf(platform.system() == 'Windows', "This test ignores drive letters")
+    def test_dotdot_dotdot_posix(self):
+        in_path = Path('/wrongroot/oops/../../path/to/a/file')
+        expect_path = Path('/path/to/a/file')
+        self.assertNotEqual(expect_path, in_path, 'pathlib should not automatically resolve ../')
+        self.assertEqual(expect_path, make_absolute(in_path))
+
+    @unittest.skipIf(platform.system() != 'Windows', "This test uses drive letters")
+    def test_dotdot_dotdot_windows(self):
+        in_path = Path('Q:/wrongroot/oops/../../path/to/a/file')
+        expect_path = Path('Q:/path/to/a/file')
+        self.assertNotEqual(expect_path, in_path, 'pathlib should not automatically resolve ../')
+        self.assertEqual(expect_path, make_absolute(in_path))
+
+    @unittest.skipIf(platform.system() == 'Windows', "This test ignores drive letters")
+    def test_way_too_many_dotdot_posix(self):
+        in_path = Path('/webroot/../../../../../etc/passwd')
+        expect_path = Path('/etc/passwd')
+        self.assertEqual(expect_path, make_absolute(in_path))
+
+    @unittest.skipIf(platform.system() != 'Windows', "This test uses drive letters")
+    def test_way_too_many_dotdot_windows(self):
+        in_path = Path('G:/webroot/../../../../../etc/passwd')
+        expect_path = Path('G:/etc/passwd')
+        self.assertEqual(expect_path, make_absolute(in_path))
+
+    @unittest.skipIf(platform.system() == 'Windows',
+                     "Symlinks on Windows require Administrator rights")
+    def test_symlinks(self):
+        with tempfile.TemporaryDirectory(suffix="-bat-symlink-test") as tmpdir_str:
+            tmpdir = Path(tmpdir_str)
+
+            orig_path = tmpdir / 'some_file.txt'
+            with orig_path.open('w') as outfile:
+                outfile.write('this file exists now')
+
+            symlink = tmpdir / 'subdir' / 'linked.txt'
+            symlink.parent.mkdir()
+            symlink.symlink_to(orig_path)
+
+            self.assertEqual(symlink, make_absolute(symlink), 'Symlinks should not be resolved')
+
+    @unittest.skipIf(platform.system() != 'Windows',
+                     "Drive letters mapped to network share can only be tested on Windows")
+    @unittest.skip('Mapped drive letter testing should be mocked, but that is hard to do')
+    def test_mapped_drive_letters(self):
+        pass
+
+    def test_path_types(self):
+        platorm_path = type(PurePath())
+        self.assertIsInstance(make_absolute(PureWindowsPath('/some/path')), platorm_path)
+        self.assertIsInstance(make_absolute(PurePosixPath('/some/path')), platorm_path)
+
+
+class StripRootTest(unittest.TestCase):
+    def test_windows_paths(self):
+        self.assertEqual(PurePosixPath(), strip_root(PureWindowsPath()))
+        self.assertEqual(
+            PurePosixPath('C/Program Files/Blender'),
+            strip_root(PureWindowsPath('C:/Program Files/Blender')))
+        self.assertEqual(
+            PurePosixPath('C/Program Files/Blender'),
+            strip_root(PureWindowsPath('C:\\Program Files\\Blender')))
+        self.assertEqual(
+            PurePosixPath('C/Program Files/Blender'),
+            strip_root(PureWindowsPath('C\\Program Files\\Blender')))
+
+    def test_posix_paths(self):
+        self.assertEqual(PurePosixPath(), strip_root(PurePosixPath()))
+        self.assertEqual(
+            PurePosixPath('C/path/to/blender'),
+            strip_root(PurePosixPath('C:/path/to/blender')))
+        self.assertEqual(
+            PurePosixPath('C/path/to/blender'),
+            strip_root(PurePosixPath('C/path/to/blender')))
+        self.assertEqual(
+            PurePosixPath('C/path/to/blender'),
+            strip_root(PurePosixPath('/C/path/to/blender')))

tests/test_pack.py

@@ -1,8 +1,11 @@
 import logging
-import pathlib
-import typing
-
+import os
+import platform
+import shutil
 import tempfile
+import typing
+import unittest
+from pathlib import Path, PurePosixPath
 from unittest import mock
 
 from blender_asset_tracer import blendfile, pack, bpathlib
@@ -23,7 +26,7 @@ class AbstractPackTest(AbstractBlendFileTest):
     def setUp(self):
         super().setUp()
         self.tdir = tempfile.TemporaryDirectory(suffix='-packtest')
-        self.tpath = pathlib.Path(self.tdir.name)
+        self.tpath = Path(self.tdir.name)
 
     def tearDown(self):
         super().tearDown()
@@ -35,10 +38,11 @@ class AbstractPackTest(AbstractBlendFileTest):
                 for path, action in packer._actions.items()
                 if action.rewrites}
 
-    def outside_project(self) -> pathlib.Path:
+    def outside_project(self) -> Path:
         """Return the '_outside_project' path for files in self.blendfiles."""
         # /tmp/target + /workspace/bat/tests/blendfiles → /tmp/target/workspace/bat/tests/blendfiles
-        extpath = pathlib.Path(self.tpath, '_outside_project', *self.blendfiles.parts[1:])
+        # /tmp/target + C:\workspace\bat\tests\blendfiles → /tmp/target/C/workspace/bat/tests/blendfiles
+        extpath = Path(self.tpath, '_outside_project', bpathlib.strip_root(self.blendfiles))
         return extpath
 
 
@@ -89,7 +93,8 @@ class PackTest(AbstractPackTest):
             path = self.blendfiles / fn
             act = packer._actions[path]
             self.assertEqual(pack.PathAction.FIND_NEW_LOCATION, act.path_action, 'for %s' % fn)
-            self.assertEqual(extpath / fn, act.new_path, 'for %s' % fn)
+            self.assertEqual(extpath / fn, act.new_path,
+                             f'\nEXPECT: {extpath / fn}\nACTUAL: {act.new_path}\nfor {fn}')
 
         to_rewrite = (
             'linked_cube.blend',
@@ -162,7 +167,12 @@ class PackTest(AbstractPackTest):
     def test_execute_rewrite(self):
         infile, _ = self._pack_with_rewrite()
 
-        extpath = pathlib.PurePosixPath('//_outside_project', *self.blendfiles.parts[1:])
+        if platform.system() == 'Windows':
+            extpath = PurePosixPath('//_outside_project',
+                                    self.blendfiles.drive[0],
+                                    *self.blendfiles.parts[1:])
+        else:
+            extpath = PurePosixPath('//_outside_project', *self.blendfiles.parts[1:])
         extbpath = bpathlib.BlendPath(extpath)
 
         # Those libraries should be properly rewritten.
@@ -185,6 +195,81 @@ class PackTest(AbstractPackTest):
         packer.close()
         self.assertFalse(packer._rewrite_in.exists())
 
+    @unittest.skipIf(platform.system() == 'Windows',
+                     "Symlinks on Windows require Administrator rights")
+    def test_symlinked_files(self):
+        """Test that symlinks are NOT resolved.
+
+        When packing, an asset that is symlinked should be treated as if it
+        were really at that location. Symlinks should NOT be resolved.
+
+        As a concrete example, a directory structure with only symlinked files
+        in it should still be BAT-packable and produce the same structure.
+        """
+        orig_ppath = self.blendfiles / 'subdir'
+
+        # This is the original structure when packing subdir/doubly_linked_up.blend:
+        #   .
+        #   ├── basic_file.blend
+        #   ├── linked_cube.blend
+        #   ├── material_textures.blend
+        #   ├── subdir
+        #   │   └── doubly_linked_up.blend
+        #   └── textures
+        #       └── Bricks
+        #           ├── brick_dotted_04-bump.jpg
+        #           └── brick_dotted_04-color.jpg
+
+        # This test copies the files to a temporary location and renames them,
+        # then recreates the above structure with symlinks. Packing the symlinks
+        # should be no different than packing the originals.
+
+        orig_paths = [
+            Path('basic_file.blend'),
+            Path('linked_cube.blend'),
+            Path('material_textures.blend'),
+            Path('subdir/doubly_linked_up.blend'),
+            Path('textures/Bricks/brick_dotted_04-bump.jpg'),
+            Path('textures/Bricks/brick_dotted_04-color.jpg'),
+        ]
+
+        import hashlib
+
+        with tempfile.TemporaryDirectory(suffix="-bat-symlink") as tmpdir_str:
+            tmpdir = Path(tmpdir_str)
+
+            real_file_dir = tmpdir / 'real'
+            symlinked_dir = tmpdir / 'symlinked'
+
+            real_file_dir.mkdir()
+            symlinked_dir.mkdir()
+
+            for orig_path in orig_paths:
+                hashed_name = hashlib.new('md5', bytes(orig_path)).hexdigest()
+                # Copy the file to the temporary project, under a hashed name.
+                # This will break Blendfile linking.
+                real_file_path = real_file_dir / hashed_name
+                shutil.copy(self.blendfiles / orig_path, real_file_path)
+
+                # Create a symlink to the above file, in such a way that it
+                # restores the original directory structure, and thus repairs
+                # the Blendfile linking.
+                symlink = symlinked_dir / orig_path
+                symlink.parent.mkdir(parents=True, exist_ok=True)
+                symlink.symlink_to(real_file_path)
+
+            # Pack the symlinked directory structure.
+            pack_dir = tmpdir / 'packed'
+            packer = pack.Packer(self.blendfiles / 'subdir/doubly_linked_up.blend',
+                                 self.blendfiles,
+                                 pack_dir)
+            packer.strategise()
+            packer.execute()
+
+            for orig_path in orig_paths:
+                packed_path = pack_dir / orig_path
+                self.assertTrue(packed_path.exists(), f'{packed_path} should exist')
+
     def _pack_with_rewrite(self):
         ppath = self.blendfiles / 'subdir'
         infile = ppath / 'doubly_linked_up.blend'
@@ -209,9 +294,13 @@ class PackTest(AbstractPackTest):
         seq = ed.get_pointer((b'seqbase', b'first'))
         seq_strip = seq.get_pointer(b'strip')
 
-        imgseq_path = (self.blendfiles / 'imgseq').absolute()
-        as_bytes = str(imgseq_path.relative_to(imgseq_path.anchor)).encode()
+        imgseq_path = bpathlib.make_absolute(self.blendfiles / 'imgseq')
+        print(f'imgseq_path: {imgseq_path!r}')
+        print(f'     anchor: {imgseq_path.anchor!r}')
+        as_bytes = bpathlib.strip_root(imgseq_path).as_posix().encode()
+        print(f'as_bytes: {as_bytes!r}')
         relpath = bpathlib.BlendPath(b'//_outside_project') / as_bytes
+        print(f'relpath: {relpath!r}')
 
         # The image sequence base path should be rewritten.
         self.assertEqual(b'SQ000210.png', seq[b'name'])
@@ -313,7 +402,7 @@ class PackTest(AbstractPackTest):
                             'Expected %s to be compressed' % bpath)
             break
         else:
-            self.fail('Expected to have Blend files in the BAT pack.')
+            self.fail(f'Expected to have Blend files in the BAT pack at {self.tpath}.')
 
         for imgpath in self.tpath.rglob('*.jpg'):
             with imgpath.open('rb') as imgfile:
@@ -322,7 +411,7 @@ class PackTest(AbstractPackTest):
                              'Expected %s to NOT be compressed' % imgpath)
             break
         else:
-            self.fail('Expected to have JPEG files in the BAT pack.')
+            self.fail(f'Expected to have JPEG files in the BAT pack at {self.tpath}.')
 
 
 class ProgressTest(AbstractPackTest):
@@ -485,7 +574,7 @@ class AbortTest(AbstractPackTest):
         packer = pack.Packer(infile, self.blendfiles, self.tpath)
 
         class AbortingCallback(progress.Callback):
-            def trace_blendfile(self, filename: pathlib.Path):
+            def trace_blendfile(self, filename: Path):
                 # Call abort() somewhere during the strategise() call.
                 if filename.name == 'linked_cube.blend':
                     packer.abort()

tests/test_shaman_transfer.py

@@ -18,6 +18,7 @@
 #
 # (c) 2019, Blender Foundation - Sybren A. Stüvel
 import pathlib
+import platform
 
 import responses
 
@@ -27,7 +28,7 @@ from blender_asset_tracer.pack.shaman import transfer
 httpmock = responses.RequestsMock()
 
 
-class AbstractChecksumTest(AbstractBlendFileTest):
+class ShamanTransferTest(AbstractBlendFileTest):
     @classmethod
     def setUpClass(cls):
         super().setUpClass()
@@ -42,8 +43,8 @@ class AbstractChecksumTest(AbstractBlendFileTest):
             cls.test_file2: cls.test_file2.stat().st_size,
         }
         cls.packed_names = {
-            cls.test_file1: pathlib.Path('path/in/pack/test1.blend'),
-            cls.test_file2: pathlib.Path('path/in/pack/test2.blend'),
+            cls.test_file1: pathlib.PurePosixPath('path/in/pack/test1.blend'),
+            cls.test_file2: pathlib.PurePosixPath('path/in/pack/test2.blend'),
         }
 
     def assertValidCheckoutDef(self, definition_file: bytes):
@@ -82,9 +83,10 @@ class AbstractChecksumTest(AbstractBlendFileTest):
 
         trans = transfer.ShamanTransferrer('auth-token', self.blendfiles,
                                            'http://unittest.local:1234/', 'DA-JOB-ID')
+
         trans.start()
-        trans.queue_copy(self.test_file1, pathlib.Path('/') / self.packed_names[self.test_file1])
-        trans.queue_copy(self.test_file2, pathlib.Path('/') / self.packed_names[self.test_file2])
+        trans.queue_copy(self.test_file1, self.packed_names[self.test_file1])
+        trans.queue_copy(self.test_file2, self.packed_names[self.test_file2])
         trans.done_and_join()
 
         self.assertFalse(trans.has_error, trans.error_message())