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
Enable long paths on Tasks #3503
Enable long paths on Tasks #3503
Conversation
src/Shared/NativeMethodsShared.cs
Outdated
var buffer = new StringBuilder(); | ||
|
||
// Try increased buffer sizes if on longpath-enabled Windows | ||
for (int bufferSize = initialSize; hasNoMaxPath && isBufferTooSmall; bufferSize *= 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly I'm not sure if there's a better way to do this with the way the native API is designed. There's no way to get the required buffer size from the function.
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683197(v=vs.85).aspx
src/Shared/NativeMethodsShared.cs
Outdated
@@ -1241,6 +1241,48 @@ internal static int GetParentProcessId(int processId) | |||
return myChildren; | |||
} | |||
|
|||
internal static string GetModuleFileName(string fileName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Noticed this execution path isn't covered at all by tests in StripTypeLibNumberFromPath
, what would be a good way to test this?
src/Tasks/FileState.cs
Outdated
@@ -115,9 +115,8 @@ public FileDirInfo(string filename) | |||
// | |||
// Also, when not under debugger (!) it will give error == 3 for path too long. Make that consistently throw instead. | |||
if ((error == 2 /* ERROR_FILE_NOT_FOUND */|| error == 3 /* ERROR_PATH_NOT_FOUND */) | |||
&& _filename.Length <= NativeMethodsShared.MAX_PATH) | |||
&& _filename.Length <= (int)NativeMethodsShared.OSMaxPathLimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum value, maybe we should just split up OSMaxPathLimit into a bool and int value if the only real case to watch for is Windows + 260.
3cc4230
to
7cfe557
Compare
src/Shared/NativeMethodsShared.cs
Outdated
@@ -48,7 +48,7 @@ internal static class NativeMethodsShared | |||
/// <summary> | |||
/// Default buffer size to use when dealing with the Windows API. | |||
/// </summary> | |||
internal const int MAX_PATH = 260; | |||
private const int MAX_PATH = 260; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd call this WindowsLegacyMaxPath
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually was my original name for it #3406 (comment)
src/Shared/NativeMethodsShared.cs
Outdated
{ | ||
SetOSMaxPathLimit(); | ||
SetMaxPath(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
put the ifdef inside SetMaxPath, to have all initialization logic in one place.
src/Tasks/FileState.cs
Outdated
@@ -101,6 +101,9 @@ public FileDirInfo(string filename) | |||
{ | |||
if (NativeMethodsShared.IsWindows) | |||
{ | |||
// GetFileAttributesEx() will fail with long paths if not normalized | |||
_filename = NativeMethodsShared.HasMaxPath ? _filename : FileUtilities.NormalizePath(_filename); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Judging by the comment, shouldn't the path get normalized only when there are max path limits?
7cfe557
to
1933283
Compare
1933283
to
e435d47
Compare
So this passes locally and I'm not seeing how a change in
|
I agree, so I reran it and it passed, which . . . ok, I guess? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome. I'd like to get #3987 in first, since it's a higher-pri change to ResolveCOMReference which might conflict if we anger the merge, so let's hold off until next week. But exciting!
Closing/reopening to retest after that other PR went in. If it passes I'm good to go on this. |
Slowly working on this whenever there's time. So far this covers 2/3 tasks.