Add warning when implicit package reference is used #375
Conversation
| Text="The PackageReference to $(_AspNetCoreAllPackageName) is missing the Version attribute. Setting the default version to $(AspNetCoreAllRuntimeFrameworkVersion). See https://aka.ms/aspnetcore/2.1/metapackage-refs for more details about this warning." | ||
| Condition="'@(_AspNetCoreAllReference->Count())' == '1' AND '@(_ExplicitAspNetCoreAllReference->Count())' == '0'" /> | ||
| <Warning | ||
| Text="The PackageReference to $(_AspNetCoreAppPackageName) is missing the Version attribute. Setting the default version to $(AspNetCoreAppRuntimeFrameworkVersion). See https://aka.ms/aspnetcore/2.1/metapackage-refs for more details about this warning." |
There was a problem hiding this comment.
The link https://aka.ms/aspnetcore/2.1/metapackage-refs doesn't currently exist. Is this a placeholder?
There was a problem hiding this comment.
just made it. Placeholder for now while we write docs...assuming this gets shiproom approval of course.
| @@ -124,15 +124,13 @@ Copyright (c) .NET Foundation. All rights reserved. | |||
| Update="$(_AspNetCoreAllPackageName)" | |||
| Condition="'@(_AspNetCoreAllReference->Count())' == '1' AND '@(_ExplicitAspNetCoreAllReference->Count())' == '0'"> | |||
| <Version>$(AspNetCoreAllRuntimeFrameworkVersion)</Version> | |||
| <IsImplicitlyDefined>true</IsImplicitlyDefined> | |||
| <PrivateAssets>All</PrivateAssets> | |||
There was a problem hiding this comment.
This flag might also need some attention, though I can't think of any issues of keeping PrivateAssets="All" while removing IsImplicitlyDefined. The difference is now that we want users to define an explicit version, we won't be adding PrivateAssets="All" by default which would be a drastic departure from the current release.
There was a problem hiding this comment.
So, expected changes from this are:
Before:
- Explicit Microsoft.AspNetCore.App reference was required in test projects as no assets flow across P2P
- Microsoft.AspNetCore.App does not appear in the generated .nuspec
After:
- Explicit Microsoft.AspNetCore.App reference is recommended in test projects as the NETPlatformLibrary setting will not flow across P2P
- Microsoft.AspNetCore.App appears in the generated .nuspec
I wonder if we should (separately) attempt to address the transitive flow of NETPlatformLibrary. We may be able to perform some MSBuild black magic to make that happen (cc @dsplaisted - thoughts?)
As for the .nuspec generation... I don't think we care at the point. Producing a .nupkg of a website is not really an important scenario.
There was a problem hiding this comment.
The general problem with the flow of NETPlatformLibrary is that build assets (.props and .targets in a NuGet package) don't flow by default. That's IMO an unfortunate design choice, but I think it's not likely to change now since changing it could break things.
What's the impact of the NETPlatformLibrary not flowing?
If we had to, we could probably come up with a workaround for the NETPlatformLibrary not flowing. However, it would probably be hacky, and I expect the issue will be fixed in 3.0 anyway, so I'd avoid trying to work around this in 2.x unless the impact is sufficiently severe.
There was a problem hiding this comment.
The impact of NETPlatformLibrary not flowing is that test projects, or anyone that has a common "web" project, will not end up using the AspNetCore.App shared framework. Instead, they run as if running on packages. This means you may end up with a web app which gets patch fixes via rollforward, and test projects which do not because they run on binaries from nupkgs.
The PrivateAssets="all" behavior avoided this problem because you were forced to add explicit AspNetCore.App package refs to all projects, thus avoiding this problem.
There was a problem hiding this comment.
So I guess the consensus here is to let the users manage privateassets themselves for explicit references? And continue adding privateassets=all for implicit references and display the warning.
|
|
| @@ -7,7 +7,7 @@ | |||
| <PropertyGroup Condition=" '$(PackageVersion)' == '' "> | |||
|
|
|||
| <!-- If $(PB_VersionStamp) is empty and $(PB_IsStable) is not true, then a default PackageBuildQuality will be added. --> | |||
| <PackageReleaseVersion Condition="'$(PackageReleaseVersion)' == '' ">2.1.400</PackageReleaseVersion> | |||
| <PackageReleaseVersion Condition="'$(PackageReleaseVersion)' == '' ">2.1.402</PackageReleaseVersion> | |||
There was a problem hiding this comment.
cc @vijayrkn - is this the right branding?
There was a problem hiding this comment.
Yes, this is correct for this branch.
natemcmaster
force-pushed
the
namc/implicit-pkg
branch
from
c618bdf to
ec804d6
Compare
|
Rebased and retriggered PR checks. I plan to merge this once those pass. |
|
@vijayrkn all of the tests on .NET Framework are failing. Known issue?
|
|
All the tests passed while running on core This PR should be good to merge. I will take a look at why full framework ones are failing. |
See discussion on dotnet/aspnetcore#3292
Changes:
"The PackageReference to Microsoft.AspNetCore.App is missing the Version attribute. Setting the default version to 2.1.1. See https://aka.ms/aspnetcore/2.1/metapackage-refs for more details about this warning."