Skip to content

[release/2.1] Add @(ProjectReference) items for patched projects #29037

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Feb 10, 2021

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Jan 5, 2021


Originally was an attempt to demonstrate #29023

  • showed test indirect references let most tests keep working

@dougbu dougbu added the * NO MERGE * Do not merge this PR as long as this label is present. label Jan 5, 2021
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 5, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Jan 5, 2021

@Tratcher I believe I've confirmed #29023 is a real issue but would appreciate your confirmation. With the current state of this PR, I'd expect some Kestrel tests (for example) to fail but all I see listed in the Tests tab are projects with direct Microsoft.AspNetCore.Http references. Do you agree many more tests should have failed❔

@Tratcher
Copy link
Member

Tratcher commented Jan 5, 2021

Lol, you intentionally broke cookies? I would not expect any kestrel tests to fail, kestrel provides values rather than consumes them. However I'm surprised I don't see any failures in Security.

@dougbu
Copy link
Contributor Author

dougbu commented Jan 5, 2021

Lol, you intentionally broke cookies?

😏

@dougbu
Copy link
Contributor Author

dougbu commented Jan 6, 2021

🆗 the binary log 💯% confirmed Microsoft.AspNetCore.Http.dll was resolved from the baseline package in every project except test projects containing <Reference Include="Microsoft.AspNetCore.Http" />. That isn't what we want and I'm starting work on resolving #29023.

/fyi @dotnet/aspnet-build

@dougbu dougbu force-pushed the dougbu/test.break.29023 branch 2 times, most recently from fb95fbc to b4f09b3 Compare January 26, 2021 01:33
@dougbu dougbu force-pushed the dougbu/test.break.29023 branch from b4f09b3 to b8c2b78 Compare January 27, 2021 23:57
@dougbu
Copy link
Contributor Author

dougbu commented Jan 28, 2021

With help from @Forgind and @yuehuang010, this new target is now working. Many thanks❕

I'm going to try out a couple of tweaks locally then back out the first two commits (i.e. don't intentionally break tests) and turn this into a real PR😃

@dougbu
Copy link
Contributor Author

dougbu commented Jan 28, 2021

Note latest iteration failed many more tests than previous ones i.e. transitive references to the project I broke are no longer hiding the break from test projects and samples.

- #29023
- ensure tests do not use baseline assemblies for projects in current patch
@dougbu dougbu force-pushed the dougbu/test.break.29023 branch from b8c2b78 to 2c68337 Compare January 28, 2021 06:08
@dougbu dougbu removed the * NO MERGE * Do not merge this PR as long as this label is present. label Jan 28, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Jan 28, 2021

/fyi test run with the fix failed 404 tests on Windows and 322 elsewhere while run before fix failed far fewer -- 206 tests on Windows and 103 elsewhere.

@dougbu dougbu requested a review from a team January 28, 2021 06:19
@dougbu dougbu marked this pull request as ready for review January 28, 2021 06:19
@dougbu dougbu changed the title [release/2.1] Do not escape cookie names in lookups [release/2.1] Add @(ProjectReference) items for patched projects Jan 28, 2021
@dougbu dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Jan 28, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Jan 28, 2021

This is tell-mode because it's infrastructure and the changes affect only non-implementation projects.

@dougbu dougbu modified the milestones: 2.1.25, 2.1.26 Jan 28, 2021

<!-- Ignore unreferenced assemblies and directly-referenced projects. -->
<_ProjectsToAdd Remove="@(_ProjectsToAdd)" />
<!-- Condition uses same Filename metadata when evaluating the other two item groups. -->
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Put another way, the Conditions here and at the end of the target use a corner case of the task batching features. Shared metadata and batching on that metadata are the keys.

<_ProjectsToAdd Remove="@(_ProjectsToAdd)" />
<!-- Condition uses same Filename metadata when evaluating the other two item groups. -->
<_ProjectsToAdd Include="@(_ProjectsInPatch)"
Condition=" '%(Filename)' != '' AND
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this condition doing exactly? Add a direct reference to the projects being patched, unless...

Copy link
Contributor Author

@dougbu dougbu Jan 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My mental model may be wrong (@Forgind and @yuehuang010, please chime in) but https://docs.microsoft.com/en-us/visualstudio/msbuild/msbuild-batching?view=vs-2019#item-and-property-mutations contains some of the information

  1. The %(Filename) item metadata causes the implicit CreateItem task to be batched
  2. Use of that metadata also limits the @(ResolvedCompileFileDefinitions) and @(RuntimeCopyLocalItems) item groups to the item(s) with the same %(Filename)
    • This part doesn't seem to be explicit in the documentation
  3. The OR clause confirms at least one such item exists

At a higher level, this creates a @(_ProjectsToAdd) item group containing those @(_ProjectsInPatch) items that match on filename with one or more @(ResolvedCompileFileDefinitions) or @(RuntimeCopyLocalItems) items, excluding existing @(ProjectReference) items. Another way of looking at it is the created item group contains the projects in the current patch that (a) aren't referenced directly but (b) match assemblies in the current build.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see it used in this exampled (https://docs.microsoft.com/en-us/visualstudio/msbuild/item-metadata-in-task-batching?view=vs-2019#divide-several-item-lists-into-batches). So, it is feature name is "Divide several item lists into batches":

    <Target Name="ShowMessage">
        <Message
            Text = "Number: %(Number) -- Items in ExampColl: @(ExampColl) ExampColl2: @(ExampColl2)"/>
    </Target>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point @yuehuang010. I'm not sure how many times I read that example without thinking "Oh, this makes intersection / subsetting dead easy"😃

@dougbu
Copy link
Contributor Author

dougbu commented Jan 28, 2021

@wtgodbe is there a reason the aspnetcore-pr-validation-temp pipeline still exists and is still the one we require for release/2.1 merges❔ Tests failed in the first run since I got rid of the intentional break but only in that pipeline. The same tests ran fine in aspnetcore-ci.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 28, 2021

@wtgodbe is there a reason the aspnetcore-pr-validation-temp pipeline still exists and is still the one we require for release/2.1 merges❔ Tests failed in the first run since I got rid of the intentional break but only in that pipeline. The same tests ran fine in aspnetcore-ci.

No, I can remove the validation-temp runs

@wtgodbe
Copy link
Member

wtgodbe commented Jan 28, 2021

#29731

@dougbu dougbu merged commit 7aef172 into release/2.1 Feb 10, 2021
@dougbu dougbu deleted the dougbu/test.break.29023 branch February 10, 2021 20:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants