Skip to content
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

Single Transport Package for aspnetcore #47684

Merged
merged 8 commits into from
Feb 8, 2021
Merged

Single Transport Package for aspnetcore #47684

merged 8 commits into from
Feb 8, 2021

Conversation

Anipik
Copy link
Contributor

@Anipik Anipik commented Jan 30, 2021

  • Move non extensions libraries to the transport package
  • change the transport package name
  • update the servicing policy
  • move the build of transport package to just allconfig leg

depends on dotnet/arcade#6886 to bin place the .xml files

@ghost
Copy link

ghost commented Jan 30, 2021

Tagging subscribers to this area: @safern, @ViktorHofer
See info in area-owners.md if you want to be subscribed.

Issue Details
  • Move non extensions libraries to the transport package
  • change the transport package name
  • update the servicing policy
  • move the build of transport package to just allconfig leg

depends on dotnet/arcade#6886 to bin place the .xml files

Author: Anipik
Assignees: -
Labels:

area-Infrastructure-libraries

Milestone: -

Copy link
Member

@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.

Is the plan to produce a separate transport package for WindowsDesktop❔ I'm pretty sure they also (like dotnet/aspnetcore) special-case runtime packages that ship in their targeting pack.

@@ -29,9 +29,20 @@ If this library ships both inbox on a platform and in its own library package th
```
Where the `AssemblyVersion` is set to the old version before updating. To determine if the library ships inbox you can look at for `InboxOnTargetFramework` item groups or `TreatAsOutOfBox` suppressions in the pkgproj for the library.

If the library is part of a Aspnetcore or .NET targeting pack then we cannot increment the assembly version. For Aspnetcore, You can examine the ```<IsAspNetCoreApp>true</IsAspNetCoreApp>``` property in the library`s ```Directory.Build.Props```
Copy link
Member

Choose a reason for hiding this comment

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

What about WindowsDesktop❔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericstj who will be the best person to tell us on what packages are used by windowsDesktop in their targeting pack ?

Copy link
Member

Choose a reason for hiding this comment

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

I don't know whom, but you could try spot checking them in the targeting pack installed in your dotnet SDK.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Should the implicit value (Microsoft.Extensions.*) be changed to be explicit?

<IsAspNetCoreApp Condition="$(MSBuildProjectName.StartsWith('Microsoft.Extensions.'))">true</IsAspNetCoreApp>

@Anipik
Copy link
Contributor Author

Anipik commented Feb 2, 2021

Should the implicit value (Microsoft.Extensions.*) be changed to be explicit?

do you mean setting IsAspNetCoreApp in directory.build.props for extensions project as well ?

@ViktorHofer
Copy link
Member

do you mean setting IsAspNetCoreApp in directory.build.props for extensions project as well ?

Right. Currently we assume that all libraries which name start with Microsoft.Extensions.* are are aspnetcore libraries which isn't true for some. My question is about if we should stop doing that and just add the property to libraries that are relevant to aspnetcore.

@Anipik
Copy link
Contributor Author

Anipik commented Feb 3, 2021

Right. Currently we assume that all libraries which name start with Microsoft.Extensions.* are are aspnetcore libraries which isn't true for some. My question is about if we should stop doing that and just add the property to libraries that are relevant to aspnetcore.

yes i think we should do that, because as we are planning on bring the hosting packages and those packages i dont think will be part of this package.

Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

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

Are all of the libs where you set IsAspNetCoreApp needed by aspnetcore?

@Anipik
Copy link
Contributor Author

Anipik commented Feb 5, 2021

@dougbu are there any extensions projects here that are not needed by the aspnetcore

@dougbu
Copy link
Member

dougbu commented Feb 5, 2021

@dougbu are there any extensions projects here that are not needed by the aspnetcore

The concise list of all assemblies expected in Microsoft.AspNetCore.App.Ref (what we'll get from the transport package) is https://github.com/dotnet/aspnetcore/blob/43a348b22468be9ed46ff1681cc113c9598b9f25/src/Framework/test/TestData.cs#L155-L288. Note that list includes more from dotnet/runtime than Microsoft.Extensions.* as well as our own ref/ assemblies.

@@ -2,5 +2,6 @@
<Import Project="..\Directory.Build.props" />
<PropertyGroup>
<PackageDescription>Internal package for sharing Microsoft.Extensions.Hosting.HostFactoryResolver type.</PackageDescription>
<IsAspNetCoreApp>true</IsAspNetCoreApp>
Copy link
Member

Choose a reason for hiding this comment

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

I don't see this library in aspnetcore's list. @dougbu is this one needed?

Copy link
Member

Choose a reason for hiding this comment

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

Right, this isn't a library. aspnetcore uses the Microsoft.Extensions.HostFactoryResolver.Sources package but (of course) doesn't ship an assembly with this name.

Copy link
Member

Choose a reason for hiding this comment

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

@Anipik that's that package, so you want to keep <IsAspNetCoreApp> as that's the source package that aspnetcore needs.

@Anipik Anipik merged commit a66b4e3 into dotnet:master Feb 8, 2021
@Anipik Anipik deleted the servicing branch February 8, 2021 08:03
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Feb 23, 2021
- handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
- remove `$(...V0Version)` / `%(RTMVersion)` workarounds
- remove mention of System.Security.AccessControl package; only need transitive references to that
- confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Feb 24, 2021
- handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
- remove `$(...V0Version)` / `%(RTMVersion)` workarounds
- remove mention of System.Security.AccessControl package; only need transitive references to that
- confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Feb 25, 2021
- handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
- remove `$(...V0Version)` / `%(RTMVersion)` workarounds
- remove mention of System.Security.AccessControl package; only need transitive references to that
- confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Feb 26, 2021
* React to dotnet/runtime#47684, a new transport package
  - handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
  - remove `$(...V0Version)` / `%(RTMVersion)` workarounds
  - remove mention of System.Security.AccessControl package; only need transitive references to that
  - confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly

* Remove unnecessary `using`s
  - namespace previously available due to Microsoft.Extensions.Internal.Transport dependency
  - no need for Microsoft.AspNetCore.Internal.Transport dependency

* nit: Clean up "Extensions" naming in Ref project

* Correct targeting pack content
  - intersection w/ `@(ExternalAspNetCoreAppReference)` items added nothing
  - excluded assemblies now included in transport package
dougbu added a commit to mmitche/AspNetCore that referenced this pull request Feb 26, 2021
- back-port from main / dotnet#30405

* React to dotnet/runtime#47684, a new transport package
  - handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
  - remove `$(...V0Version)` / `%(RTMVersion)` workarounds
  - remove mention of System.Security.AccessControl package; only need transitive references to that
  - confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly

* Remove unnecessary `using`s
  - namespace previously available due to Microsoft.Extensions.Internal.Transport dependency
  - no need for Microsoft.AspNetCore.Internal.Transport dependency

* nit: Clean up "Extensions" naming in Ref project

* Correct targeting pack content
  - intersection w/ `@(ExternalAspNetCoreAppReference)` items added nothing
  - excluded assemblies now included in transport package
dougbu added a commit to dotnet/aspnetcore that referenced this pull request Feb 26, 2021
…#30461)

- back-port from main / #30405

* React to dotnet/runtime#47684, a new transport package
  - handle Microsoft.Extensions.Internal.Transport -> 'Microsoft.AspNetCore.Internal.Transport name change
  - remove `$(...V0Version)` / `%(RTMVersion)` workarounds
  - remove mention of System.Security.AccessControl package; only need transitive references to that
  - confirm content of Microsoft.AspNetCore.Internal.Transport package
    - missing Microsoft.Extensions.DependencyModel assembly

* Remove unnecessary `using`s
  - namespace previously available due to Microsoft.Extensions.Internal.Transport dependency
  - no need for Microsoft.AspNetCore.Internal.Transport dependency

* nit: Clean up "Extensions" naming in Ref project

* Correct targeting pack content
  - intersection w/ `@(ExternalAspNetCoreAppReference)` items added nothing
  - excluded assemblies now included in transport package

Co-authored-by: Doug Bunting <[email protected]>
@ghost ghost locked as resolved and limited conversation to collaborators Mar 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants