From 20159417ea65215f0db135cdd6a3e739af498ca5 Mon Sep 17 00:00:00 2001 From: pixar-oss Date: Thu, 20 Jun 2024 22:50:17 -0700 Subject: [PATCH] Changing TfNormPath's and TfRealPath's behavior to match python os.normpath and os.realpath's behavior. The decision to lowercase the drive letters on Windows caused inconsistencies when normalizing paths so now these functions will return drive letters with the same case as the input paths. Also updated TfPathUtils tests, UsdNamespaceEditor tests, and added an ARDefaultResolver test. Fixes #3020 (Internal change: 2331818) --- pxr/base/arch/fileSystem.cpp | 7 +---- pxr/base/arch/testenv/testFileSystem.cpp | 6 ++-- pxr/base/tf/pathUtils.cpp | 6 ---- pxr/base/tf/testenv/pathUtils.cpp | 25 ++++------------ pxr/base/tf/testenv/testTfPathUtils.py | 22 ++++---------- pxr/usd/ar/testenv/testArDefaultResolver.py | 29 +++++++++++++++---- pxr/usd/usd/testenv/testUsdNamespaceEditor.py | 2 +- .../testUsdNamespaceEditorProperties.py | 2 +- 8 files changed, 42 insertions(+), 57 deletions(-) diff --git a/pxr/base/arch/fileSystem.cpp b/pxr/base/arch/fileSystem.cpp index 76683443bc..50ce636cb3 100644 --- a/pxr/base/arch/fileSystem.cpp +++ b/pxr/base/arch/fileSystem.cpp @@ -361,15 +361,10 @@ ArchNormPath(const string& inPath, bool stripDriveSpecifier) // Extract the drive specifier. Note that we don't correctly handle // UNC paths or paths that start with \\? (which allow longer paths). - // - // Also make sure drive letters are always lower-case out of ArchNormPath - // on Windows -- this is so that we can be sure we can reliably use the - // paths as keys in tables, etc. string prefix; if (path.size() >= 2 && path[1] == ':') { if (!stripDriveSpecifier) { - prefix.assign(2, ':'); - prefix[0] = std::tolower(path[0]); + prefix = path.substr(0,2); } path.erase(0, 2); } diff --git a/pxr/base/arch/testenv/testFileSystem.cpp b/pxr/base/arch/testenv/testFileSystem.cpp index e48905a7d8..bbd790699d 100644 --- a/pxr/base/arch/testenv/testFileSystem.cpp +++ b/pxr/base/arch/testenv/testFileSystem.cpp @@ -36,8 +36,10 @@ TestArchNormPath() "foo/bar/../../../../../../baz") == "../../../../baz"); #if defined(ARCH_OS_WINDOWS) - ARCH_AXIOM(ArchNormPath("C:\\foo\\bar") == "c:/foo/bar"); - ARCH_AXIOM(ArchNormPath("C:foo\\bar") == "c:foo/bar"); + ARCH_AXIOM(ArchNormPath("C:\\foo\\bar") == "C:/foo/bar"); + ARCH_AXIOM(ArchNormPath("C:foo\\bar") == "C:foo/bar"); + ARCH_AXIOM(ArchNormPath("c:\\foo\\bar") == "c:/foo/bar"); + ARCH_AXIOM(ArchNormPath("c:foo\\bar") == "c:foo/bar"); ARCH_AXIOM(ArchNormPath( "C:\\foo\\bar", /* stripDriveSpecifier = */ true) == "/foo/bar"); ARCH_AXIOM(ArchNormPath( diff --git a/pxr/base/tf/pathUtils.cpp b/pxr/base/tf/pathUtils.cpp index 7529df11ed..a0a902598d 100644 --- a/pxr/base/tf/pathUtils.cpp +++ b/pxr/base/tf/pathUtils.cpp @@ -137,12 +137,6 @@ TfRealPath(string const& path, bool allowInaccessibleSuffix, string* error) } std::string resolved = _ExpandSymlinks(prefix); - // Make sure drive letters are always lower-case out of TfRealPath on - // Windows -- this is so that we can be sure we can reliably use the - // paths as keys in tables, etc. - if (resolved[0] && resolved[1] == ':') { - resolved[0] = tolower(resolved[0]); - } return TfAbsPath(resolved + suffix); #else char resolved[ARCH_PATH_MAX]; diff --git a/pxr/base/tf/testenv/pathUtils.cpp b/pxr/base/tf/testenv/pathUtils.cpp index 77b3f3a973..e95161280e 100644 --- a/pxr/base/tf/testenv/pathUtils.cpp +++ b/pxr/base/tf/testenv/pathUtils.cpp @@ -86,17 +86,6 @@ TestTfRealPath() } if (testSymlinks) { -#if defined(ARCH_OS_WINDOWS) - // On Windows, TfRealPath explicitly lower-cases drive letters if the given path contains symlinks, - // otherwise it returns the same as os.path.abspath. - // This is inconsistent, using TfNormPath to test the current behavior. - // Leaf dir is symlink - TF_AXIOM(TfNormPath(TfRealPath("d", true)) == TfNormPath(TfAbsPath("subdir"))); - // Symlinks through to dir - TF_AXIOM(TfNormPath(TfRealPath("d/e", true)) == TfNormPath(TfAbsPath("subdir/e"))); - // Symlinks through to nonexistent dirs - TF_AXIOM(TfNormPath(TfRealPath("d/e/f/g/h", true)) == TfNormPath(TfAbsPath("subdir/e/f/g/h"))); -#else // Leaf dir is symlink TF_AXIOM(TfRealPath("d", true) == TfAbsPath("subdir")); // Symlinks through to dir @@ -104,8 +93,6 @@ TestTfRealPath() // Symlinks through to nonexistent dirs TF_AXIOM(TfRealPath("d/e/f/g/h", true) == TfAbsPath("subdir/e/f/g/h")); // Symlinks through to broken link -#endif - // Symlinks through to broken link TF_AXIOM(TfRealPath("g", true) == ""); } @@ -125,7 +112,7 @@ TestTfRealPath() } #if defined(ARCH_OS_WINDOWS) - string thisdir = TfNormPath(TfRealPath(".")); + string thisdir = TfRealPath("."); // This directory on Windows should start with a drive letter. TF_AXIOM(thisdir.length() > 2 && thisdir[1] == ':'); // Strip off the drive letter, leaving a path that starts with a slash, @@ -133,7 +120,7 @@ TestTfRealPath() // (because of the "current drive" concept in Windows). string testdir = thisdir; testdir.erase(0, 2); - TF_AXIOM(TfNormPath(TfRealPath(testdir)) == thisdir); + TF_AXIOM(TfRealPath(testdir) == thisdir); if (testSymlinks) { // Call Windows function to change the current working directory to // put us inside a directory that is a symlink. Then validate that @@ -142,17 +129,17 @@ TestTfRealPath() ::SetCurrentDirectory("b"); string thissubdir = thisdir; thissubdir += "/subdir"; - TF_AXIOM(TfNormPath(TfRealPath(".")) == thissubdir); + TF_AXIOM(TfRealPath(".") == thissubdir); ::SetCurrentDirectory(".."); // Then from outside the directory, validate that the more indirect // symlink (d->c->b->subdir) also resolves properly. We can't actually // "cd" to "d", because it isn't configured as a "directory" symlink. string testsubdir = thisdir; testsubdir += "/d"; - TF_AXIOM(TfNormPath(TfRealPath(testsubdir)) == thissubdir); + TF_AXIOM(TfRealPath(testsubdir) == thissubdir); // Test the more direct and indirect symlinks as relative paths. - TF_AXIOM(TfNormPath(TfRealPath("b")) == thissubdir); - TF_AXIOM(TfNormPath(TfRealPath("d")) == thissubdir); + TF_AXIOM(TfRealPath("b") == thissubdir); + TF_AXIOM(TfRealPath("d") == thissubdir); } #endif diff --git a/pxr/base/tf/testenv/testTfPathUtils.py b/pxr/base/tf/testenv/testTfPathUtils.py index 2c34afe98e..647135dd9f 100644 --- a/pxr/base/tf/testenv/testTfPathUtils.py +++ b/pxr/base/tf/testenv/testTfPathUtils.py @@ -38,29 +38,17 @@ def test_TfRealPath(self): if not os.path.islink('g'): os.symlink('f', 'g') - # XXX: - # On Windows, TfRealPath explicitly lower-cases drive letters - # if the given path contains symlinks, otherwise it returns - # the same as os.path.abspath. This is inconsistent but - # fixing it is a bit riskier. So for now, we just test the - # the current behavior. - def _TestSymlink(expected, got): - if platform.system() == 'Windows': - drive, tail = os.path.splitdrive(expected) - expected = drive.lower() + tail - self.assertEqual(expected, got) - self.log.info('leaf dir is symlink') - _TestSymlink(os.path.abspath('subdir'), + self.assertEqual(os.path.abspath('subdir'), Tf.RealPath('d', True)) self.log.info('symlinks through to dir') - _TestSymlink(os.path.abspath('subdir/e'), + self.assertEqual(os.path.abspath('subdir/e'), Tf.RealPath('d/e', True)) self.log.info('symlinks through to nonexistent dirs') - _TestSymlink(os.path.abspath('subdir/e/f/g/h'), + self.assertEqual(os.path.abspath('subdir/e/f/g/h'), Tf.RealPath('d/e/f/g/h', True)) self.log.info('symlinks through to broken link') - _TestSymlink('', Tf.RealPath('g', True)) + self.assertEqual('', Tf.RealPath('g', True)) self.log.info('symlinks through to broken link, ' 'raiseOnError=True') @@ -78,7 +66,7 @@ def _TestSymlink(expected, got): cwd = os.getcwd() try: os.chdir(r'C:/symlink-test-link') - _TestSymlink(os.path.abspath('C:/symlink-test'), + self.assertEqual(os.path.abspath('C:/symlink-test'), Tf.RealPath(r'C:/symlink-test-link')) finally: # Restore cwd before trying to remove the test diff --git a/pxr/usd/ar/testenv/testArDefaultResolver.py b/pxr/usd/ar/testenv/testArDefaultResolver.py index d0557b4f23..31e607a4ed 100644 --- a/pxr/usd/ar/testenv/testArDefaultResolver.py +++ b/pxr/usd/ar/testenv/testArDefaultResolver.py @@ -7,6 +7,7 @@ # import os import pathlib +import sys from pxr import Ar, Tf import unittest @@ -20,11 +21,9 @@ def assertPathsEqual(self, path1, path2): path1 = str(path1) path2 = str(path2) - # Flip backslashes to forward slashes and make sure path case doesn't - # cause test failures to accommodate platform differences. We don't use - # os.path.normpath since that might fix up other differences we'd want - # to catch in these tests. - self.assertEqual(os.path.normcase(path1), os.path.normcase(path2)) + # Flip backslashes to forward slashes to accommodate platform + # differences. + self.assertEqual(os.path.normpath(path1), os.path.normpath(path2)) def _CreateEmptyTestFile(self, path): dir = os.path.dirname(path) @@ -86,6 +85,16 @@ def _RP(path = None): self.assertPathsEqual( '/dir/AbsolutePath.txt', r.CreateIdentifier('/dir/.//AbsolutePath.txt', _RP('subdir/A.txt'))) + + # Windows is case insensitive so ensuring that the drive letter of the + # resulting identifier matches that of the original input path. + if sys.platform == "win32": + self.assertPathsEqual( + 'C:\dir\AbsolutePath.txt', + r.CreateIdentifier('C:/dir/AbsolutePath.txt')) + self.assertPathsEqual( + 'c:\dir\AbsolutePath.txt', + r.CreateIdentifier('c:/dir/AbsolutePath.txt')) # The identifier for a file-relative path (i.e. a relative path # starting with "./" or "../" is obtained by anchoring that path @@ -147,6 +156,16 @@ def _RP(path = None): '/dir/AbsolutePath.txt', r.CreateIdentifierForNewAsset( '/dir/.//AbsolutePath.txt', _RP('subdir/A.txt'))) + + # Windows is case insensitive so ensuring that the drive letter of the + # resulting identifier matches that of the original input path. + if sys.platform == "win32": + self.assertPathsEqual( + 'C:\dir\AbsolutePath.txt', + r.CreateIdentifier('C:/dir/AbsolutePath.txt')) + self.assertPathsEqual( + 'c:\dir\AbsolutePath.txt', + r.CreateIdentifier('c:/dir/AbsolutePath.txt')) # The identifier for a relative path (file-relative or search-relative) # will always be the anchored absolute path. diff --git a/pxr/usd/usd/testenv/testUsdNamespaceEditor.py b/pxr/usd/usd/testenv/testUsdNamespaceEditor.py index a7d4fca76b..bcea9144a8 100644 --- a/pxr/usd/usd/testenv/testUsdNamespaceEditor.py +++ b/pxr/usd/usd/testenv/testUsdNamespaceEditor.py @@ -900,7 +900,7 @@ def _VerifyCannotApplyMovePrimAtPath(path, newPath, expectedMessage): # identifier regardless of platform def getFormattedCwd(): drive, tail = os.path.splitdrive(os.getcwd()) - return drive.lower() + tail.replace('\\', '/') + return drive + tail.replace('\\', '/') # Cannot delete or rename /C (there are no valid reparent targets for # /C on this stage currently regardless of layer permission) diff --git a/pxr/usd/usd/testenv/testUsdNamespaceEditorProperties.py b/pxr/usd/usd/testenv/testUsdNamespaceEditorProperties.py index 7786ca59bb..725857385e 100644 --- a/pxr/usd/usd/testenv/testUsdNamespaceEditorProperties.py +++ b/pxr/usd/usd/testenv/testUsdNamespaceEditorProperties.py @@ -812,7 +812,7 @@ def _VerifyCannotApplyMovePropertyAtPath(path, newPath, expectedMessage): # identifier regardless of platform def getFormattedCwd(): drive, tail = os.path.splitdrive(os.getcwd()) - return drive.lower() + tail.replace('\\', '/') + return drive + tail.replace('\\', '/') uneditableLayerMessage = ( "The spec @{cwd}/basic/sub_1.usda@<{propPath}> cannot be "