Fix System.IO.File/Directory property functions with relative paths in -mt mode#13239
Fix System.IO.File/Directory property functions with relative paths in -mt mode#13239JanProvaznik wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes MSBuild property function evaluation in multi-threaded (-mt) builds by resolving relative path arguments for System.IO.File/System.IO.Directory against MSBuild’s thread-local working directory rather than the process-global Environment.CurrentDirectory, aligning behavior with existing Path.GetFullPath interception.
Changes:
- Added
FileUtilities.MakeFullPathFromThreadWorkingDirectoryto resolve relative paths using the thread-local working directory. - Updated
Expander.Function<T>.Execute()to apply thread-local relative-path resolution forSystem.IO.File/System.IO.Directorypath arguments and fixed receiver type comparisons to useSystem.IO.*explicitly. - Added extensive unit tests validating correct resolution behavior (including
..traversal and backslash normalization cases).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Shared/FileUtilities.cs |
Introduces helper to resolve relative paths against CurrentThreadWorkingDirectory. |
src/Build/Evaluation/Expander.cs |
Applies the helper during property function argument assembly and corrects System.IO.* type comparisons. |
src/Build.UnitTests/Evaluation/Expander_Tests.cs |
Adds coverage for relative/absolute path behavior across File/Directory APIs in -mt and non--mt modes. |
| if ((_receiverType == typeof(System.IO.File) || _receiverType == typeof(System.IO.Directory)) | ||
| && IsFileOrDirectoryPathArgument(_methodMethodName, n)) | ||
| { | ||
| argumentValue = FileUtilities.MakeFullPathFromThreadWorkingDirectory(argumentValue); | ||
| } |
There was a problem hiding this comment.
In the System.IO.File/System.IO.Directory path-argument case, argumentValue is already passed through FrameworkFileUtilities.FixFilePath immediately above, but MakeFullPathFromThreadWorkingDirectory currently calls FixFilePath again. Consider avoiding double-normalization here (e.g., have the helper assume fixed input, or only call the helper and do FixFilePath once) to keep property function expansion as cheap as possible.
| private static string ExpandWithThreadWorkingDirectory(string expression, string workingDir, string wrongDir = null) | ||
| { | ||
| string savedThreadDir = FileUtilities.CurrentThreadWorkingDirectory; | ||
| string savedCwd = Environment.CurrentDirectory; | ||
| try | ||
| { | ||
| FileUtilities.CurrentThreadWorkingDirectory = workingDir; | ||
| if (wrongDir != null) | ||
| { | ||
| Environment.CurrentDirectory = wrongDir; | ||
| } | ||
|
|
||
| var pg = new PropertyDictionary<ProjectPropertyInstance>(); | ||
| var expander = new Expander<ProjectPropertyInstance, ProjectItemInstance>(pg, FileSystems.Default); | ||
| return expander.ExpandIntoStringLeaveEscaped(expression, ExpanderOptions.ExpandProperties, MockElementLocation.Instance); | ||
| } | ||
| finally | ||
| { | ||
| FileUtilities.CurrentThreadWorkingDirectory = savedThreadDir; | ||
| Environment.CurrentDirectory = savedCwd; | ||
| } |
There was a problem hiding this comment.
This helper manually saves/restores Environment.CurrentDirectory and FileUtilities.CurrentThreadWorkingDirectory. In this repo, tests are expected to use TestEnvironment to manage state and avoid manual try/finally restore blocks (e.g., use env.SetCurrentDirectory(...) for the working directory). Refactoring this helper to leverage TestEnvironment will make the tests less error-prone and consistent with the test infrastructure.
| public void FileReadAllText_RelativePath_ResolvesFromThreadWorkingDirectory() | ||
| { | ||
| using var env = TestEnvironment.Create(); | ||
| var correctDir = env.CreateFolder(createFolder: true); | ||
| var wrongDir = env.CreateFolder(createFolder: true); |
There was a problem hiding this comment.
New tests here create the test environment via TestEnvironment.Create() without wiring xUnit's ITestOutputHelper. The repository guidance is to inject ITestOutputHelper into the test class and pass it to TestEnvironment.Create(_output) so failures include diagnostic output in CI logs.
…n -mt mode
Property functions like $([System.IO.File]::Exists('relative.txt')) resolved
relative paths against Environment.CurrentDirectory (process-global), which is
wrong in -mt mode where multiple projects build concurrently on different threads.
This change resolves relative path arguments for File/Directory property functions
against FileUtilities.CurrentThreadWorkingDirectory (the thread-local working
directory set by the multithreaded task environment driver).
Changes:
- Add FileUtilities.MakeFullPathFromThreadWorkingDirectory helper that resolves
relative paths using AbsolutePath + NormalizePath, with FixFilePath called before
resolution (critical on Linux where backslash is a valid filename character)
- Add path resolution in Function<T>.Execute() arg assembly loop for File/Directory
receiver types, covering all methods generically (both default-allowed and extended)
- Fix pre-existing bug: type comparisons in arg assembly loop used ypeof(Directory)
which resolves to Microsoft.IO.Directory on net472 due to FEATURE_MSIOREDIST alias,
but _receiverType from AvailableStaticMethods is always System.IO.Directory.
Changed to fully-qualified ypeof(System.IO.Directory) etc.
- Add 38 unit tests covering File methods, Directory methods, extended methods,
absolute path passthrough, multi-path methods, parent traversal, and regular mode
Fixes dotnet#13237
f361efb to
6f927f8
Compare
Summary
Property functions like
FalseandFalseresolved relative paths againstEnvironment.CurrentDirectory(process-global), which produces wrong results in-mtmode where multiple projects build concurrently on different threads sharing the same process.This change resolves relative path arguments against
FileUtilities.CurrentThreadWorkingDirectory(the thread-local working directory set by the multithreaded task environment driver), matching the existing behavior ofPath.GetFullPathwhich was already intercepted inWellKnownFunctions.Changes
src/Shared/FileUtilities.csMakeFullPathFromThreadWorkingDirectory(string path)helper that resolves relative paths usingAbsolutePath+NormalizePath. CallsFixFilePathbefore resolution (critical on Linux where backslash is a valid filename character). Returns path unchanged whenCurrentThreadWorkingDirectoryis null (non-mt mode) or path is already rooted.src/Build/Evaluation/Expander.csIsFileOrDirectoryPathArgument(string methodName, int argIndex)to identify which arguments are file/directory paths (arg0=always, arg1=Copy/Move/Replace, arg2=Replace).Function<T>.Execute()arg assembly loop forSystem.IO.FileandSystem.IO.Directoryreceiver types, afterFixFilePathand beforeUnescapeAll.typeof(Directory)which resolves toMicrosoft.IO.Directoryon net472 due toFEATURE_MSIOREDISTalias, but_receiverTypefromAvailableStaticMethodsis alwaysSystem.IO.Directory. Changed to fully-qualifiedtypeof(System.IO.Directory)/typeof(System.IO.Path).src/Build.UnitTests/Evaluation/Expander_Tests.csMSBUILDENABLEALLPROPERTYFUNCTIONS=1(File.Copy, File.Move, File.Delete, File.*Utc, Directory.CreateDirectory, Directory.Delete, Directory.Move)../) and backslash parent traversal (..\\)Design Notes
WellKnownFunctionsandInvokeMember, so it covers all File/Directory methods generically.Function<T>.Execute()codepath, so they're covered automatically.string.IsNullOrEmpty(CurrentThreadWorkingDirectory)ensures zero behavior change in non-mt modes (multi-process, single-threaded).C:file.txt) are a known limitation —Path.Combineignores the base when the second argument has a drive letter.Fixes #13237