Enlighten ResolveKeySource for multithreaded mode#13623
Enlighten ResolveKeySource for multithreaded mode#13623jankratochvilcz wants to merge 1 commit intomainfrom
Conversation
Add [MSBuildMultiThreadableTask] and implement IMultiThreadableTask. Absolutize KeyFile path before File.OpenRead() while preserving the original path string for [Output] ResolvedKeyFile property (Sin 1) and error messages (Sin 2). Also absolutize CertificateFile for FileExists, GetCertContentType, Import, and X509Certificate2 ctor. Fixes #13620 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
/review |
|
✅ Expert Code Review (command) completed successfully! |
There was a problem hiding this comment.
Expert Code Review — 24-Dimension Analysis
Verdict: Clean PR with one moderate observation
Summary
This PR correctly applies the established IMultiThreadableTask pattern to ResolveKeySource, matching the approach used for Copy, Delete, Move, Touch, and ~15 other enlightened tasks. The changes are minimal, focused, and mechanically correct.
Dimension Results (24 dimensions evaluated)
| Dimension | Result | Notes |
|---|---|---|
| 1. Backwards Compatibility | ✅ LGTM | TaskEnvironment.Fallback preserves single-process behavior identically |
| 2. ChangeWave Discipline | ✅ LGTM | Multithreaded mode is opt-in; no ChangeWave needed |
| 3. Performance | ✅ LGTM | One GetAbsolutePath call per execution — negligible |
| 4. Test Coverage | See inline comment — no tests exist for this task | |
| 5. Error Message Quality | ✅ LGTM | No new messages; originals correctly preserved |
| 6. Logging | ✅ LGTM | No behavioral change requiring new diagnostics |
| 7. String Comparison | ✅ LGTM | No comparison logic changed |
| 8. API Surface | ✅ LGTM | TaskEnvironment property is public per IMultiThreadableTask contract |
| 9. Target Authoring | N/A | Not a .targets file |
| 10. Design | ✅ LGTM | Follows established pattern precisely |
| 11. Cross-Platform | ✅ LGTM | AbsolutePath/Path.Combine handles cross-platform correctly |
| 12. Code Simplification | ✅ LGTM | Clean, minimal changes |
| 13. Concurrency | ✅ LGTM | No shared mutable state; HashFromBlob is pure; StrongNameHelpers uses [ThreadStatic] |
| 14. Naming | ✅ LGTM | keyFilePath and certificateFilePath are clear |
| 15. SDK Integration | N/A | |
| 16. Idiomatic C# | ✅ LGTM | Follows codebase conventions |
| 17. File I/O & Path Handling | ✅ LGTM | Correctly uses GetAbsolutePath for all file I/O; preserves original paths for outputs/errors |
| 18. Documentation | ✅ LGTM | |
| 19. Build Infrastructure | N/A | |
| 20. Scope & PR Discipline | ✅ LGTM | Single-file, focused change |
| 21. Evaluation Model | N/A | |
| 22. Correctness & Edge Cases | ✅ LGTM | Validated: absolute paths handled correctly by Path.Combine; null/empty guarded before GetAbsolutePath; #if FEATURE_PFX_SIGNING scoping correct |
| 23. Dependency Management | N/A | |
| 24. Security | ✅ LGTM | No security regression |
Key Validations Performed
- Path.Combine behavior: Confirmed
Path.Combine(base, absolutePath)correctly returns just the absolute path — no malformed paths - Output preservation:
ResolvedKeyFile(line 114) uses originalKeyFile; error messages (lines 149, 150, 159, 246, 250) use original paths — correct #ifguard placement: BothkeyFilePath(line 124) andcertificateFilePath(line 212) are inside#if FEATURE_PFX_SIGNING— correct- Thread safety: No static mutable fields,
HashFromBlobis pure, each instance operates independently
Generated by Expert Code Review (command) for issue #13623 · ● 6.7M
| [MSBuildMultiThreadableTask] | ||
| public class ResolveKeySource : TaskExtension, IMultiThreadableTask | ||
| { | ||
| public TaskEnvironment TaskEnvironment { get; set; } = TaskEnvironment.Fallback; |
There was a problem hiding this comment.
Test Coverage — MODERATE
ResolveKeySource has zero unit tests in the repo, and this PR adds none. Other tasks enlightened for multithreaded mode (Copy, Delete, Move, Touch) all have test suites that set TaskEnvironment = TaskEnvironmentHelper.CreateForTest(). While this change is a mechanical pattern application and the infrastructure is tested centrally, consider adding at least a basic test that verifies:
KeyFilewith a relative path resolves correctly viaTaskEnvironment.GetAbsolutePath(e.g., usingTaskEnvironment.CreateWithProjectDirectoryAndEnvironment)ResolvedKeyFileand error messages preserve the original (non-absolutized) path
The pre-existing test gap isn't this PR's fault, but enlightenment is a good opportunity to start covering this task.
| /// Determine the strong name key source | ||
| /// </summary> | ||
| public class ResolveKeySource : TaskExtension | ||
| [MSBuildMultiThreadableTask] |
There was a problem hiding this comment.
Is the attribute necessary if the class implements IMultiThreadableTask?
Adds
[MSBuildMultiThreadableTask]andIMultiThreadableTasktoResolveKeySource. AbsolutizesKeyFilebeforeFile.OpenRead()andCertificateFilebeforeFileExists/GetCertContentType/Import/X509Certificate2ctor, while preserving the original paths for[Output] ResolvedKeyFileproperty and error messages.Fixes #13620
Parent epic: #11834