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

Fixes to handling of Windows symbolic links and mount points #1378

Conversation

marktucker
Copy link
Contributor

Description of Change(s)

Two fixes to low level handling of symbolic links and mount points on Windows. One handles Windows "Junction" mount points the same way symbolic links are currently handled (junctions are able to span between local drives, whereas symbolic links can only point to the same drive). The other change causes Tf_HasAttribute in tf/fileUtils.cpp to treat paths with trailing slashes the same way lstat handles them on Linux, which is that is resolves the link (even though the same path without the trailing slash would cause lstat to return information about the link itself, not the destination of the link).

I'm not sure how to submit a regression test for this fix since any such test would require setting up a mount point between two drives on a Windows machine (which requires administrative priveleges and knowing that two separate drives exist onthe machine).

Fixes Issue(s)

…reparse

points. Without this, USD was mangling any path on Windows that included a
"mount point" (or junction) directory entry in the path.
…ath resolves

the symlink (https://man7.org/linux/man-pages/man7/path_resolution.7.html).
This same behavior needs to be provided explicitly on Windows in the
Tf_HasAttribute method when resolveSymlinks is false or TfIsDir will return
false when passed "C:/foo/bar/" if "C:/foo/bar" is a mount point. On Linux
this situation would return true.
@gonewest818
Copy link

See also #1229

@marktucker
Copy link
Contributor Author

marktucker commented Nov 1, 2020

I wasn't aware of that commit. But I see several issues with it. For one, it doesn't handle the "trailing slash on a directory should resolve the symlink" issue. It also requires running additional tests (which aren't necessary in this PR). In general I think #1229 adds more complications to the logic to make the higher level functions work on Windows in more cases, whereas this PR attempts to make the existing lowest level Windows methods simply match the Linux low level method behavior, which corrects the high level behavior without any additional logic.

The test code looks like something I could adopt into this PR. It might be worth making this a platform-agnostic test too, where only the "system" command differs between Windows and other platforms... I'll try adding such a test.

prior to the recent mount point and TF_HasAttributes changes, but passes
with these changes (note that the test must be run with adminstrative
priveleges on Windows or the testSymlink global will be false, skipping
this test entirely). This test was largely copied from
PixarAnimationStudios#1229, with a
few additions, and making it run under Windows and non-Windows platforms.
@jilliene
Copy link

jilliene commented Nov 3, 2020

Filed as internal issue #USD-6463

@DougRogers-DigitalFish
Copy link

DougRogers-DigitalFish commented Nov 3, 2020

I think the low level functions mentioned in #1229 should be updated as well. They are used in other parts for the code.
ArchReadLink should be updated as well, but think there is a more straightforward way to implement it, though. Forcing the symbolic evaluation seems seems fine if Linux systems work that way. I need to do some more testing, but I should have something to evaluate very soon.

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.

6 participants