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 long path support for Windows #1939

Closed
wants to merge 11 commits into from
Closed

Conversation

majaeger
Copy link

@majaeger majaeger commented Mar 25, 2021

Currently, ninja only works for paths less than MAX_PATH (255) on Windows. This change makes ninja long-path aware by manifesting the application as "longPathAware", and using the unicode version of windows file APIs

See the below docs for more info:
https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation
https://docs.microsoft.com/en-us/dotnet/standard/io/file-path-formats#skip-normalization
https://docs.microsoft.com/en-us/windows/win32/fileio/maximum-file-path-limitation#enable-long-paths-in-windows-10-version-1607-and-later

@majaeger
Copy link
Author

Appears there are still some issues with chdir

@majaeger
Copy link
Author

As far as performance, there seems to be negligible difference, though actually the UNICODE is consistently slightly faster. My guess is this is because of the reduced file path verification, where it lets the file driver do more of the work.

On my machine Win10 20H2, for a small/medium sized project, I see:

ANSI: ~51s
UNICODE: ~49s

@jhasse jhasse added the windows label Mar 28, 2021
@aronyu79
Copy link

aronyu79 commented Jul 7, 2021

Any update on this? Looking forward to it being merged!

@parbo
Copy link

parbo commented Aug 4, 2021

I would also love for this to be merged. Projects at my workplace are affected by this problem.

@jonesmz

This comment was marked as abuse.

@@ -205,6 +205,9 @@ if(BUILD_TESTING)
target_sources(ninja_test PRIVATE src/includes_normalize_test.cc src/msvc_helper_test.cc)
endif()
target_link_libraries(ninja_test PRIVATE libninja libninja-re2c)
if (MSVC AND "$ENV{VSCMD_ARG_TGT_ARCH}" STREQUAL "x86")
target_compile_options(ninja_test PRIVATE "/fsanitize=address")

This comment was marked as abuse.

debug.log Show resolved Hide resolved
WIN32_FILE_ATTRIBUTE_DATA attrs;
if (!GetFileAttributesExA(path.c_str(), GetFileExInfoStandard, &attrs)) {
if (!GetFileAttributesExW(path.c_str(), GetFileExInfoStandard, &attrs)) {

This comment was marked as abuse.

DWORD win_err = GetLastError();
if (win_err == ERROR_FILE_NOT_FOUND || win_err == ERROR_PATH_NOT_FOUND)
return 0;
*err = "GetFileAttributesEx(" + path + "): " + GetLastErrorString();
*err = "GetFileAttributesExW(" + NarrowPath(path) + "): " + GetLastErrorString();

This comment was marked as abuse.

@@ -147,7 +148,7 @@ bool CanonicalizePath(char* path, size_t* len, uint64_t* slash_bits,
return false;
}

const int kMaxPathComponents = 60;
const int kMaxPathComponents = 120;

This comment was marked as abuse.

HANDLE f = ::CreateFileA(path.c_str(), GENERIC_READ, FILE_SHARE_READ, NULL,
OPEN_EXISTING, FILE_FLAG_SEQUENTIAL_SCAN, NULL);

HANDLE f = ::CreateFileW(WidenPath(path).c_str(), GENERIC_READ, FILE_SHARE_READ, NULL,

This comment was marked as abuse.

@javier-martinez-palmer
Copy link

It would be great this is merged and working fine soon. Looking forward to it

@rpiper
Copy link

rpiper commented Aug 12, 2021

Looking forward to this fix as well. I haven't found a way to successfully workaround the long path problem when building with Jenkins.

@jaeger25
Copy link

Hey all, just wanted to give an update that this is still an issue effecting our team, and I plan to come back and work on getting this to a mergeable state near the end of August / start of September.

@trzecieu
Copy link

Hello! Is there any obstacle for merging this pull request? Looks quite useful, especially in the context of using ninja by Android Studio on Windows.

@jhasse
Copy link
Collaborator

jhasse commented Nov 29, 2021

closing in favor of #2056

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants