Allow Remove item operation to match on specified metadata instead of just item identity

[cdmihai]

PR based on #4975. Merge that one in first. The changes for this PR are in 7aa753e

If you have two items I2 and I1, and you want to remove all the items from I2 that match on some metadata with items from I1, today you have to go through some msbuild gymnastics. It would be nice to be able to express this with just one operation: <I2 Remove="@(I1)" MatchOnMetadata="M"/>.

Benefits:

• makes MSBuild easier to understand, which may reduce bugs.
• better perf, because it replaces a bunch of operations with a single one. This may not seem like much, but if this is in some sdk file that runs for every project out there, it can start getting noticeable.

Example which shows the gymnastics users have to go through today (second itemgroup), and what the gymnastics would be replaced with via this PR (third itemgroup)

<Project>
    <Target Name="Build">
        <ItemGroup>
            <I1 Include='a1' M='1'/>
            <I1 Include='b1' M='2'/>
            <I1 Include='c1' M='3'/>
            <I1 Include='d1' M='4'/>

            <I2 Include='a2' M='x'/>
            <I2 Include='b2' M='2'/>
            <I2 Include='c2' M='3'/>
            <I2 Include='d2' M='y'/>

            <I2_Copy Include="@(I2)"/>
        </ItemGroup>

        <ItemGroup>
            <I1_a Include="%(I1.M)" PreviousIdentity="%(I1.Identity)" />
            <I2_a Include="%(I2.M)" PreviousIdentity="%(I2.Identity)" />

            <I2_b Include="@(I2_a)" KeepMetadata="PreviousIdentity"/>

            <I2_a Remove="@(I1_a)" />

            <I2_c Include="@(I2_b)" KeepMetadata="PreviousIdentity"/>
            <I2_c Remove="@(I2_a)" />

            <I2_d Include="%(I2_c.PreviousIdentity)" />

            <I2_copy Remove="@(I2_d)" />
        </ItemGroup>

        <ItemGroup>
            <I2 Remove="@(I1)" MatchOnMetadata="M"/>
        </ItemGroup>

        <Message Importance="High" Text="I2_Copy: %(I2_Copy.Identity) M=[%(I2_Copy.M)]" />
        <Message Importance="High" Text=".............................." />

        <Message Importance="High" Text="I2: %(I2.Identity) M=[%(I2.M)]" />

    </Target>
</Project>

Output using this PR:

  I2_Copy: a2 M=[x]
  I2_Copy: d2 M=[y]
  ..............................
  I2: a2 M=[x]
  I2: d2 M=[y]

Specs:

• simplification: if MatchOnMetadata is present, then the Remove attribute must only contain one item reference @()
• simplification: MatchOnMetadata may specify a single metadata: <I2 Remove="@(I1)" MatchOnMetadata="M1"/>
• if a metadata name doesn't exist in both items, then it's considered a match. Special case: if none of the metadata exist in both items, then it should not match.

Todos:

• This PR only does Remove operations under targets. Reimplement it for Remove operations not under targets. Should be straight-forward, in LazyItemEvaluator.RemoveOperation
• Check that perf did not regress. RPS and, more importantly, total time spent in eval for OrchardCore.
• Write tests
  • For MatchOnData behaviour
    • Things to cover:
      • metadata name matching is case insensitive
      • metadata contents is not case sensitive
      • MatchOnMetadata can have multiple semicolon delimited metadata names
      • MatchOnMetadata can contain metadata which does not exist on the items. Nothing should match.
      • When MatchOnMetadata is present, a Remove with anything other than one item reference fails the build.
      • What happens when one puts metadata references (%()) in MatchOnMetadata
      • MatchOnMetadata is now a reserved msbuild keyword, project parsing should error if you do <I Include="foo" MatchOnMetadata="bar"/>
  • These need to be duplicated between IntrinsicTask_Tests (for Remove under target elements) and Build.OM.UnitTests.ProjectItem_Tests (for Remove not under target elements). Duplication ftw. Sad, so sad.
  • For OM construction, in https://github.com/microsoft/msbuild/blob/master/src/Build.OM.UnitTests/Construction/ConstructionEditing_Tests.cs

[cdmihai]

This would be readily useful in #4931

[cdmihai]

Another interesting scenario would be when the two items have differing metadata names that need to match. Say, Remove all items from I2 in I1 where I2.Foo == I1.Bar. The workaround here would be to rename the metadata name in one of the items so they match, which might be good enough.

[cdmihai]

Oops, took the PR out of draft mode by mistake (thinking it was another PR). Sadly I can't find the option to make it a draft again. Ah well.

[sfoslund]

@cdmihai a few notes about my changes:

• I believe I've covered all of the test cases you mentioned above, but let me know if I missed anything. Specifically, the test that covers putting metadata references in MatchOnMetadata tests for an error right now and I'm not sure if this is the right behavior.
• I haven't done perf testing and I don't know how to go about this- any tips?

[cdmihai]

For perf testing of evaluation, here's what I do nowadays:

• trigger an RPS run (by pushing the branch into an exp/* named branch of the official repo. The CI should pick it up and trigger a VS insertion and RPS run)
• manually measure how long evaluation takes for OrchardCore:
  • get total evaluation time by using the evaluation profiler, via the msbuild.exe command line arg. E.g.: /profileEvaluation:profile.tab. Total evaluation time is under the Total evaluation row, on the Inc column. Time is in milliseconds.
  • run this on master msbuild and the PR msbuild to compare time differences
  • for each branch, run it multiple times and average the result, to account for variation. I do 3-4 runs. Ensure you're not doing anything else on the machine to avoid a noisy environment. I usually close all my VS instances

[Forgind]

Only a partial review.

[sfoslund]

@cdmihai I think this latest change covers all of your previous feedback although I have not done perf testing. I also didn't respond to any of @Forgind's feedback since a lot of it seems to correspond to commits from #4975.

[sfoslund]

Thanks to @Forgind for doing some end to end testing of this along with #4931! @rainersigwald @cdmihai that PR has been rebased on top of this one so this is good to go for a final round of review.

[cdmihai]

LGTM, pending comments (apparently I can't approve the PR since it was opened by me 😃)

[rainersigwald]

Looking nice! Many nits but overall 👍

I can't remember--is this the one we evaluated doc impact on/drafted doc changes? Or was that another PR?

[sfoslund]

@rainersigwald I think I'm caught up with all the feedback other than this comment, let me know if I missed anything. I'm not aware of any pending docs changes.

[Forgind]

I think the only comment I had that I still feel pretty strongly about is unifying the ItemSpec methods. 90% of the logic is the same, so I really don't see why we should have duplicate code.

[Forgind]

LGTM!

[Forgind]

🥳