search

NuGet / Home

Repo for NuGet Client issues
Other
1.5k stars 252 forks source link

[Bug]: Centralized Package Versioning causes NU1008 from included .targets files using non-centralized PackageReferences #11967

Open dtarjeft opened 2 years ago

dtarjeft commented 2 years ago

NuGet Product Used

dotnet.exe, MSBuild.exe, NuGet.exe

Product Version

dotnet 6.0.302, msbuild 16.11.2.50704, nuget 6.2.1.2

Worked before?

New issue encountered in upgrade process

Impact

I'm unable to use this version

Repro Steps & Context

  1. Create a C# project.
  2. Set up Centralized Package Versioning
  3. Install a package that contains a .targets file that has a PackageReference in it. Example of a .targets file that causes the issue: image
  4. Attempt to build.

What I expected: A build that successfully handles the PackageReference's .targets' dependencies.

What I got: NU1008, due to the referenced target file having declared a version.

This is a blocker for my team's adoption of CPVM, as any build helpers we have installed become build stoppers.

Verbose Logs

No response

nkolev92 commented 2 years ago

Hey @dtarjeft

Packages must not include restore affecting items/props in the build props and targets.

I imagine this isn't something you are seeing on the commandline just in Visual Studio?

I just created a pull request improving the docs and specifically calling out scenarios such as this: https://github.com/NuGet/docs.microsoft.com-nuget/pull/2807.

dtarjeft commented 2 years ago

@nkolev92 Both command line and VS show NU1008. On the command line, it fails the build. In VS, it causes the projects to reload continuously.

nkolev92 commented 2 years ago

I synced offline with @dtarjeft.

The target is not coming from a package, but rather just shared infrastructure.

nkolev92 commented 2 years ago

Team Triage: @jeffkl can you please take a look and let us know what your recommendations are.

jeffkl commented 2 years ago

@dtarjeft any PackageReference declared in your project or an import must not declare a version and instead a PackageVersion should be declared in Directory.Packages.props. If that will not work for your scenario, you can do one of the following:

dtarjeft commented 2 years ago

The first option isn't possible for my use case, as the package reference is in a referenced package. I'll give the second option a whack, although it seems to undercut the intent of the feature a little bit. Thank you for your help!

On Fri, Jul 29, 2022, 12:33 PM Jeff Kluge @.***> wrote:

@dtarjeft https://github.com/dtarjeft any PackageReference declared in your project or an import must not declare a version and instead a PackageVersion should be declared in Directory.Packages.props. If that will not work for your scenario, you can do one of the following:

— Reply to this email directly, view it on GitHub https://github.com/NuGet/Home/issues/11967#issuecomment-1199692539, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACJXPZPP5JEZ3T57EZATIPDVWQBVXANCNFSM53QFSQOQ . You are receiving this because you were mentioned.Message ID: @.***>

nkolev92 commented 2 years ago

@dtarjeft

The first option isn't possible for my use case, as the package reference is in a referenced package

That's not going to work on the commandline. Packages must not add PackageReference items in their own build targets as it'd make restore inconsistent. https://docs.microsoft.com/en-us/nuget/concepts/msbuild-props-and-targets#guidance-for-the-content-of-msbuild-props-and-targets

dtarjeft commented 2 years ago

@nkolev92 makes sense, I'll send that documentation to the publishers of the package. Is there a recommended approach for using a nuget reference in that context?

nkolev92 commented 2 years ago

If a package wants another package to be installed as well, it should declare it as a dependency.

Alternatively, that package could be changed to an SDK package that'd be installed in a completely different way, https://github.com/microsoft/MSBuildSdks, and not as a PackageReference.

gaborposz commented 2 years ago

Hi, I've also encountered the same issue lately, so let me add my 2 cents:

  1. The NU1008 error messages are not reliable, sometimes the compilation is successful, sometimes it's failing.
  2. I found a workaround (if anyone needs it): If the version is updated in the consumer project to nothing (e.g.: <PackageReference Update="ILRepack.Lib.MSBuild.Task" Version=""/>), then it can be centrally managed.
  3. I don't completely get the problem with the PackageReferences added by .props files. We use it for "integration & test related" stuff, that should not appear in the dependency tree anyway, e.g. MinVer, Microsoft.SourceLink.GitLab, Microsoft.NET.Test.Sdk, etc. Is it bad practice in this case too?
  4. Why isn't NU1008 a warning instead, and local versions are overriden by central versions?
jeffkl commented 2 years ago

@gaborposz can you supply a binlog by specifying the /bl command-line argument and attaching msbuild.binlog? Something else must be setting the Version and I'd like to understand more about what's going on.

gaborposz commented 2 years ago

@jeffkl: I sent the logs directly to you via e-mail because they might contain some restricted information. I managed to record logs (with dotnet build -bl command) both in the failing and in the succeeding case, but I had to do a compilation in VS2022 in order to make it succeed for the next try with dotnet build -bl. Without the VS2022 compilation it was failing all the time (as expected).

jeffkl commented 2 years ago

@gaborposz I'm not sure what's going on. The binlogs of course don't include every import so I can't see why but MSBuild is find PackageReference items with versions:

image

Is something in your codebase setting these versions?

gaborposz commented 2 years ago

@jeffkl: As I wrote, for "integration & test related" packages (e.g. MinVer, Microsoft.SourceLink.GitLab, Microsoft.NET.Test.Sdk, etc.) we set the versions centrally by a .props file, provided by the Woda.Build and Woda.Build.Test packages, e.g.:

  <ItemGroup>
    <!-- Source Link Support -->
    <PackageReference Include="Microsoft.SourceLink.GitLab" GeneratePathProperty="true" IncludeAssets="all" PrivateAssets="all" Version="1.1.1"/>
    <!-- SemVer 2.0 versioning -->
    <PackageReference Include="MinVer" GeneratePathProperty="true" IncludeAssets="all" PrivateAssets="all" Version="4.0.0"/>
  </ItemGroup>

Remark: The PrivateAssets="all" etc. setings are there because we want to hide these technical dependencies in our product NuGet packages.

Obviously, it was not compatible with the central version management, hence we fixed it like this:

  <ItemGroup>
    <!-- Source Link Support -->
    <PackageReference Include="Microsoft.SourceLink.GitLab" GeneratePathProperty="true" IncludeAssets="all" PrivateAssets="all" />
    <!-- SemVer 2.0 versioning -->
    <PackageReference Include="MinVer" GeneratePathProperty="true" IncludeAssets="all" PrivateAssets="all" />
  </ItemGroup>

  <!-- in case of no central dependency management is available, specify the version as well locally -->
  <ItemGroup Condition="'$(ManagePackageVersionsCentrally)'!='true'">
    <!-- Source Link Support -->
    <PackageReference Update="Microsoft.SourceLink.GitLab" Version="1.1.1" />
    <!-- SemVer 2.0 versioning -->
    <PackageReference Update="MinVer" Version="4.0.0" />
  </ItemGroup>

The questions are:

nkolev92 commented 2 years ago

I can help answer the 2nd question.

If a standard (library style) package has dependencies, then it can only declare those dependencies as package dependencies, never in the props/targets files. props/targets content will always be ignored on the commandline, but in VS, you may run into inconsistencies because of https://github.com/dotnet/project-system/issues/3734.

gaborposz commented 2 years ago

@nkolev92: Yes, we had the same idea, and it seems to be working now, but we needed some magic with the PackageReferences:

This way MinVer versioning (and a sort of other things) are applied to our product packages (as well as to WoDa.Build), but the technical dependencies (e.g. WoDa.Build, MinVer) are hidden in our product packages.

delanym commented 1 year ago

Seems it doesn't matter if Directory.Packages.props is added to the solution or not. If its in the directory it will be considered.

williambohrmann3 commented 7 months ago

Is there a workaround for this? We are hitting this in our monorepo, specifically the .NET MAUI project is being problematic.

zivkan commented 7 months ago

A workaround for what, exactly? Reading this issue, it seems that people had props/targets, either in their repo, or a package, that had a<PackageReference with a version. In this scenario NU1008.

The solution if it's props/targets in your own repo is to remove the Version= from the <PackageReference, and add a <PackageVersion for the package. Alternatively use the version override syntax. Packages need to declare their dependencies in the nuspec (which dotnet pack does automatically for packages used by the packed project), so no need for the package to include its own props/targets. And as mentioned, it's by design that props/targets in a command line solution restore can't modify the restore, such as adding additional <PackageReferences.

My interpreation of this issue is that CPM is working as designed and this issue should be closed. CPM has some onboarding costs, but the design reduces risk of unexpected behaviour by validating ambiguous scenarios and reporting an error instead.

williambohrmann3 commented 7 months ago

@zivkan tl;dr: we might be running into a different issue.

I agree, CPM took some time to implement but was worth it for our team. After digging into it more, it seems like there isn't an issue with our actual implementation of central package management. We are able to deploy our WPF and WinUI apps fine. Our .NET MAUI app is deploying to MacCatalyst / iOS on VS Code for Mac. Problem is with deploying our MAUI app to Windows and Android from Visual Studio. We are running into NU1008 errors and a persistent issue XA1018 with Android Manifest: Specified AndroidManifest file does not exist. These issues are often coupled together. We have some machines which are deploying just fine, others like my PC are no longer deploying with Visual Studio. Interestingly, on my PC, I am able to deploy just fine with a dotnet build command. Not sure what is actually causing this behavior, but it seems like a Visual Studio issue (I am on 17.9.5 as of time of writing).

JonDouglas commented 1 month ago

We need to figure out how to inform the parties that aren't conforming to CPM properly.

I have experienced this with dotnet new test recently.

error NU1008: Projects that use central package version management should not define the version on the PackageReference items but on the PackageVersion items: coverlet.collector;Microsoft.NET.Test.Sdk;xunit;xunit.runner.visualstudio.

I believe this issue happens because these SDK-style projects implicitly bring in these <PackageReference /> with specific versions.

The workarounds are really painful and I think we'll need to find MAUI and Tests contacts to resolve this on their end. Pinging @davidortinau (maui), @Redth (maui), @baronfel (who may know a test contact).

In other words, I should be able to use CPM w/ test and MAUI projects which disagree with the requirements CPM has today.

baronfel commented 1 month ago

cc @Evangelink for test-related issues.

This is something we hit in the .NET SDK itself as well, and in fact on the 23rd of this month I think the topic of the .NET SDK Partner Sync is this very topic - how should packages that are defined by tooling (.NET SDK, Workloads, MSBuild SDKs, etc) interoperate with the Lockfiles mechanisms. For the very short term, my thinking has been something like:

However, this is complex and irritating. When I've briefly chatted with @nkolev92 and @zivkan about this in the past, there have been a couple suggestions:

Somewhat related to this, NuGet-delivered MSBuild SDKs are a package too, but cannot be tracked in CPM as far as I am aware. I'm also not sure if they can participate in package source mapping.

zivkan commented 1 month ago

@baronfel I think that Jon was reporting a different issue. The packages listed in Jon's comment are not implicit packages added by an (MSBuild) SDK, they're PackageReferences hardcoded in the project template.

I'm quite confident there's at least one project template that works around this issue by having some kind of "post create" actions of running dotnet add package ..., and therefore letting NuGet decide if the PackageReference should have a version attribute or if versions are added to Directory.Packages.props.

JonDouglas commented 1 month ago

I ported the template to CPM, so it wasn't just out of the box. It should just work, but something is preventing it in certain workloads.

Here's some similar issues:

https://github.com/dotnet/sdk/issues/25797#issuecomment-1642788491