Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Autodesk: Use TfNormPath to fix TfPathUtils test failures on Windows #2476

Conversation

erikaharrison-adsk
Copy link
Contributor

Description of Change(s)

Unit tests were failing on our Windows setup due to path issues. Wrap with TfNormPath.

Fixes Issue(s)

  • N/A
  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link
Contributor

Filed as internal issue #USD-8406

@erikaharrison-adsk erikaharrison-adsk changed the title Use TfNormPath to fix TfPathUtils test failures on Windows Autodesk: Use TfNormPath to fix TfPathUtils test failures on Windows Jun 16, 2023
Copy link
Contributor

@tallytalwar tallytalwar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @erikaharrison-adsk

Thanks for submitting the PR.

Added a note about using "ARCH_OS_WINDOWS" for the test updates as the test fixups are required for windows only.

Also in an internal review, we were wondering if you can provide an example of a path which fails, just in case there are any code improvements we can do to mitigate this specific case?

Thanks

@@ -104,11 +104,11 @@ TestTfRealPath()

if (testSymlinks) {
// Leaf dir is symlink
TF_AXIOM(TfRealPath("d", true) == TfAbsPath("subdir"));
TF_AXIOM(TfNormPath(TfRealPath("d", true)) == TfNormPath(TfAbsPath("subdir")));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the updates here are primarily to fix the test on windows and the failure is not noticed on linux or macOS, it was mentioned in an internal review if we can split and add some ARCH_OS_WINDOWS guards for these updates?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your comments. We'll wrap these changes under ARCH_OS_WINDOWS and add some comments as well.

The reason is that TfRealPath on windows explicitly lower-cases drive letters if the given path contains symlinks, otherwise it returns the same as os.path.abspath. The similar workaround is already used in pxr/base/tf/testenv/testTfPathUtils.py

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Branch updated. @tallytalwar - let us know if this works or if you need any more follow-up.

@pixar-oss pixar-oss merged commit 71b777f into PixarAnimationStudios:dev Sep 19, 2023
34 of 36 checks passed
@erikaharrison-adsk erikaharrison-adsk deleted the adsk/bugfix/TfPathUtils_Tests branch October 12, 2023 02:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants