Add missing assemblies to ref pack by JunTaoLuo · Pull Request #19161 · dotnet/aspnetcore

JunTaoLuo · 2020-02-19T19:46:56Z

This fixes the incorrect condition from #18380. The issue is that the CoreFx ref assemblies do not have the IsAssemblyReference metadata set. As a result, the assemblies were excluded from the AspNetCore.App.Ref ref pack.

This PR updates the condition so that only the assemblies with IsAssemblyReference set to false are excluded.

Directory.Build.props

dougbu · 2020-02-19T20:47:54Z

src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj

      <AspNetCoreReferenceAssemblyPath
          Include="@(ReferencePathWithRefAssemblies)"
-          Condition="'%(ReferencePathWithRefAssemblies.IsReferenceAssembly)' == 'true'"
+          Condition="'%(ReferencePathWithRefAssemblies.IsReferenceAssembly)' != 'false'"


Have trouble believing the previous Exclude item was wrong. Instead the ref/ project shouldn't provide metadata in @(ReferencePathWithRefAssemblies).

Ahh, I think I now understand what you mean. We should probably investigate what's happening here.

I believe we need to exclude %(IsReferenceAssemblyProject) items from @(AspNetCoreAppReference) in the main @(Reference) item group at

aspnetcore/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj

Line 62 in 0e4635c

<Reference Include="@(AspNetCoreAppReference);@(AspNetCoreAppReferenceAndPackage);@(ExternalAspNetCoreAppReference)" />

then add another with <ReferenceOutputAssembly>false</ReferenceOutputAssembly> for the @(AspNetCoreAppReference) items that are from ref/ projects. Might be possible to shorten the new code to

<ItemGroup> <Reference Include="@(AspNetCoreAppReference);@(AspNetCoreAppReferenceAndPackage);@(ExternalAspNetCoreAppReference)"> <ReferenceOutputAssembly Condition=" '%(IsReferenceAssemblyProject)' == 'true' ">false</ReferenceOutputAssembly> </Reference>

Might be fine to remove references to the ref/ projects entirely i.e. exclude @(AspNetCoreAppReference) and @(AspNetCoreAppReferenceAndPackage) items with '%(IsReferenceAssemblyProject)' == 'true' metadata. Of course, need to confirm that metadata is present. (Root cause here is those two item groups map names to projects and the names are ambiguous.)

JunTaoLuo · 2020-02-19T21:13:32Z

Spot checking and it seems like the CoreFx binaries are now included in the ref pack:

wtgodbe · 2020-02-19T21:18:13Z

This has the same content as the 3.1.0 ref pack now (the same 253 items). However, we now need to make sure that the updated ref assembly from System.IO.Pipelines 4.7.1 does not make it in to the 3.1.3 targeting pack, but we do need to include the patched implementation assembly in the runtime pack (see dotnet/corefx#42837). @dougbu @JunTaoLuo @ericstj any ideas?

wtgodbe · 2020-02-19T21:20:55Z

Convert

aspnetcore/eng/SharedFramework.External.props

Lines 55 to 72 in ff6e1eb

    
               <ExternalAspNetCoreAppReference Include="System.IO.Pipelines"                                          Version="$(SystemIOPipelinesPackageVersion)" /> 
        
               <ExternalAspNetCoreAppReference Include="System.Security.Cryptography.Xml"                             Version="$(SystemSecurityCryptographyXmlPackageVersion)" /> 
        
               <!-- 
        
                 Transitive dependencies of other assemblies in the shared framework. These are listed separately and should not be included directly 
        
                 when setting `<Reference>`. These are listed for the purpose of tests and servicing builds only. 
        
                 If implementation details change and these assemblies are no longer showing up in the shared framework as a result of that, 
        
                 it is okay to remove these transitive dependencies. 
        
                 If these are needed as direct dependencies, it is okay to change them to ExternalAspNetCoreAppReference and move up into sections above. 
        
               --> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="Microsoft.Win32.SystemEvents"                       Version="$(MicrosoftWin32SystemEventsPackageVersion)" /> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="System.Diagnostics.EventLog"                        Version="$(SystemDiagnosticsEventLogPackageVersion)" /> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="System.Drawing.Common"                              Version="$(SystemDrawingCommonPackageVersion)" /> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="System.Security.Cryptography.Pkcs"                  Version="$(SystemSecurityCryptographyPkcsPackageVersion)" /> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="System.Security.Permissions"                        Version="$(SystemSecurityPermissionsPackageVersion)" /> 
        
               <_TransitiveExternalAspNetCoreAppReference Include="System.Windows.Extensions"                          Version="$(SystemWindowsExtensionsPackageVersion)" />

into 2 separate item groups, one for the latest versions and one for the PatchVersion=0 versions, which are used by the runtime pack & targeting pack respectively?

JunTaoLuo · 2020-02-19T21:29:50Z

@wtgodbe can you try that approach and see what it looks like? Feel free to update this branch.

wtgodbe · 2020-02-19T22:11:26Z

Took a crack at it with 903ddbd & cac1ed2. We'll need to wait until dotnet/corefx#42837 flows to AspNetCore to really be sure it worked.

src/Framework/test/Microsoft.AspNetCore.App.UnitTests.csproj

JunTaoLuo · 2020-02-20T02:44:43Z

looks like tests are passing, @wtgodbe can you check the targeting pack?

wtgodbe · 2020-02-20T02:48:59Z

@JunTaoLuo I'll check it once we take the CoreFx change that bumps System.IO.Pipelines to 4.7.1 (which should be very soon)

dougbu

Other than @wtgodbe's extra newline (a nit 😈), code looks good. Of course, we want to be sure that part of the change works w/ new CoreFx packages before actual merge

dougbu · 2020-02-20T02:52:06Z

Thanks @JunTaoLuo for banging this out and @wtgodbe for your additions as well as

I'll check it once we take the CoreFx change

wtgodbe · 2020-02-20T03:16:55Z

Might not be able to validate this w/ the corefx change tonight, Maestro seems to be on the fritz - it hasn't opened any PRs in the last ~3 hours (@chcosta @riarenas @JohnTortugo @mmitche is it a known thing?)

…the-ref-pack

wtgodbe · 2020-02-20T18:53:33Z

This is now ready to be validated once we have the build

wtgodbe · 2020-02-20T19:01:15Z

/__w/1/s/src/Framework/ref/Microsoft.AspNetCore.App.Ref.csproj : error NU1605: Detected package downgrade: System.IO.Pipelines from 4.7.1 to 4.7.0. Reference the package directly from the project to select a different version. [/__t/dotnet/sdk/3.1.100/NuGet.targets]

I'll add NU1605 to the ref pack's NoWarn, but will we wind up transitively bringing in the higher version anyway? CC @dougbu @JunTaoLuo

eng/SharedFramework.External.props

@JunTaoLuo

- yeah, @JunTaoLuo and I agreed w/ you @wtgodbe

wtgodbe · 2020-02-20T19:36:52Z

##[error]eng/SharedFramework.External.props(10,18): error MSB4100: Expected "$(IsReferenceAssemblyProject)" to evaluate to a boolean instead of "", in condition "'$(MSBuildProjectName)' == 'Microsoft.AspNetCore.App.Ref' OR $(IsReferenceAssemblyProject) ".

Looks like IsReferenceAssemblyProject isn't always set. Will update the condition

JunTaoLuo · 2020-02-20T20:51:00Z

Which projects was resolving IsReferenceAssemblyProject as empty?

wtgodbe · 2020-02-20T20:53:16Z

Which projects was resolving IsReferenceAssemblyProject as empty?

I'm not sure, the build for Doug's commit seems to have disappeared

wtgodbe · 2020-02-20T21:02:02Z

The ref pack & runtime pack both look good! The ref pack has the same content as the 3.1.0, the same platform manifest as 3.1.2, and the 4.0.2.0 ref assembly for System.IO.Pipelines (rather than the newly versioned 4.0.2.1 version, which is in the 4.7.1 package we just ingested). The runtime pack has the same content as 3.1.2, and System.IO.Pipelines is versioned 4.0.2.1.

@dougbu @JunTaoLuo @ericstj anything else you'd like me to check on before merging?

JunTaoLuo · 2020-02-20T21:16:14Z

dougbu · 2020-02-20T22:59:37Z

Oh wait, who owns this now? I nominate @JunTaoLuo to do the final squash

John Luo added 2 commits February 19, 2020 11:46

Add missing assemblies to ref pack

b61c478

amend

d6654d1

dougbu reviewed Feb 19, 2020

View reviewed changes

Update comments too

0e4635c

Use patchVersion=0 for ref pack dependencies

903ddbd

JunTaoLuo marked this pull request as ready for review February 19, 2020 22:13

wtgodbe and others added 3 commits February 19, 2020 14:13

Fixup versions

cac1ed2

Enable targeting pack tests

abe41c2

Fix targeting pack test

44dab04

JunTaoLuo commented Feb 20, 2020

View reviewed changes

src/Framework/test/Microsoft.AspNetCore.App.UnitTests.csproj Outdated Show resolved Hide resolved

Feedback

4c15267

dougbu approved these changes Feb 20, 2020

View reviewed changes

dougbu mentioned this pull request Feb 20, 2020

Merge branch release/3.1 --> master #19122

Merged

Pilchie added the feature-platform Deprecated: Cross-cutting issues related to ASP.NET Core as a platform label Feb 20, 2020

wtgodbe mentioned this pull request Feb 20, 2020

[release/3.1] Update dependencies from EFCore #19201

Merged

Merge remote-tracking branch 'origin/release/3.1' into johluo/always-…

370f79e

…the-ref-pack

Update Microsoft.AspNetCore.App.Ref.csproj

168dff4

wtgodbe reviewed Feb 20, 2020

View reviewed changes

eng/SharedFramework.External.props Outdated Show resolved Hide resolved

Downgrade System.* versions for all ref/ projects

5df69da

- yeah, @JunTaoLuo and I agreed w/ you @wtgodbe

Update condition

0572a25

wtgodbe merged commit 042e642 into release/3.1 Feb 20, 2020

wtgodbe deleted the johluo/always-the-ref-pack branch February 20, 2020 23:01

Conversation

JunTaoLuo commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dougbu Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

dougbu Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

dougbu Feb 19, 2020

Choose a reason for hiding this comment

Uh oh!

JunTaoLuo commented Feb 19, 2020

Uh oh!

wtgodbe commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wtgodbe commented Feb 19, 2020

Uh oh!

JunTaoLuo commented Feb 19, 2020

Uh oh!

wtgodbe commented Feb 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

JunTaoLuo commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

dougbu left a comment

Choose a reason for hiding this comment

Uh oh!

dougbu commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

JunTaoLuo commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

wtgodbe commented Feb 20, 2020

Uh oh!

JunTaoLuo commented Feb 20, 2020

Uh oh!

dougbu commented Feb 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

JunTaoLuo commented Feb 19, 2020 •

edited

Loading

wtgodbe commented Feb 19, 2020 •

edited

Loading

wtgodbe commented Feb 19, 2020 •

edited

Loading