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

Remove support for the implicit package version of Microsoft.AspNetCore.App #3292

Closed
natemcmaster opened this issue Jul 3, 2018 · 44 comments
Assignees
Labels
✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. investigate

Comments

@natemcmaster
Copy link
Contributor

natemcmaster commented Jul 3, 2018

Microsoft.AspNetCore.App, unlike normal NuGet packages, represents a shared framework. This means the package provides:

  • A) The minimum expected version of of the ASP.NET Core shared runtime (found in C:\Program Files\dotnet\shared\Microsoft.AspNetCore.App$(PackageVersion))
  • B) A baseline of assemblies versions which the SDK expects to find in the shared runtime
  • C) References assemblies for the compiler
  • D) Implementations assemblies for self-contained deployments

There is tension between these as we want the lowest possible version of (A) to simplify deployment concerns, the highest version of (D) for security updates and patch fixes, and a version of (B) and (C) compatible with both (A) and (D).

We attempted to solve this tension in 2.1.0 by optimizing for (A) and hiding the package version from users and keeping it low, pinned to 2.1.0. That made it difficult to use packages which shared dependencies with the shared framework. In 2.1.1, we tried to fix this by easing off the restrictions and allowing package upgrades, but means users unintentionally upgrade out of the baseline (B), and end up with redundant copies of binaries in their publish output.

After lots of design discussion reviewing half a dozen alternatives, we think it will be better in the long run to back to reverse course and encourage users to use an explicit version on the PackageReference to Microsoft.AspNetCore.App, and it regularly update it to latest.

Proposed changes

  • Update dotnet new templates to include the latest Microsoft.AspNetCore.App package version in the .csproj file.

  • Update Microsoft.NET.Sdk.Web to warn when Microsoft.AspNetCore.App is missing a package version.

    • Text: The package reference to Microsoft.AspNetCore.App is missing the Version attribute. Defaulting to Version="2.1.1"..
  • Remove "IsImplicitlyDefined" = true from Microsoft.NET.Sdk.Web. This unblocks the NuGet UI from displaying updates.
    Before:

    After:

Downsides

One of the primary reasons for making the version implicit was to simplify deployment. As history since 2.0.0 has taught us, our users that updating a PackageReference should be sufficient to use the newest version of ASP.NET Core. However, there is one more step required: users need to install a compatible shared runtime. This step is not obvious from the NuGet Package UI in VS, and error messages indicating the missing runtime are sometimes hidden (e.g. IIS Express, Test Explorer).

Proposed enhancement

[Nate: July 9]: this was not implemented, and we are not planning to implement right now.

  • Update Microsoft.NET.Sdk to produce a warning when compiling for a .NET Core shared framework which is not present on the machine (cc @dsplaisted @nguerrera).
    • Text: This project is compiling for Microsoft.AspNetCore.App 2.1.3, but a compatible runtime was not found. .NET Core runtimes can be installed from https://aka.ms/dotnet-download
      image

cc @DamianEdwards @davidfowl @JunTaoLuo @vijayrkn

@vijayrkn
Copy link

vijayrkn commented Jul 3, 2018

/cc @mlorbetske

@nguerrera
Copy link
Contributor

Cc @steveharter. I'm not sure how we would implement that warning. We should tread carefully before putting host internals into build logic.

Fundamentally, I'm always wary of build trying to decide things based on the current machine. We have a long history of tooling bugs from doing just that.

But I do think there needs to be a diagnostic of some kind somewhere.

@nguerrera
Copy link
Contributor

Isn't a bug that IIS express and test explorer are hiding the clear runtime message?

@natemcmaster
Copy link
Contributor Author

@steveharter @nguerrera how much work do you think it would be to implement something like hostfxr_resolve_sdk, but instead hostfxr_resolve_shared_runtime?

Isn't a bug that IIS express and test explorer are hiding the clear runtime message?

Yes, yes it is. We should work with VSTest and IIS Express to see if we can improve this, too.

@natemcmaster natemcmaster added the Servicing-consider Shiproom approval is required for the issue label Jul 3, 2018
@nguerrera
Copy link
Contributor

Writing the function is easy enough but there are some deployment issues. Chief among them is that we often run in 32 bit msbuild but dotnet installed is 64 bit. Sdk resolver gets around this by carrying its own hostfxr, now we'd need access to same during build. It's messy already and I don't have a great feeling about more interaction between build and hostfxr after the experience so far.

@nguerrera
Copy link
Contributor

Adding @jeffschwMSFT

@natemcmaster
Copy link
Contributor Author

It's messy already and I don't have a great feeling about more interaction between build and hostfxr after the experience so far.

Presumably we can piggy back off the messiness you've already wrangled, right? From what I can tell, to implement this diagnostic we only need to read a runtimeconfig.json and check if a runtime exists to satisfy the runtime framework name and version. Are there complications in doing this that would make the messiness even worse?

@nguerrera
Copy link
Contributor

nguerrera commented Jul 3, 2018

Not really. The resolver lives in its own world with its private hostfxr and no knowledge of the build tasks. And the build tasks have no knowledge of the resolver. So we'd either have to duplicate this messiness in the build tasks (yuck) or come up with some other activation scheme. I'd push for the latter but I don't have a solution in mind for it yet. It's definitely something that would need careful thought. I hope we don't decide to rush on it for 2.1.3.

@nguerrera
Copy link
Contributor

And I'm still concerned about the general idea of warning about what won't run with the dotnet on PATH. It is valid to publish from an environment that can't run a program to an environment that can.

A suppressible warning isn't the worst sin of this kind. It pales in comparison to say RAR thinking the presence of an assembly in the build machine's GAC is relevant to whether it should be copied. But bad decisions like that give me pause whenever we discuss anything that ties the runtime environment of the build to the runtime environment of the program being built.

@rockerinthelocker
Copy link

@natemcmaster , @coolcsh , While you're at it, #3257 might need the same treatment.

@poke
Copy link
Contributor

poke commented Jul 3, 2018

As much as I dislike this going back and forth, I believe making the version explicit again is for the best to get rid of all the problems that came with the shared framework.

@natemcmaster The “that made it difficult” link in the opening post has the wrong link.

@steveharter
Copy link
Member

@natemcmaster can you elaborate on this:

In 2.1.1, we tried to fix this by easing off the restrictions and allowing package upgrades, but means users unintentionally upgrade out of the baseline (B), and end up with redundant copies of binaries in their publish output.

Are the redundant "binaries" assemblies, or some other binaries? Did this result in an error of some form?

For the proposed new API, to get a 100% accurate result it requires us to read the app's runtimeconfig.json in order to pull in the same roll-forward settings and potentially in the future resolve other references to the same framework. An alternative (< 100%) would be to just do a dotnet --info and parse the output (possibly json) and apply roll-forward semantics probably just using >=

@jeffschwMSFT
Copy link
Member

Text: This project is compiling for Microsoft.AspNetCore.App 2.1.3, but a compatible runtime was not found. .NET Core runtimes can be installed from https://aka.ms/dotnet-download

What does it mean to be compatible? It seems like there are practical considerations to get this right, which makes me think that it will be hard for our customers to get right as well. Is it as simple as 'matches .'?

@natemcmaster
Copy link
Contributor Author

The “that made it difficult” link in the opening post has the wrong link.

@poke Updated -> aspnet/Universe#1180

What does it mean to be compatible?

@jeffschwMSFT would an error message like this be better?

"This project is compiling for the Microsoft.AspNetCore.App 2.1.3 shared runtime, but a stable version of this runtime that is higher than or equal to 2.1.3 but no greater than 3.0.0, could not be found. Your app may fail to start unless you install the latest 2.1.* patch version of the Microsoft.AspNetCore.App shared runtime from https://aka.ms/dotnet-download"

@natemcmaster can you elaborate on this:

In 2.1.1, we tried to fix this by easing off the restrictions and allowing package upgrades, but means users unintentionally upgrade out of the baseline (B), and end up with redundant copies of binaries in their publish output.

Are the redundant "binaries" assemblies, or some other binaries? Did this result in an error of some form?

At the moment, we're not seeing runtime errors, but we're seeing the published output of apps starting to grow as we patch packages that overlap dependencies with the shared framework. In theory, it could happen with native binaries, too, such as sni.dll. We want to avoid putting users in a state in which they are overriding the shared runtime with app-local copies.

You can see this clearly in our daily builds. This project has 16 additional assemblies in publish output which are normally part of the Microsoft.AspNetCore.App shared runtime.

<Project Sdk="Microsoft.NET.Sdk.Web">
  <PropertyGroup>
    <TargetFramework>netcoreapp2.1</TargetFramework>
    <RestoreAdditionalProjectSources>https://dotnet.myget.org/F/aspnetcore-dev/api/v3/index.json</RestoreAdditionalProjectSources>
  </PropertyGroup>
  <ItemGroup>
    <PackageReference Include="Microsoft.AspNetCore.App" />
    <PackageReference Include="Microsoft.EntityFrameworkCore.SQLite" Version="2.1.2-rtm-30877" />
    <PackageReference Include="Microsoft.Extensions.Caching.Redis" Version="2.1.2-rtm-30877" />
  </ItemGroup>
</Project>

The way a user would fix this is to upgrade Microsoft.AspNetCore.App.

    <PackageReference Include="Microsoft.AspNetCore.App" Version="2.1.3-rtm-30877" />

to get a 100% accurate result

@steveharter IMO I'm okay with less than 100% accuracy in this warning. In the event there is an error in using the new hostfxr API or dotnet --info, I would just skip this check since there will be a runtime error anyways. The main point of the idea is to provide an early diagnostic, soon after a user upgrades their package references in NuGet, so they will know that they might have introduced an issue by upgrading Microsoft.AspNetCore.App.

@jeffschwMSFT
Copy link
Member

"This project is compiling for the Microsoft.AspNetCore.App 2.1.3 shared runtime, but a stable version of this runtime that is higher than or equal to 2.1.3 but no greater than 3.0.0, could not be found. Your app may fail to start unless you install the latest 2.1.* patch version of the Microsoft.AspNetCore.App shared runtime from https://aka.ms/dotnet-download"

It does, but I fear it too does not cover all the cases. If for instance, a pre-release was used then it would be slightly different rollforward logic. I don't have a great suggestion. But I dislike that our rollforward logic is bleeding into this experience. Between the two, I like this one more.

@dsplaisted
Copy link
Member

How about leaving the message as is but adding a link with the detailed explanation for how version selection works? Documentation for that is in PR here: dotnet/docs#6060

@natemcmaster
Copy link
Contributor Author

Yeah, we should add a link to this warning.

By the way, I agree with all of your concerns about a build-time diagnostic of a runtime behavior which may or may not be accurate.

But, I'm more concerned about how often users will find their applications failing to launch after updating their NuGet packages. I think we can address this with better tooling, and an MSBuild warning is the only tooling option we have at our disposal right now.

natemcmaster added a commit to aspnet/Templating that referenced this issue Jul 3, 2018
As discussed in dotnet/aspnetcore#3292, we're reversing course on the implicit package version feature. This adds a package version for Microsoft.AspNetCore.App to the `dotnet new` and VS templates.
@natemcmaster natemcmaster added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jul 6, 2018
@natemcmaster
Copy link
Contributor Author

We talked about this in shiproom and a handful of design meetings. The plan is to move forward with the changes to the project templates and web SDK tweaks.

We'll take more time to investigate enhancements to tooling to help users identify when their .NET Core console or web projects are building against a runtime framework which is not installed on the machine.

@normj
Copy link

normj commented Jul 7, 2018

Adding another voice, in this case from AWS, that this setting needs to be explicit. This is breaking our customers using ASP.NET Core because they update their dev/build machines but AWS services usually get updated about a month after a new .NET Core release. Right now this seems like a major reversal back to the .NET Core 1.x days when every patch release would cause pain.

natemcmaster added a commit to dotnet/AspNetCore.Docs that referenced this issue Jul 10, 2018
Rick-Anderson pushed a commit to dotnet/AspNetCore.Docs that referenced this issue Jul 10, 2018
… for AspNetCore.App/All (#7540)

* Remove instructions about using the implicit PackageReference feature for AspNetCore.App/All

cref dotnet/aspnetcore#3292

* Minor language updates

* Remove extra line space

* Update metapackage-app.md
@natemcmaster natemcmaster modified the milestones: 2.1.3, 2.2.0-preview1 Jul 18, 2018
@natemcmaster natemcmaster added investigate and removed Servicing-approved Shiproom has approved the issue labels Jul 18, 2018
@natemcmaster
Copy link
Contributor Author

Hey all, status update. I'm reopening this issue because we had to revert some changes. This call was made after lots and lots of discussion with our partner teams and management. We still intend to remove the versionless PackageRef, but we haven't landed on a replacement for it. Until we do, our templates and tools will continue to support the versionless PackageRef for Microsoft.AspNetCore.App.

One candidate for replacement is being discussed here: #3307. There are several other ideas to address the problem, including those mentioned above in this thread. We will continue to explore those for the 2.2 timeframe, and possibly for a 2.1 SDK (no promises.) In the meantime, I'll keep you posted.

Side note: the default implicit of Microsoft.AspNetCore.App stays pinned at 2.1.1, much like how the implicit package ref to Microsoft.NETCore.App stays pinned to 2.1.0. This is intentional to make your app compatible with more deployment environments. To get security patches, we recommend installing the new runtime version to your deployment servers and rely on the patch-rollforward behavior to run your app on the newer framework version.

Thanks,
Nate

@natemcmaster natemcmaster reopened this Jul 19, 2018
@poke
Copy link
Contributor

poke commented Jul 20, 2018

our templates and tools will continue to support the versionless PackageRef

Support is one thing, but the templates revert means that the version-less PackageReference is currently still the recommended way. Is that correct? Should we instruct users to keep using a version-less reference for now, or is there some other guidance?

I’m thinking about this comment of yours which recommended the versioned reference in preparation for the upcoming changes.

@natemcmaster
Copy link
Contributor Author

version-less PackageReference is currently still the recommended way. Is that correct?

Yes, until we can land SDK changes updates. When/if that happens, we'll update our guidance and templates.

That comment, by the way, is made in the context of a user who was still on the 2.1.300 SDK (implicit package version == 2.1.0) and they were fighting NU1107 errors. NU1107 goes away with the 2.1.1 metapackage, which is a baseline we intend to keep for the lifetime of the 2.1 release.

The scenarios in which I still recommend adding a version to the package ref are:

  • you want to lift the minimum required version of the shared runtime to 2.1.x
  • you are writing a test project and don't want to use Microsoft.NET.Sdk.Web
  • you need to use a patched version of something newer than what is in the 2.1.1 shared runtime, and upgrading would have cause you to get a copy-local version instead of using the one from the shared runtime.

@jeffschwMSFT
Copy link
Member

cc @swaroop-sridhar

@natemcmaster
Copy link
Contributor Author

After lots of discussion and experimentation, I'm closing this issue as we do not plan to make changes in 2.1 or 2.2 to address this issue. We are going to stick with original design; versionless PackageReference is the recommended way to use Microsoft.AspNetCore.App. I will update our docs and guidance to match this recommendation.

We implemented and tested alternatives ways to reference the Microsoft.AspNetCore.App shared framework. While these might have been accepted if we had done them in the first place, we did not feel benefits of switching approaches outweighed the costs. This was especially true as we began to design 3.0. In 3.0, we plan to rework the way shared frameworks are referenced in projects. Rather than introducing more churn, we're going to make some improvements in the 2.2 SDK for using the versionless PackageReference (see dotnet/sdk#2533), and will focus our efforts on making 3.0 better. We'll share more details about our plans for 3.0 soon.

cc @DamianEdwards

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
✔️ Resolution: Won't Fix Resolved because we decided not to change the behavior reported in this issue. investigate
Projects
None yet
Development

No branches or pull requests