Migrate CreateItem task to multithreaded execution model#13590
Migrate CreateItem task to multithreaded execution model#13590jankratochvilcz wants to merge 1 commit intomainfrom
Conversation
Apply the absolutize-at-boundary pattern to CreateItem: mark it with [MSBuildMultiThreadableTask], implement IMultiThreadableTask with TaskEnvironment defaulted to Fallback, and route wildcard expansion through TaskEnvironment.ProjectDirectory instead of null (cwd). This ensures CreateItem resolves relative wildcard patterns against the correct project directory under multithreaded execution, where multiple projects may share a single process cwd. Fixes #13568 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR migrates the built-in CreateItem task to MSBuild’s multithreaded execution model by wiring it into IMultiThreadableTask and ensuring wildcard expansion is rooted at the task’s project directory instead of process-wide current directory.
Changes:
- Marked
CreateItemas[MSBuildMultiThreadableTask]and implementedIMultiThreadableTaskwith aTaskEnvironmentdefaulting toTaskEnvironment.Fallback. - Updated wildcard expansion to call
FileMatcher.GetFiles(TaskEnvironment.ProjectDirectory, ...)instead of using process cwd. - Added new unit tests validating project-directory-based wildcard behavior (Include/Exclude/RecursiveDir/absolute patterns/literals).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Tasks/CreateItem.cs | Adds MT task plumbing and switches wildcard expansion base directory to TaskEnvironment.ProjectDirectory. |
| src/Tasks.UnitTests/CreateItem_Tests.cs | Adds tests for ProjectDirectory-rooted wildcard expansion scenarios. |
| using System.IO; | ||
| using System.Linq; | ||
| using Microsoft.Build.Execution; |
There was a problem hiding this comment.
using System.Linq; appears unused in this test file; please remove it to avoid unnecessary imports and potential analyzer noise.
| /// <summary> | ||
| /// Relative wildcard Include resolves against TaskEnvironment.ProjectDirectory, not cwd. | ||
| /// </summary> | ||
| [Fact] | ||
| public void WildcardInclude_ResolvesAgainstProjectDirectory() | ||
| { | ||
| using TestEnvironment env = TestEnvironment.Create(_testOutput); | ||
|
|
There was a problem hiding this comment.
PR description says 7 new tests were added (including concurrency + fallback-parity cases), but this file currently adds 5 tests. Either add the missing tests or update the PR description so it matches what’s actually in the diff.
| /// Pre-expanded literal items (no wildcards) produce identical output regardless of | ||
| /// ProjectDirectory, Exclude, and AdditionalMetadata settings. |
There was a problem hiding this comment.
The XML doc for this test claims the output is identical “regardless of ProjectDirectory”, but the test never sets TaskEnvironment (so it doesn’t actually vary ProjectDirectory). Consider either setting a non-default TaskEnvironment here or rewording the comment to reflect what’s being asserted.
| /// Pre-expanded literal items (no wildcards) produce identical output regardless of | |
| /// ProjectDirectory, Exclude, and AdditionalMetadata settings. | |
| /// Verifies that for pre-expanded literal items (no wildcards), <see cref="CreateItem"/> | |
| /// applies <c>Exclude</c> filtering and <c>AdditionalMetadata</c> to the resulting items. |
| (files, _, _, string? globFailure) = FileMatcher.Default.GetFiles(TaskEnvironment.ProjectDirectory, i.ItemSpec); | ||
| if (globFailure != null) |
There was a problem hiding this comment.
FileMatcher.GetFiles base directory change is the key MT migration behavior; it would be good to add a regression test that runs two CreateItem instances with different TaskEnvironment.ProjectDirectory values concurrently (or at least back-to-back with different project dirs) and asserts they return disjoint file sets for the same relative glob, to guard against cross-project leakage in multithreaded builds.
24-Dimension Expert Review — PR #13590Test Coverage: Missing Tests from PR DescriptionThe PR description lists 7 new tests, but only 5 appear in the diff. Two tests are missing:
The unused Recommendation: Either include the two missing tests in this PR (they are the most important coverage for the migration's correctness claims), or update the PR description to remove them and file a follow-up issue. Minor Naming Observations (NIT)The existing tests in this file use flat PascalCase (
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
24-Dimension Expert Review Summary
Clean, well-structured migration that follows the established pattern from ~47 previously migrated tasks. The production code change is minimal and correct. No blocking issues found.
| # | Dimension | Verdict |
|---|---|---|
| 1 | Backwards Compatibility | ✅ LGTM |
| 2 | ChangeWave Discipline | ✅ LGTM |
| 3 | Performance & Allocation Awareness | ✅ LGTM (NIT: cache ProjectDirectory outside loop) |
| 4 | Test Coverage & Completeness | |
| 5 | Error Message Quality | ✅ LGTM |
| 6 | Logging & Diagnostics Rigor | ✅ LGTM |
| 7 | String Comparison Correctness | ✅ LGTM |
| 8 | API Surface Discipline | ✅ LGTM |
| 9 | MSBuild Target Authoring | ✅ LGTM (no targets/props changed) |
| 10 | Design Before Implementation | ✅ LGTM |
| 11 | Cross-Platform Correctness | ✅ LGTM |
| 12 | Code Simplification | ✅ LGTM (NIT: unused using System.Linq) |
| 13 | Concurrency & Thread Safety | ✅ LGTM |
| 14 | Naming Precision | ✅ LGTM (NIT: naming style) |
| 15 | SDK Integration Boundaries | ✅ LGTM |
| 16 | Idiomatic C# Patterns | ✅ LGTM |
| 17 | File I/O & Path Handling | ✅ LGTM |
| 18 | Documentation Accuracy | ✅ LGTM |
| 19 | Build Infrastructure Care | ✅ LGTM |
| 20 | Scope & PR Discipline | |
| 21 | Evaluation Model Integrity | ✅ LGTM |
| 22 | Correctness & Edge Cases | ✅ LGTM |
| 23 | Dependency Management | ✅ LGTM |
| 24 | Security Awareness | ✅ LGTM |
Key findings:
- Production code is correct and follows the established pattern. The
null→TaskEnvironment.ProjectDirectorychange is semantically equivalent in Fallback mode and correctly enables isolated project directories in MT mode. - Two tests from the PR description are missing.
ConcurrentProjectDirectories_ReturnDisjointFileSetsandFallbackTaskEnvironment_WildcardParityWithLegacyare not in the diff. These would provide the strongest validation of this migration's correctness claims. - Minor NITs: unused
using System.Linq, cacheableProjectDirectoryaccess in loop, naming style.
No BLOCKING issues. Overall this is a clean, well-executed migration PR.
Generated by Expert Code Review (on open) for issue #13590 · ● 15.6M
| using System; | ||
| using System.Collections.Generic; | ||
| using System.IO; | ||
| using System.Linq; |
There was a problem hiding this comment.
[NIT] Code Simplification
using System.Linq is added but none of the 5 new test methods (nor any existing test in this file) use any System.Linq extension method. This appears to be a leftover from the two tests mentioned in the PR description (ConcurrentProjectDirectories_ReturnDisjointFileSets and FallbackTaskEnvironment_WildcardParityWithLegacy) that are not present in the diff.
Recommendation: Remove the unused using System.Linq; directive, or add the two missing tests that likely depend on it.
| else if (isLegalFileSpec) | ||
| { | ||
| (files, _, _, string? globFailure) = FileMatcher.Default.GetFiles(null /* use current directory */, i.ItemSpec); | ||
| (files, _, _, string? globFailure) = FileMatcher.Default.GetFiles(TaskEnvironment.ProjectDirectory, i.ItemSpec); |
There was a problem hiding this comment.
[NIT] Performance & Allocation Awareness
TaskEnvironment.ProjectDirectory is accessed inside the foreach loop (via TryExpandWildcards). In Fallback mode, each access invokes NativeMethods.GetCurrentDirectory() (a syscall + string allocation) and wraps the result in an AbsolutePath struct. While the old code with null also involved per-item file system resolution internally, caching the value before the loop is a minor cleanliness improvement:
string projectDir = TaskEnvironment.ProjectDirectory;
// then inside loop:
(files, _, _, string? globFailure) = FileMatcher.Default.GetFiles(projectDir, i.ItemSpec);Note: The practical impact is small since CreateItem tasks typically have few wildcard items, so this is NIT-level rather than a real performance concern.
Migrates the
CreateItemtask to MSBuild's multithreaded execution model, applying the "absolutize-at-boundary" pattern previously established for other tasks (see #13267).Fixes #13568.
Changes
src/Tasks/CreateItem.cs[MSBuildMultiThreadableTask]attribute andIMultiThreadableTaskinterfaceTaskEnvironmentproperty with= TaskEnvironment.Fallbackdefault (matching all other migrated tasks)FileMatcher.Default.GetFiles(null, ...)→FileMatcher.Default.GetFiles(TaskEnvironment.ProjectDirectory, ...)so relative wildcard patterns resolve against the project directory instead of process cwdsrc/Tasks.UnitTests/CreateItem_Tests.cs— 7 new testsChangeWave consideration
No ChangeWave required. For absolute and rooted wildcard patterns,
GetFilesignores the project directory entirely. For relative wildcard patterns, the base directory changes fromEnvironment.CurrentDirectorytoTaskEnvironment.ProjectDirectory. In legacy (non-MT) execution the two are equal during normal builds, so observable behavior is unchanged.Validation
CreateItem_Testspass on net10.0 with no behavior changes./build.sh -v quietsucceeds with 0 warnings