Use Microsoft.IO.Redist in Framework FileUtilities/NativeMethods on net472 (#13078)#13428
Use Microsoft.IO.Redist in Framework FileUtilities/NativeMethods on net472 (#13078)#13428sachinsharma3191 wants to merge 6 commits intodotnet:mainfrom
Conversation
|
@dotnet-policy-service agree |
b5b1ffe to
72bfc27
Compare
…3078) Add Microsoft.IO.Redist to Microsoft.Build.Framework for net4* targets. FileUtilities.GetFullPath and NativeMethods.GetCurrentDirectory use Microsoft.IO.Path and Microsoft.IO.Directory when FEATURE_MSIOREDIST is set. Legacy Win32 GetCurrentDirectory/GetFullPathName are not enabled by default: the FEATURE_LEGACY_* compile constants are preserved only as comments in Directory.BeforeCommon.targets, while the DllImports and helpers remain available behind FEATURE_LEGACY_* preprocessor guards for opt-in builds. Adjust Framework unit tests with a NETFRAMEWORK gate for path-too-long expander cases.
72bfc27 to
96d5dfb
Compare
When Path.GetFullPath throws PathTooLongException, rethrow instead of falling back to Microsoft.IO.Path.GetFullPath. The latter may not throw for the same path, causing Regress451057_ExitGracefullyIfPathNameIsTooLong tests to fail (they expect the task to return false, not succeed with a long path). Made-with: Cursor
…ths as invalid - Paths with leading/trailing whitespace: Microsoft.IO.Path.GetFullPath may trim them, causing incorrect resolution. Treat as invalid (CanNotResolveHintPathWithSpace). - Paths >= 260 chars: GetFullPath throws PathTooLongException on legacy Windows. Treat as invalid so callers skip NormalizePath and handle gracefully (Regress314573_VeryLongPaths). Made-with: Cursor
b4db326 to
ccc36bd
Compare
| catch (PathTooLongException) | ||
| { | ||
| // Trigger the same exception for truly invalid characters even if path is long | ||
| if (path.Contains('|')) |
There was a problem hiding this comment.
Is there a reason only "|" is handled? I believe there are multiple invalid characters.
|
|
||
| // Paths that exceed MAX_PATH (260) will cause GetFullPath to throw PathTooLongException on | ||
| // legacy Windows. Treat as invalid so callers skip NormalizePath and handle gracefully (e.g. RAR Regress314573). | ||
| if (path.Length >= NativeMethods.MAX_PATH) |
There was a problem hiding this comment.
PathIsInvalid usage is not limited to normalization. I am not yet sure about marking paths with trailing whitespaces as invalid, but marking all long paths as invalid may cause build failures on systems where long paths are allowed.
| <DefineConstants>$(DefineConstants);FEATURE_INSTALLED_MSBUILD</DefineConstants> | ||
| <!-- Directory.GetCurrentDirectory The pre .Net 4.6.2 implementation of Directory.GetCurrentDirectory is slow and creates strings in its work. --> | ||
| <DefineConstants>$(DefineConstants);FEATURE_LEGACY_GETCURRENTDIRECTORY</DefineConstants> | ||
| <!-- <DefineConstants>$(DefineConstants);FEATURE_LEGACY_GETCURRENTDIRECTORY</DefineConstants> --> |
There was a problem hiding this comment.
nit: better remove the line alltogether.
| This check can only be properly done after normalizing, so | ||
| \\foo\.. will be properly rejected. Also, reject \\?\GLOBALROOT\ | ||
| (an internal kernel path) because it provides aliases for drives. | ||
| string redistResult = NewPath.GetFullPath(path); |
There was a problem hiding this comment.
Since exceptions are significantly more expensive than simple branching, relying on the ArgumentException fallback for many paths could noticeably slow down builds. It would be better to avoid using exceptions for control flow if we can.
|
|
||
| <Error Condition="!Exists('$(TlbExpPath)')" | ||
| Text="TlbExp was not found at '$(TlbExpPath)'. Ensure the .NET Framework SDK tools are installed." /> | ||
| <Warning Condition="!Exists('$(TlbExpPath)')" |
There was a problem hiding this comment.
Could you explain why this needs to be downgraded from an error to a warning? I’m not sure how the change to this file relates to the changes in this PR.
|
|
||
| throw new ArgumentException(Environment.GetResourceString("Arg_PathIllegalUNC")); | ||
| // Re-validate UNC roots that Redist might accept but MSBuild tests expect to fail. | ||
| if (redistResult.StartsWith(@"\\", StringComparison.Ordinal) && (redistResult is @"\\" or @"\\\\" or @"\\localhost" or @"\\XXX\")) |
There was a problem hiding this comment.
It seems UNC paths handling changed significantly. Can you explain those changes in detail? Are there any behavioral changes compared to the previous version?
| #endif | ||
| } | ||
|
|
||
| #if FEATURE_LEGACY_GETCURRENTDIRECTORY |
There was a problem hiding this comment.
I thought that we are removing FEATURE_LEGACY_GETCURRENTDIRECTORY from project files. Do we still have this code path enabled anywhere?
|
I intentionally left it for later when moving FileUtilities to Microsoft.Build.Framework, so I'm glad to see this in the works! Note: There are a lot of tests that will fail because they expect exceptions to be thrown on .NET Framework. Microsoft.IO.Redist doesn't do all of the path validation that was done by System.IO.Path on full framework. Instead, it matches the behavior on modern .NET. So, I would expect there to be significant test updates for those tests that expect thrown exceptions on .NET Framework. cc @JeremyKuhne (the Microsoft.IO.Redist author/guru) in case he has any thoughts. |
If there are any questions or issues feel free to tag me when they arise. |
Summary
Addresses #13078 by replacing custom Win32
GetFullPath/GetCurrentDirectorypaths inMicrosoft.Build.Frameworkwith Microsoft.IO.Redist whenFEATURE_MSIOREDISTis enabled (net472).Changes
FEATURE_LEGACY_GETCURRENTDIRECTORYandFEATURE_LEGACY_GETFULLPATHfromDirectory.BeforeCommon.targetsfor all net4* targets.FileUtilities.GetFullPath: useNewPath.GetFullPathwhenFEATURE_MSIOREDIST; otherwiseSystem.IO.Path.GetFullPath.NativeMethods.GetCurrentDirectory: useMicrosoft.IO.Directory.GetCurrentDirectory()whenFEATURE_MSIOREDIST; remove unused kernel32 imports and Win32 helpers removed with the legacy path code.Microsoft.IO.Redistpackage reference toMicrosoft.Build.Framework.csproj(conditional onFeatureMSIORedist, consistent with other projects).FileUtilities_Testsexpectation aligned with non-legacy behavior; Expander path-too-long cases gated with#if NETFRAMEWORK.Notes
MSBuildTaskHost(net35) keeps its own utilities and is unchanged.Microsoft.Build.Frameworkbuilds fornet10.0/netstandard2.0locally; fullnet472+ tests should be run on Windows per repo guidance.Fixes #13078