Skip to content

Commit

Permalink
Changing TfNormPath's and TfRealPath's behavior to match python os.no…
Browse files Browse the repository at this point in the history
…rmpath 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)
  • Loading branch information
pixar-oss committed Jun 28, 2024
1 parent 1cba8a9 commit 2015941
Show file tree
Hide file tree
Showing 8 changed files with 42 additions and 57 deletions.
7 changes: 1 addition & 6 deletions pxr/base/arch/fileSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
6 changes: 4 additions & 2 deletions pxr/base/arch/testenv/testFileSystem.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
6 changes: 0 additions & 6 deletions pxr/base/tf/pathUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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];
Expand Down
25 changes: 6 additions & 19 deletions pxr/base/tf/testenv/pathUtils.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -86,26 +86,13 @@ 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
TF_AXIOM(TfRealPath("d/e", true) == TfAbsPath("subdir/e"));
// 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) == "");
}

Expand All @@ -125,15 +112,15 @@ 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,
// but is still a valid absolute path equivalent to this directory
// (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
Expand All @@ -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

Expand Down
22 changes: 5 additions & 17 deletions pxr/base/tf/testenv/testTfPathUtils.py
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand All @@ -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
Expand Down
29 changes: 24 additions & 5 deletions pxr/usd/ar/testenv/testArDefaultResolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#
import os
import pathlib
import sys
from pxr import Ar, Tf

import unittest
Expand All @@ -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)
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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.
Expand Down
2 changes: 1 addition & 1 deletion pxr/usd/usd/testenv/testUsdNamespaceEditor.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion pxr/usd/usd/testenv/testUsdNamespaceEditorProperties.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 "
Expand Down

0 comments on commit 2015941

Please sign in to comment.