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

[WIP] 2.2: Add support for referencing Microsoft.AspNetCore.App as a FrameworkReference #2501

Closed

Conversation

natemcmaster
Copy link
Contributor

@natemcmaster natemcmaster commented Aug 29, 2018

For your consideration. We need to get more agreement from the PMs first, but wanted to get @dsplaisted to take a look since he made similar changes in the master branch.

Changes:

  • Move support for the version-less PackageReference from Microsoft.NET.Sdk.Web into the base SDK
  • Add support for specifying Microsoft.AspNetCore.App/All as <FrameworkReference> items in 2.1 and 2.2
  • Issue a warning when a <PackageReference> to "Microsoft.AspNetCore.App" exists.
    image
  • Make it an error to use unrecognized FrameworkReference items

image

  • Add support for using RuntimeFrameworkVersion to control both NETCore.App and AspNetCore.App/All implicit packages. It is expected that these package versions should always align.

Sample usage:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <!-- Optional -->
    <RuntimeFrameworkVersion>2.1.3</RuntimeFrameworkVersion>
  </PropertyGroup>

  <ItemGroup>
    <FrameworkReference Include="Microsoft.AspNetCore.App" />
  </ItemGroup>

</Project>

TODO:

  • Get agreement from @DamianEdwards who is OOF till next week.
  • Make FrameworkReference transitive accross ProjRef's, maybe?

that the SDK knows about, while framework-dependent apps will target the ".0" patch (and roll forward to the latest
patch installed at runtime).

The TargetLatestAspNetCoreRuntimePatch property can be set to true or false to explicitly opt in or out of the logic
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to opt-in on latest only to one particular framework reference but not others? Just wondering if all should key off of the TargetLatestRuntimePatch property.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We added the separate flag for source-build SDKs. The RedHat SDK sets TargetLatestAspNetCoreRuntimePatch=true because users in that environment run from NuGet packages, not a shared framework. I think it's worth dropping 3.0, but we need it in 2.x.

@livarcocc
Copy link
Contributor

Me, @DustinCampbell and @dsplaisted were discussing exactly that yesterday and how we would like for ASP.NET to also use Microsoft.Net.SDK here with the FrameworkReference. So, it is great to see this PR. However, do we know what will break when the user add the asp.net framework reference without the web sdk?

@dsplaisted
Copy link
Member

We'd remove the version selection logic from the Web SDK at the same time, so it would continue to work.

@natemcmaster
Copy link
Contributor Author

FYI @livarcocc @dsplaisted @nguerrera @DamianEdwards I'm out on vacation for the next 2 weeks. I'm in favor of adding FrameworkReference support to the 2.2 SDK, but I won't be around to drive this until we get close to the cutoff for preview 3. If this is something we want to get in for preview 3, it would be good to get agreement soon.

cc @vijayrkn @ryanbrandenburg - if this goes in, we will need to update aspnet templates and delete the duplicated code from the websdk, but those are easy changes to make.

@dasMulli
Copy link
Contributor

dasMulli commented Sep 7, 2018

Make FrameworkReference transitive accross ProjRef's, maybe?

+1 since this will fix integration tests. (dotnet/aspnetcore#3156, https://stackoverflow.com/questions/50401152) without the need for the test project to use the web sdk (icon, project system etc.)

There's no chance to also use this reference to remove transitive references to individual packages that are also contained in the framework, right? e.g. a 2.2.1/2.2.2 reference to something a library would reference (because the author will just "update all nuget packages" without knowing about the implications) shouldn't deploy its dll with the app built using the 2.2.0 framework package.

Btw, this may also happen for windows-related things in 3.0 where I believe the winforms/wpf things will use a shared framework as well (guessing form various GH activity). They would also have this problem for things that are also in the compatibility pack for non-winforms/wpf applications (that either use packages with non-windows implementations or have an if(windows) check)

@dsplaisted
Copy link
Member

I like the idea of supporting FrameworkReference for this in 2.2. However, I'd prefer using a task to add the right version PackageReference instead of all of the logic in MSBuild evaluation. That will be a lot easier to read and maintain.

Make FrameworkReference transitive accross ProjRef's, maybe?

We plan to do this for .NET Core 3.0, but that's probably too much for .NET Core 2.2.

@DamianEdwards
Copy link
Member

I'm supportive of this, but just to clarify, ASP.NET Core projects need to continue to use the Web SDK due to different defaults for globs, publishing targets, etc. A test project that references an ASP.NET Core project could use the base SDK, and <FrameworkReference Include="Microsoft.AspNetCore.App" /> and work properly though, AFAIK.

How do we get to closure on this for preview3?

@livarcocc
Copy link
Contributor

We will take the implicit version change for 2.2, but I don't believe we have landed all the details of FrameworkReferences in 3.0 to be able to rush and pull it in for 2.2.

The implicit version change should solve one big category of issues.

@DamianEdwards I have asked @dsplaisted to take care of the implicit version changes for 2.2, given that @natemcmaster is out right now. Would this work for you?

@DamianEdwards
Copy link
Member

@livarcocc that's fair. Given that, I'd also like us to add the warning then for when the <PackageReference Include="Microsoft.AspNetCore.App/All" /> has a version specified. We continue to get issue reports from customers because they're setting the version specifically (NuGet UI will offer it to them) then deploying to Azure or other places that don't have the patch yet.

@livarcocc
Copy link
Contributor

Sound's good. @dsplaisted when working on porting @natemcmaster's changes for the implict versions, please also work on the warning that @DamianEdwards mentions above.

@dsplaisted
Copy link
Member

I've started work on moving the implicit PackageReference version into the SDK: #2533

However, we will also need to remove the version selection logic from the Web SDK at the "same" time.

@DamianEdwards Who from ASP.NET can work with me on the ASP.NET side of this?

@DamianEdwards
Copy link
Member

@dsplaisted that's a question for @muratg or @Eilon

@muratg
Copy link

muratg commented Sep 14, 2018

@dsplaisted What repo does the Web SDK logic you mention live in? (cc @vijayrkn in case this is related to his areas)

@dsplaisted
Copy link
Member

@muratg It's in aspnet/websdk. Specifically, I think most of it is here

@vijayrkn
Copy link
Contributor

I am oof today. John and Nate has made most of the changes for this in the websdk.

@muratg
Copy link

muratg commented Sep 14, 2018

cc @natemcmaster & @JunTaoLuo

@livarcocc
Copy link
Contributor

Closing this PR in favor of #2533.

@livarcocc livarcocc closed this Sep 19, 2018
@natemcmaster natemcmaster deleted the framework-ref-22 branch September 24, 2018 16:57
@natemcmaster natemcmaster restored the framework-ref-22 branch September 24, 2018 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants