Support long module path

[nxtn]

The "\?" prefix which indicates it's a long path should not be stripped.

[jkotas]

The "?" prefix which indicates it's a long path should not be stripped.

Why not?

I believe that our general strategy is to strip and append this prefix around the Win32 API calls, so that the rest of the code does not need to deal with it.

cc @JeremyKuhne

[nxtn]

I believe that our general strategy is to strip and append this prefix around the Win32 API calls, so that the rest of the code does not need to deal with it.

OK. I'm fine with that.

[stephentoub]

This PR is now purely about using ArrayPool, and the PR/commit title and description are stale, correct?

[nxtn]

This PR is now purely about using ArrayPool, and the PR/commit title and description are stale, correct?

The existing implementation doesn't support paths longer than 1024 characters.

[stephentoub]

Ok. Can you please add a test for this?

[danmoseley]

what is the purpose of toReturn - can't you do:

                               ArrayPool<char>.Shared.Return(chars);
                                minimumLength = Math.Min(minimumLength * 2, short.MaxValue);
                                chars = ArrayPool<char>.Shared.Rent(minimumLength);

[jkotas]

@danmosemsft This would call Return multiple timeswhenRent` throws an exception.

It would be fine to write it this way if the Return is not inside finally block (ie we let GC take care of collecting the buffer when exception is thrown).

[danmoseley]

ah yes - tricky. might be worth a comment.

[nxtn]

@stephentoub I have no idea why the test failed. It passed on my machine. Could you please have a look?

[stephentoub]

I have no idea why the test failed. It passed on my machine. Could you please have a look?

It appears you just pushed a new commit, so any old failures are no longer showing up.

[stephentoub]

This is a failure I see from a previous commit:

System.DllNotFoundException : Unable to load DLL 'C:\Windows\TEMP\ProcessModuleTests_fxjjvqiv.gvd\TestModuleLongPath_38_dcf0b318\__________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________.dll' or one of its dependencies: The specified module could not be found. (0x8007007E)


Stack trace
   at System.Runtime.InteropServices.NativeLibrary.LoadFromPath(String libraryName, Boolean throwOnError)
   at System.Runtime.InteropServices.NativeLibrary.Load(String libraryPath) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs:line 45
   at System.Diagnostics.Tests.ProcessModuleTests.TestModuleLongPath() in /_/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs:line 44

[nxtn]

This is a failure I see from a previous commit:

It reappeared. I added a File.Exists check to ensure the file does exist. Does NativeLibrary.Load require a \\?\ prefix?

[eiriktsarpalis]

@danmosemsft This PR has stagnated for a while. Do you think we should try to get it merged for 5.0?

[danmoseley]

@eiriktsarpalis I do not think it is worth rushing over the next few days without evidence of widespread impact. After that, I hope we can work to complete this PR one way or another, like any other community PR.

[nxtn]

Hi there, I need some suggestions to complete the test. I couldn't make the symbolic link work here. WDYT about copying an arbitrary system library to a temporary long path and loading it for testing?

[jeffhandley]

@adamsitnik @eiriktsarpalis Do you have any suggestions for the question about how to test this?

[adamsitnik]

WDYT about copying an arbitrary system library to a temporary long path and loading it for testing?

I am OK with that.

@NextTurn please let me know if you have time to work on this, if not we can finish what you have started. Thank you!

[nxtn]

It's succeeding on Windows x64, but on Windows x86 it gives me this:

System.DllNotFoundException : Unable to load DLL 'C:\h\w\A86F08AE\t\ProcessModuleTests_5mkc4e32.nw0\TestModuleLongPath_70_f9ba421c\__________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________________.dll' or one of its dependencies: The data area passed to a system call is too small. (0x8007007A)


Stack trace
   at System.Runtime.InteropServices.NativeLibrary.LoadFromPath(String libraryName, Boolean throwOnError)
   at System.Runtime.InteropServices.NativeLibrary.Load(String libraryPath) in /_/src/libraries/System.Private.CoreLib/src/System/Runtime/InteropServices/NativeLibrary.cs:line 44
   at System.Diagnostics.Tests.ProcessModuleTests.TestModuleLongPath() in /_/src/libraries/System.Diagnostics.Process/tests/ProcessModuleTests.cs:line 76

[adamsitnik]

@steveharter @GrabYourPitchforks (as you are the owners of System.Reflection.Emit) I remember that Full .NET Framework offered a way to dynamically generate, save to disk, and load from disk a managed assembly. Is this still supported?

We need to test the long process module paths and our current best idea is to dynamically load such an assembly to the test runner process.

[stephentoub]

dynamically generate, save to disk [...] Is this still supported?

No.
#15704

[adamsitnik]

@stephentoub do you know if we have any existing test that we could just extend? Like something that validates that a .NET executable with path > MAX_PATH can just run for example?

I was thinking about creating a new standalone .exe with just a very long name and asserting the module inside. Something like the standalone coreclr regression tests: https://github.com/dotnet/runtime/tree/master/src/tests/Regressions/coreclr

My other idea is to... add a new project with a very long output dll name to the System.Diagnostics.sln, reference it from this tests project, and load it dynamically.

I am not sure if this would work if we run our test on machines where long path support is disabled on purpose. In such a case we would have to give it a short name, and after detecting that it's fine (conditional fact), rename the file and then load it. Any thoughts on that?

[danmoseley]

Most or all of our developers have long paths enabled (since it is/was documented as a requirement to build the repo) so I think if the test simply checks for it being enabled that makes sense to me and it would still run fairly often.

[adamsitnik]

I've sent a proposal #46685

[jeffhandley]

@nxtn / @adamsitnik -- I wasn't sure between the two of you who was going to try to carry this forward. @nxtn, are you blocked by the test failure? Thanks!

[jeffhandley]

@nxtn / @adamsitnik -- I wasn't sure between the two of you who was going to try to carry this forward. @nxtn, are you blocked by the test failure? Thanks!

Ping on this @nxtn. Please let us know if you'd like help moving this forward. Thanks!

[danmoseley]

I'm going to close this PR as stalled. @nxtn thank you for your work, and feel free to reopen if you wish to pick this up.

[adamsitnik]

@nxtn once again big thanks for your help. I've fixed the tests and merged your changes in #57335. They are going to be part of .NET 6