Skip to content

Ref pack fix [3.1] #18380

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
Jan 16, 2020
Merged

Ref pack fix [3.1] #18380

merged 1 commit into from
Jan 16, 2020

Conversation

JunTaoLuo
Copy link
Contributor

@JunTaoLuo JunTaoLuo commented Jan 16, 2020

It seems like the issue in the 3.1 template tests are caused by malformed targeting pack. In fact no ref assemblies were included in the targeting pack:
image

Indeed it seems like we weren't capturing aspnetcore ref assemblies in the ref pack contents at https://github.com/dotnet/aspnetcore/blob/release/3.1/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj#L131-L140
image

It turns out that the items ReferencePathWithRefAssemblies had the same identity (but different metadata) between implementation assemblies and ref assemblies and were therefore being excluded by the line https://github.com/dotnet/aspnetcore/blob/release/3.1/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj#L139
image

I haven't tracked down how or when this broke but it would have been something that was between the branding changes and when #17970 was first opened.

@ghost ghost added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Jan 16, 2020
@JunTaoLuo JunTaoLuo added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework and removed area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework labels Jan 16, 2020
@JunTaoLuo JunTaoLuo marked this pull request as ready for review January 16, 2020 07:42
@JunTaoLuo
Copy link
Contributor Author

Ping @dougbu @wtgodbe I think I figured out why 3.1's templating tests are broken. Though I know why our targeting pack is broken, I don't know how it came to be. Also, I haven't done much verification on the targeting pack so I don't know if there are other issues.

fyi @anurse @Pilchie

@JunTaoLuo JunTaoLuo changed the title Test ref pack fix Ref pack fix [3.1] Jan 16, 2020
Copy link
Contributor

@mkArtakMSFT mkArtakMSFT left a comment

Choose a reason for hiding this comment

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

Thanks for figuring this out, John!

Copy link
Contributor

@dougbu dougbu left a comment

Choose a reason for hiding this comment

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

Awesome if this works 😺

Can you explain why the condition and exclusions operate differently? Do the exclusions work correctly for the %(...NuGetPackageId) cases?

@dougbu
Copy link
Contributor

dougbu commented Jan 16, 2020

Test failure is something known to @BrennanConroy and he's promised to fix it soon. Getting this in.

@dougbu dougbu merged commit 4682c2a into release/3.1 Jan 16, 2020
@dougbu dougbu deleted the johluo/test-ref branch January 16, 2020 16:51
@dougbu dougbu added the tell-mode Indicates a PR which is being merged during tell-mode label Jan 16, 2020
@dougbu dougbu added this to the 3.1.2 milestone Jan 16, 2020
@BrennanConroy
Copy link
Member

Test failure is something known to @BrennanConroy and he's promised to fix it soon

I'm working on it right now :)

@dougbu
Copy link
Contributor

dougbu commented Jan 16, 2020

@Pilchie FYI I put this into the 3.1.2 milestone and marked it as tell-mode. Once things settle down, you or I should query for that label and send a summary to Tactics. Let me know your preference.

@Pilchie
Copy link
Member

Pilchie commented Jan 16, 2020

I'm (so far), only seeing 4 small things that I think are non-controversial in that label. Feel free to send the heads up to tactics.

@wtgodbe
Copy link
Member

wtgodbe commented Jan 16, 2020

Awesome, thanks @JunTaoLuo!

@JunTaoLuo
Copy link
Contributor Author

My understanding is that Exclusion filters on the Identity but Condition isn't. For example, if you have two items in the ItemGroup A [Metadata = x] and A [Metadata = y] , the items

<Filtered Include="@(A)" Exclude="@(A->WithMetadataValue('Metadata', 'x'))"/>

will resolve to:

<Filtered Include="A;A" Exclude="A"/>

Which means that Filtered will resolve to empty. Condition doesn't work this way and is applied to all items regardless of Identity.

My suspicion is that in the past the ReferencePathWithRefAssemblies items had different identities (for ref and impl assemblies) but now resolves to the same file as its identity. Luckily all the metadata remained so we can still filter correctly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform 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.

6 participants