Consider MT-safe refactor of ManifestUtil library
- Dominant language
- C#
- Stars
- 5.5k
- Forks
- 1.5k
- Avg merge
- 1d 13h
- Merged PRs (30d)
- 133
Description
## Summary
The `ManifestUtil` library (under `src/Tasks/ManifestUtil/`) contains several methods that internally use `Directory.GetCurrentDirectory()` and other process-global APIs. While MSBuild's built-in manifest tasks have been migrated to use `TaskEnvironment` (resolving paths against `ProjectDirectory` instead of CWD), the underlying `ManifestUtil` APIs remain unsafe for multithreaded execution.
Third-party task authors who use these APIs directly and want to migrate to `-mt` mode will encounter incorrect path resolution.
## Context
From [PR #13177](https://github.com/dotnet/msbuild/pull/13177) review discussion between @OvesN and @JanProvaznik:
> **@OvesN**: `Manifest.ResolveFiles()` internally uses `Directory.GetCurrentDirectory()`. If a third-party task author uses this overload and wants to migrate their task to -mt mode, their paths would resolve against the wrong directory. Should we add a doc comment warning that it's not safe for multithreaded execution?
> **@JanProvaznik**: Added comment for now, I think we can consider broader "MT-aware manifest library refactor" but I would rather focus on finalizing tasks rather than this refactor with questionable business value.
## Affected APIs
- `Manifest.ResolveFiles()` (parameterless overload) — uses `Directory.GetCurrentDirectory()` to resolve relative paths ([Manifest.cs:220](https://github.com/dotnet/msbuild/blob/main/src/Tasks/ManifestUtil/Manifest.cs))
- `Util.WriteLog()` — uses a static `StreamWriter` and `logPath` derived from environment at class init time
- Various `ManifestReader`/`ManifestWriter` methods that accept string paths
## Current State
- A doc comment warning has been added to the parameterless `ResolveFiles()` overload.
- All built-in MSBuild tasks using ManifestUtil already pass absolute paths (via `TaskEnvironment.GetAbsolutePath()`), so there is no correctness issue for built-in tasks.
- The only caller of the unsafe parameterless `ResolveFiles()` is `ResolveNativeReference`, which is not migrated to `IMultiThreadableTask` and will be ejected to out-of-proc TaskHost in MT mode.
## Potential Work
- Add `TaskEnvironment`-accepting overloads to key ManifestUtil APIs, use them in our tasks
- Discourage use of old APIs
## Priority
Low — no built-in task is affected, and third-party manifest task migration is uncommon. This is tracked for completeness and to make our static analyzer clean.
Contributor guide
No contributing guide indexed for this repository
Assessment
This issue has not been assessed yet.