Skip to content

Improve ArchGetFileName for windows to return full path #3361

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.

Sign up for GitHub

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

Merged
pixar-oss merged 3 commits into from
Dec 12, 2024
Merged

Improve ArchGetFileName for windows to return full path #3361

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3b41ea1
Improve ArchGetFileName to return full path for Windows platform
roggiezhang-nv Nov 20, 2024
b2baa52
Handle long paths that are over MAX_PATH on Windows
roggiezhang-nv Dec 4, 2024
19201b2
Use std::filesystem::canonical to normalize path
roggiezhang-nv Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 23 additions & 11 deletions pxr/base/arch/fileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <cstdio>
#include <cstdlib>
#include <cerrno>
#include <filesystem>
#include <memory>
#include <utility>

Expand Down Expand Up @@ -532,24 +533,35 @@ ArchGetFileName(FILE *file)
}
return result;
#elif defined (ARCH_OS_WINDOWS)
static constexpr DWORD bufSize =
sizeof(FILE_NAME_INFO) + sizeof(WCHAR) * 4096;
HANDLE hfile = _FileToWinHANDLE(file);
auto fileNameInfo = reinterpret_cast<PFILE_NAME_INFO>(malloc(bufSize));
string result;
if (GetFileInformationByHandleEx(
hfile, FileNameInfo, static_cast<void *>(fileNameInfo), bufSize)) {
std::vector<WCHAR> filePath(MAX_PATH);
HANDLE hfile = _FileToWinHANDLE(file);
DWORD dwSize = GetFinalPathNameByHandleW(hfile, filePath.data(), MAX_PATH, VOLUME_NAME_DOS);
// * dwSize == 0. Fail.
// * dwSize < MAX_PATH. Success, and dwSize returns the size without null terminator.
// * dwSize >= MAX_PATH. Buffer is too small, and dwSize returns the size with null terminator.
if (dwSize >= MAX_PATH) {
filePath.resize(dwSize);
dwSize = GetFinalPathNameByHandleW(hfile, filePath.data(), dwSize, VOLUME_NAME_DOS);
}

if (dwSize != 0) {
size_t outSize = WideCharToMultiByte(
CP_UTF8, 0, fileNameInfo->FileName,
fileNameInfo->FileNameLength/sizeof(WCHAR),
CP_UTF8, 0, filePath.data(),
dwSize,
NULL, 0, NULL, NULL);
result.resize(outSize);
WideCharToMultiByte(
CP_UTF8, 0, fileNameInfo->FileName,
fileNameInfo->FileNameLength/sizeof(WCHAR),
CP_UTF8, 0, filePath.data(),
-1,
&result.front(), outSize, NULL, NULL);

// Strip path prefix if necessary.
// See https://learn.microsoft.com/en-us/dotnet/standard/io/file-path-formats
// for format of DOS device paths.
auto canonicalPath = std::filesystem::canonical(result);
result = canonicalPath.string();
Comment on lines +558 to +563
Copy link
Contributor

@anwang2009 anwang2009 Dec 5, 2024

Choose a reason for hiding this comment

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

We should actually consider removing this entire block. According to this post from three years ago std::filesystem::canonical is identical to GetFinalPathNameByHandleW. Have you found differently? If not, we should just be able to return result directly instead of trying to canonicalize it again.

One thing to also note is that GetFinalPathNameByHandleW by default (with VOLUME_NAME_DOS) includes prefixes by default, and therefore so does std::filesystem::canonical, if the above post is correct. So the prefixes are not stripped in this current implementation, which seems fine as long as downstream use cases are handled correctly. Why did you want prefixes stripped originally?

Copy link
Contributor Author

@roggiezhang-nv roggiezhang-nv Dec 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@anwang2009 We just thought we should return a canonical path.

Not sure what you mean that std::filesystem::canonical is identical to GetFinalPathNameByHandleW. std::filesystem::canonical will strip prefixes if necessary to return a canonical path. For linux paths, they are the same, but not for windows paths.

\?\C:\test_files\main.usd <---- before std::filesystem::canonical is called.
C:\test_files\main.usd <---- after std::filesystem::canonical is called.

Copy link
Contributor

@anwang2009 anwang2009 Dec 6, 2024

Choose a reason for hiding this comment

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

I see, the post must be out of date. Thanks @roggiezhang-nv !

}
free(fileNameInfo);
return result;
#else
#error Unknown system architecture
Expand Down
7 changes: 7 additions & 0 deletions pxr/base/arch/testenv/testFileSystem.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <cstdio>
#include <cstdlib>
#include <cstring>
#include <filesystem>

PXR_NAMESPACE_USING_DIRECTIVE

Expand Down Expand Up @@ -92,6 +93,12 @@ int main()
fclose(firstFile);
ARCH_AXIOM(ArchGetFileLength(firstName.c_str()) == strlen(testContent));

// Open a file, check that the file path from FILE* handle is matched.
ARCH_AXIOM((firstFile = ArchOpenFile(firstName.c_str(), "rb")) != NULL);
std::string filePath = ArchGetFileName(firstFile);
ARCH_AXIOM(std::filesystem::equivalent(filePath, firstName));
fclose(firstFile);

// Map the file and assert the bytes are what we expect they are.
ARCH_AXIOM((firstFile = ArchOpenFile(firstName.c_str(), "rb")) != NULL);
ArchConstFileMapping cfm = ArchMapFileReadOnly(firstFile);
Expand Down