[vs18.10] Expand built-in metadata references in the task host - #14823
[vs18.10] Expand built-in metadata references in the task host#14823JanProvaznik wants to merge 7 commits into
Conversation
Item definition metadata may reference built-in metadata, as in <OutputName>%(Filename)</OutputName>. Such a value is stored unexpanded and substituted when the metadata is read, so that it tracks the item it is read from. The marshalled item used to carry items across a process boundary holds a single flat dictionary and returned the stored text verbatim, so a task running in a task host saw the literal "%(Filename)" where the same task run in-proc saw "hello". Under -mt nearly every task runs in a task host, which is how the VS repository build ended up emitting paths containing "%(Filename)". Substitute the references on read instead, which keeps the value tracking the item spec even if the task reassigns it. Values a task writes on the item are literal and are excluded, matching what the task would read back in-proc. RecursiveDir is resolved from the item's own metadata because it derives from the wildcard the item was expanded from rather than from the item spec. No serialized state changes, so a task host built from different sources behaves exactly as it does today. Fixes dotnet#14763 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
A task that clones its input, through CopyMetadataTo or the TaskItem copy constructor that calls it, was handed the stored text rather than the finished value, so the inconsistency remained for that very common shape. An engine item substitutes when copying onto an item a task can reach; do the same here. Values the task wrote stay literal, as they do in-proc. Reassigning ItemSpec now clears the derived-metadata cache, as it does on an engine item. Without this a FullPath, RootDir or Directory read before the move was returned again after it. That was already the case for direct reads of those modifiers, and substitution on read would otherwise have extended it to references embedded in item definition metadata. Only remember a written value as literal when it could otherwise be substituted, so ordinary metadata writes no longer allocate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
The marshalled item recovers something the boundary erased, but nothing said so.
Name it: a value has an origin, either set directly on the item or inherited
from an item definition, and only the latter is expanded on read. Say at the
type level that this is a flattened view of an engine item, how origin is
recovered, and which accessors expand.
Rename to match, and use one word for one idea. Expansion is what MSBuild calls
this, so drop "substitute" as a synonym: _locallySetMetadata becomes
_writtenByTask, Substitute becomes ExpandIfFromItemDefinition, and the repeated
inline checks for a remaining "%(" become IsUnexpanded, which is what actually
distinguishes the two origins.
Add tests that hold the concepts still: every name in ItemSpecModifiers.All
reads the same on both sides, so a modifier added later is covered without
anyone remembering to; qualified references stay a decision rather than an
accident; and receiving an item does not mark its metadata as written by the
task.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
The expander had no tests of its own. It was covered only where TaskParameter
happened to exercise it, which left its own edge cases unchecked.
Add direct tests: whitespace and casing, text that is not a well formed
reference, several references in one value, a supplied RecursiveDir, derivation
from the given item spec, and no allocation when there is nothing to expand.
One test failed. After a "%(" that does not start a reference, the expander
resumed after the closing parenthesis, so it did not see a well formed
reference that began inside the text it had spanned. "%(foo%(Filename)" stayed
as it was, where evaluation gives "%(foohello". Resume just after the "%(",
which is what the evaluation expander does.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
Collapse the scan that guarded the copy. HasUnexpandedMetadata walked every value to decide whether to run a transform whose first act was the same check on the same string, so the work was done twice whenever anything needed expanding. The engine item can afford that guard because it bails out at once when the item has no item definitions, and it scans only the small shared definition metadata. The marshalled item has no such field, since that is the distinction the boundary erased, so the guard could only ever be a full scan. Chain the transform unconditionally instead: ExpandIfFromItemDefinition returns the value it was given when there is nothing to expand. Look for the metadata marker by searching for a single character. IndexOf(char) vectorizes and beats an ordinal two-character search, above all when the marker is absent, which is the usual case for a metadata value. This mirrors ExpressionShredder.IndexOfMarker, which is not available in every branch this has to reach. Also correct a test comment that described what an engine item does rather than what the test asserts, and a repeated word in another. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
There was a problem hiding this comment.
Pull request overview
Backports the fix for #14763 to vs18.10 so that item-definition metadata containing built-in metadata references (for example %(Filename)) is expanded consistently when tasks execute out-of-proc in a TaskHost, matching in-proc behavior and preventing literal %(...) from leaking into task-observed paths/values.
Changes:
- Teach
TaskParameterTaskItem(the marshalled item crossing the TaskHost boundary) to expand item-definition built-in metadata references on read while preserving task-written metadata as literal. - Add a minimal
BuiltInMetadataExpanderinMicrosoft.Build.Frameworkto expand only built-in metadata references in escaped strings, including correct handling ofRecursiveDir. - Add regression/unit tests covering cross-boundary parity, ItemSpec reassignment cache invalidation, and end-to-end TaskHost execution; bump servicing version to
18.10.1.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Shared/TaskParameter.cs | Expands unexpanded built-in metadata on read for marshalled items; tracks task-written metadata; clears cached modifiers when ItemSpec changes; expands during metadata copy operations. |
| src/Shared/UnitTests/TaskParameter_Tests.cs | Adds extensive regression coverage for TaskParameter round-tripping and task-host boundary semantics. |
| src/Framework/BuiltInMetadataExpander.cs | Introduces a lightweight expander for built-in metadata references needed in task-host scenarios. |
| src/Framework.UnitTests/BuiltInMetadataExpander_Tests.cs | Adds unit tests validating expansion behavior, marker scanning, and allocation-sensitive behavior. |
| src/Build.UnitTests/BackEnd/MetadataObservationTask.cs | Adds a test task used to observe metadata values and confirm in-proc vs out-of-proc execution. |
| src/Build.UnitTests/BackEnd/ItemDefinitionMetadataInTaskHost_Tests.cs | Adds end-to-end TaskHost regression tests asserting task-observed metadata matches in-proc behavior. |
| eng/Versions.props | Updates VersionPrefix from 18.10.0 to 18.10.1 for the servicing backport. |
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
Consider updating this value as well now that 18.10 is stable: https://github.com/JanProvaznik/msbuild/blob/3c551b98b940fb989ffb026282b499a953b0b97c/azure-pipelines/vs-insertion.yml#L64 Then you don't have to manually pick the insertion branch and we are prepared for the next months in case there's any other servicing event :) |
18.10 has snapped to stable, so the automatic insertion target is retargeted from rel/insiders to rel/stable. This is step 4.6 of the release checklist, and matches vs18.9. A servicing commit on this branch now reaches the branch it is servicing without anyone selecting the target by hand. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3c4a0419-0feb-42ee-9963-d1da3b10d3d6
|
Note This comment was generated by GitHub Copilot CLI. Good catch, done in 2aa0df0. This is step 4.6 of the release checklist ("retarget |
Backport of #14770 to
vs18.10. Fixes #14763.Summary
Building the VS repository with CloudBuild in multithreaded mode (
-mt) produces wrong file paths. This corrects it. Normal builds are not affected.-mt.%(Filename)instead of the file name. One build wrote a path ending in\%(Filename).cs.Why this matters now
The defect is old, not a regression. It became visible because
-mtsends almost every task to a separate process, where the defect lives. Before-mt, a build had to opt a single task into that process to see it, which is rare.We need VS msbuild updated with the fix to continue Cloudbuild dogfooding
What was wrong
A project can set default metadata for a group of items:
A task in the MSBuild process reads
OutputNameashello. The same task in a separate process reads%(Filename), the literal text. The task then uses that text as if it were a file name.MSBuild resolves
%(Filename)at the moment the metadata is read, not once in advance, because a task can rename an item and the value has to follow. When an item is sent to another process, the data that says "this value still needs to be resolved" is dropped. The receiving side returned the text as it was.This change lets the receiving side recognise such a value and resolve it, so a task reads the same value in either process. Nothing about the data sent between processes changes.
Scope of the change
VersionPrefixto18.10.1, and insert into VSrel/stable.The product change is 220 added lines in two files, of which 72 are code and the rest are documentation comments and braces. The code that runs during a normal build is untouched.
Five ways a task could tell that it was running in a task host are corrected. Each was measured, running the same task in both processes.
hello%(Filename)renamed%(Filename)%(RecursiveDir), e.g.out\%(RecursiveDir)%(Filename)%(Extension).out\sub1\sub2\hello.txtout\%(RecursiveDir)hello.txtnew TaskItem(input)orCopyMetadataTo.hello%(Filename)Row 5 is a separate old defect, not caused by this change. The released MSBuild returns the same stale path. It is corrected here because the fix for the other rows would otherwise have spread it further.
Why normal builds cannot be affected
The changed type is only created when items are sent between processes. There are four such places, all of them task host or resolver traffic. A task in the MSBuild process never reaches this code.
-mt, so no build chose the current behaviour and none can depend on it.The full technical description, including two limits that are deliberately left as they are, is in #14770.
Backport notes
All five commits from #14770 apply to
vs18.10with no conflict. No changes were needed to make them apply.Two release infrastructure changes are included:
VersionPrefixmoves from18.10.0to18.10.1.rel/insiderstorel/stable, since 18.10 has snapped to stable. This is step 4.6 of the release checklist, and matchesvs18.9.Validation evidence
The change was validated with three experimental Visual Studio insertions. The two forced variants differ by this fix, giving a direct control instead of relying only on unit-test results.
This comparison establishes two separate facts:
The remaining failures do not distinguish the fix from the control:
DownloadComponentsin the same way because an isolated CLR4 TaskHost could not apply the parent process's assembly-version binding policy. The normal insertion passed. This is a separate TaskHost compatibility limitation, tracked by #14838.Conclusion: the controlled forced-task comparison reproduces the customer-impacting failure without the fix and removes it with the fix. Normal task execution shows no correctness or performance change, and no repeatable failure remains unique to the fix.