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

Add support for long Windows paths #2239

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

mistafunk
Copy link

@mistafunk mistafunk commented Feb 2, 2023

Description of Change(s)

Prefix all file system paths longer than 248 characters with "\\?\" before passing into Win32 File I/O API functions.

Fixes Issue(s)

Avoids errors when reading/writing USD files on long Windows paths.

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

Notes

  • The CLA is pending on my side.
  • This is my first stab at fixing this problem and also my first time contributing larger changes to USD.
  • This PR is not complete yet (Arch and Tf test coverage). I submit it early to get some basic feedback.
  • This PR contains temporary/helper commits not intended for merge.

@mistafunk mistafunk force-pushed the windows-long-paths-support branch 2 times, most recently from b457c34 to 3bee74b Compare February 2, 2023 19:31
@mistafunk
Copy link
Author

Looking into the compile issues...

@meshula
Copy link
Member

meshula commented Feb 3, 2023

Looks promising! Out of curiosity, I noticed you removed the Debug target, which I assume is a temporary development detail. Is there some aspect of Debug that was getting in the way, or is it just a patch to streamline a workflow you have?

@tallytalwar
Copy link
Contributor

Filed as internal issue #USD-7967

@mistafunk
Copy link
Author

Looks promising! Out of curiosity, I noticed you removed the Debug target, which I assume is a temporary development detail. Is there some aspect of Debug that was getting in the way, or is it just a patch to streamline a workflow you have?

Thanks. I saw CMake install errors regarding testArchStackTrace.pdb in non-Release modes. I don't grok these yet, but it seems to be related to the VS generator. Switching to Ninja makes these go away.

Anyways, I'm working on another PR iteration and will trigger a review request once I'm ready.

@erikaharrison-adsk
Copy link
Contributor

Re: testArchStackTrace.pdb, see #2179

@mistafunk
Copy link
Author

mistafunk commented Apr 3, 2023

@meshula I've rebased the PR to latest dev and expanded the test coverage a bit.

Please note, I've yet to backport these changes to our product and test them there. But I consider the changes done.

@mistafunk
Copy link
Author

Before merging, the _ArchHandleLongWindowsPaths also needs to remove dot and dotdot in the paths as well as two (or more) consecutive backslashes.

(Stumbled over this while adding long path support to another library.)

@johnk-realtime
Copy link

Hey guys, just checking on the status of this as it appears the discussion has been inactive for a while. Besides the review is there anything else that's preventing this from moving forwards?

We are experiencing this issue at the moment and considering our options :)

@mistafunk
Copy link
Author

Hi, the ball is with Pixar :-) I'm happy to receive feedback and work some more on this...

@spiffmon
Copy link
Member

@mistafunk , it's taken awhile, but this has, coincidentally, recently finally risen to near top of queue, and we'll be having a look shortly. Can't promise it will make it in to 24.08, but soon.

@@ -648,21 +762,27 @@ MakeUnique(

#endif

namespace {

const std::string TMPDIR_FMT = std::string("%s") + ARCH_PATH_SEP + "%s.XXXXXX";
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this just be a literal? "%s" ARCH_PATH_SEP "%s.XXXXXX" ?

Copy link
Author

Choose a reason for hiding this comment

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

good catch, will turn this into a constexpr const char*

Copy link
Author

Choose a reason for hiding this comment

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

done

@jesschimein
Copy link
Contributor

/AzurePipelines run

Copy link

Pull request contains merge conflicts.

@mistafunk
Copy link
Author

/AzurePipelines run

Copy link

Commenter does not have sufficient privileges for PR 2239 in repo PixarAnimationStudios/OpenUSD

@mistafunk
Copy link
Author

@spiffmon @meshula I've rebased the branch onto latest dev and addressed the one remaining comment as well as made sure the long paths are normalized (removal of dotdot and dot) as per MS specs.

@jesschimein
Copy link
Contributor

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@mistafunk
Copy link
Author

Oops, let me take care of the compile issues on Linux/macOS...

@mistafunk
Copy link
Author

Force pushed the last two commits and fixed Linux build by adding a missing typename (hopefully this takes care of macOS as well... otherwise I'll fire up my macbook in the next few days)

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.

8 participants