Fix working directory of taskhosts launched in -mt mode#12807
Fix working directory of taskhosts launched in -mt mode#12807JanProvaznik wants to merge 1 commit intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug where task hosts launched in multi-threaded mode (-mt) were using the current working directory instead of the project's directory as their working directory. This caused tasks that rely on the current working directory to fail. The fix propagates the project file path to the task host so it can set the correct working directory in multi-threaded builds.
Key Changes
- Adds logic to set the task host's startup directory to the project's directory in multi-threaded mode
- Propagates the project file path through the task instantiation pipeline
- Updates all call sites to pass the project file path parameter
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Build/Instance/TaskFactories/TaskHostTask.cs | Adds field to store project file path, new GetStartupDirectory method that returns project directory in multi-threaded mode, and uses it when creating task host configuration |
| src/Build/Instance/TaskFactories/AssemblyTaskFactory.cs | Updates CreateTaskInstance signature to accept project file path and passes it to TaskHostTask constructor |
| src/Build/BackEnd/TaskExecutionHost/TaskExecutionHost.cs | Passes _projectInstance.FullPath to task factory methods at both call sites |
| src/Build.UnitTests/BackEnd/TaskHostTask_Tests.cs | Adds new test file with unit tests for GetStartupDirectory method covering various scenarios |
| src/Build.UnitTests/BackEnd/AssemblyTaskFactory_Tests.cs | Updates all test calls to CreateTaskInstance to pass the new string.Empty parameter |
| catch (Exception) | ||
| { | ||
| // If Path.GetDirectoryName fails, we'll fall back to GetCurrentDirectory | ||
| } |
There was a problem hiding this comment.
The catch block is too broad and swallows all exceptions without logging or specificity. Consider catching only specific exceptions that Path.GetDirectoryName can throw (e.g., ArgumentException, PathTooLongException) to avoid masking unexpected errors. At minimum, add a comment explaining why all exceptions are acceptable to swallow here.
| catch (Exception) | |
| { | |
| // If Path.GetDirectoryName fails, we'll fall back to GetCurrentDirectory | |
| } | |
| catch (ArgumentException) | |
| { | |
| // If Path.GetDirectoryName fails due to invalid path, we'll fall back to GetCurrentDirectory | |
| } | |
| catch (PathTooLongException) | |
| { | |
| // If Path.GetDirectoryName fails due to path being too long, we'll fall back to GetCurrentDirectory | |
| } | |
| catch (NotSupportedException) | |
| { | |
| // If Path.GetDirectoryName fails due to an invalid format, we'll fall back to GetCurrentDirectory | |
| } |
| [Fact] | ||
| public void GetStartupDirectory_ReturnsProjectDirectory_WhenMultiThreadedAndProjectFileExists() | ||
| { | ||
| string projectFile = Path.Combine("C:", "MyProject", "Project.csproj"); |
There was a problem hiding this comment.
The hardcoded path Path.Combine("C:", "MyProject", "Project.csproj") will fail on non-Windows platforms. Consider using a platform-agnostic approach similar to other tests in the codebase that use NativeMethodsShared.IsWindows to conditionally construct paths, or use Path.GetTempPath() or relative paths for test paths.
| [Fact] | ||
| public void GetStartupDirectory_ReturnsCurrentDirectory_WhenNotMultiThreaded() | ||
| { | ||
| string projectFile = Path.Combine("C:", "MyProject", "Project.csproj"); |
There was a problem hiding this comment.
The hardcoded path Path.Combine("C:", "MyProject", "Project.csproj") will fail on non-Windows platforms. Consider using a platform-agnostic approach similar to other tests in the codebase that use NativeMethodsShared.IsWindows to conditionally construct paths, or use Path.GetTempPath() or relative paths for test paths.
| /// <summary> | ||
| /// The project file of the task node. | ||
| /// </summary> | ||
| private string _projectFileOfTaskNode; |
There was a problem hiding this comment.
Field '_projectFileOfTaskNode' can be 'readonly'.
| private string _projectFileOfTaskNode; | |
| private readonly string _projectFileOfTaskNode; |
|
alternative design is probably better but relies of things that are not merged, so drafting for now |
|
fixed in #12842 |
Fixes #12802
Context
in -mt mode taskhosts are spawned with currentworkingdirectory, which may not mirror behavior in multiprocess build - a node has cwd = directory where the project file being built is, leading to task failures for tasks which rely on cwd being set correctly
Changes Made
propagate information which project are building to TaskHostTask
Testing
updated unit tests,
manually validated it fixes the repro project
Notes
alternative design: once taskenvironemtn apis are merged
567e44b