Enlighten GenerateTrustInfo for multithreaded mode#13622
Enlighten GenerateTrustInfo for multithreaded mode#13622jankratochvilcz wants to merge 2 commits intomainfrom
Conversation
Add [MSBuildMultiThreadableTask] to both conditional compilation variants. NETFRAMEWORK version: implement IMultiThreadableTask and absolutize BaseManifest and TrustInfoFile paths before file I/O. Non-Framework version: attribute-only (stub that returns false). Fixes #13619 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR updates the built-in GenerateTrustInfo task to participate in MSBuild’s multithreaded task execution model by marking both conditional-compilation implementations as multithread-safe and ensuring NETFRAMEWORK file I/O uses task-isolated absolute paths.
Changes:
- Added
[MSBuildMultiThreadableTask]to both NETFRAMEWORK and non-NETFRAMEWORKGenerateTrustInfoimplementations. - Implemented
IMultiThreadableTaskfor the NETFRAMEWORK implementation and routedBaseManifest/TrustInfoFilefile access throughTaskEnvironment.GetAbsolutePath(...).
| if (BaseManifest != null) | ||
| { | ||
| try | ||
| { | ||
| trustInfo.ReadManifest(BaseManifest.ItemSpec); | ||
| } | ||
| catch (Exception ex) | ||
| AbsolutePath baseManifestPath = TaskEnvironment.GetAbsolutePath(BaseManifest.ItemSpec); | ||
| if (FileSystems.Default.FileExists(baseManifestPath)) | ||
| { |
There was a problem hiding this comment.
TaskEnvironment.GetAbsolutePath(BaseManifest.ItemSpec) will throw ArgumentException when BaseManifest.ItemSpec is null/empty, whereas the previous File.Exists check would simply treat it as non-existent. To avoid a behavior regression/crash, guard with !string.IsNullOrEmpty(BaseManifest.ItemSpec) (or handle the exception and log an error) before calling GetAbsolutePath.
There was a problem hiding this comment.
Good catch — that's Sin 6 (exception type change). Fixed in the latest commit by adding a string.IsNullOrEmpty guard before GetAbsolutePath, preserving the old silent no-op semantics for empty BaseManifest.ItemSpec.
| // Write trust-info back to a stand-alone trust file | ||
| trustInfo.Write(TrustInfoFile.ItemSpec); | ||
| AbsolutePath trustInfoFilePath = TaskEnvironment.GetAbsolutePath(TrustInfoFile.ItemSpec); | ||
| trustInfo.Write(trustInfoFilePath); |
There was a problem hiding this comment.
This change is specifically meant to make BaseManifest/TrustInfoFile path handling deterministic under multithreaded task isolation. There don’t appear to be unit tests covering GenerateTrustInfo with a non-default TaskEnvironment (e.g., project directory different from process CWD); adding one would help prevent regressions in relative-path resolution.
There was a problem hiding this comment.
The relative-path correctness here flows through the same TaskEnvironment.GetAbsolutePath path that's exercised by the engine-level multithreaded driver tests and by other migrations of similar shape (~10+ tasks now using the same pattern for relative path resolution). Adding a per-task multithreaded fixture would duplicate that infrastructure.
The behavior under the multithreaded driver is: GetAbsolutePath resolves against the task's project directory rather than process CWD — the resolution itself is the contract validated by TaskEnvironment unit tests. The functional path-resolution behavior here is identical to other migrated tasks (SignFile, ResolveKeySource in the same series), all relying on the same code path.
Happy to add coverage if reviewers feel strongly, but my read is the regression risk is low and the test cost is high.
…temSpec The previous code path treated a null-or-empty BaseManifest.ItemSpec as 'no file' via File.Exists returning false. Calling TaskEnvironment.GetAbsolutePath on an empty ItemSpec now throws ArgumentException (Sin 6), regressing that behavior. Add an explicit string.IsNullOrEmpty guard to preserve the old silent no-op semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds
[MSBuildMultiThreadableTask]to both conditional compilation variants ofGenerateTrustInfo. The NETFRAMEWORK version implementsIMultiThreadableTaskand absolutizesBaseManifest/TrustInfoFilepaths. The non-Framework stub is attribute-only.Fixes #13619
Parent epic: #11834