NuGet / Home

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

PackageReference should support DevelopmentDependency metadata #4125

Closed AArnott closed 6 years ago

AArnott commented 7 years ago

msbuild /t:pack fails when building a stable versioned package that depends on an unstable one, even though that unstable one is a DevelopmentDependency:

  1. Create .NET Core console app
  2. Add this package reference
    <PackageReference Include="Nerdbank.GitVersioning" Version="1.5.28-rc" developmentDependency="true" />`
  3. Pack the project with msbuild /t:pack

Build fails with a crashed MSBuild task with this message:

C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(73,5): error MSB4018: The "PackTask" task failed unexpectedly.\r [C:\Users\andre\OneDrive\Documents\Visual Studio 2017\Projects\ConsoleApp1\ConsoleApp1\ConsoleApp1.csproj] C:\Program Files (x86)\Microsoft Visual Studio\2017\Enterprise\MSBuild\Sdks\NuGet.Build.Tasks.Pack\buildCrossTargeting\NuGet.Build.Tasks.Pack.targets(73,5): error MSB4018: System.IO.InvalidDataException: A stable release of a package should not have a prerelease dependency. Either modify the version spec of dependency "Nerdbank.GitVersionin g [1.5.61-ga5f362ccc1, )" or update the version field in the nuspec.\r [C:\Users\andre\OneDrive\Documents\Visual Studio 2017\Projects\ConsoleApp1\ConsoleApp1\ConsoleApp1.csproj]

In fact I shouldn't even have to specify DevelopmentDependency="true" in my reference because the nuspec of the referenced package itself has that property set.

AArnott commented 6 years ago

Sounds interesting. But what new thing does this let us do? If the user were willing to edit their project file manually, they can already use PrivateAssets="true". Or, as in my case, I usually add the PackageReference to a top-level Directory.Build.props file. Does this new SDK feature have support for inheriting SDK use from parent directories? If not, I'm not sure where I'd use this.

I think Microsoft might be able to use this for flavors of projects like WPF though, which is interesting.

dasMulli commented 6 years ago

Yes I'm also excited that this works well for non-package reference projects like classic asp.net, wpf etc.

It also doesn't require the package to be compatible with the target framework of the app. So the SDK's assemblies can target net46 / netcoreapp2.0 etc. and still work on a net35 project.

It should be possible to use this from Directory.Build.props/targets via the expanded syntax, e.g.

<Import Project="Sdk.props" Sdk="RoslynCodeTaskFactory/1.2.6" />

note that when not specifying the sdk version, it can be set in global.json - see https://docs.microsoft.com/en-us/visualstudio/msbuild/how-to-use-project-sdk

It does not solve any "easy to use" problems like developmentDependency would. esp. since there is no tooling integration like the NuGet package management. But it solves a lot of the pitfalls you'd fall into when using nuget packages for build-only dependencies.

clairernovotny commented 6 years ago

Can the SDK's here have PackageReference's in them? That's one big thing that the on-disk SDK's can do that NuGet targets cannot do. Will there be a "two-pass" restore, one for the SDK and then again for all the PackageReferences?

dasMulli commented 6 years ago

Yes that works. The work just like the on-disk SDKs and are just resolved by a different SDK resolver. (MSBuild now has a NuGet resolver in addition to the file system resolver). SDK resolving happens as part of the project evaluation so before any NuGet restore logic or build tasks run. There's also no need for an "SDK restore" since the download of the package will happen during project evaluation if needed.

clairernovotny commented 6 years ago

I wonder how the ordering works w.r.t. multiple SDK's. First one goes outer (first props/last targets?)

One big issue I have with MSBuild.Sdk.Extras is that I want my props to be after the .NET SDK props and my targets before the .NET SDK targets.

I wonder if this would do that: <Project Sdk="Microsoft.NET.Sdk;MSBuild.Sdk.Extras/1.2.1">

dasMulli commented 6 years ago

@onovotny I believe that should work. the sdk package will need Sdk/Sdk.props and Sdk/Sdk.targets files to make this work. Better follow up on this on an MSBuild issue like https://github.com/Microsoft/msbuild/issues/2803

nkolev92 commented 6 years ago

I'm glad the MsBuildSDK got brought up here, because I personally believe that's the correct approach for pure build time dependencies. One advantage of MSBuildSDKs is that they allow the author to participate in restore itself, unlike the build folder in regular dependency nupkgs.

It seems very dirty to allow package authors loose control over the transitivity flow of their own package. It's just a workaround for features/scenarios that were not designed at that time.

The beautiful thing about PackageReference is that it's consistent among clients and all the different gestures for adding a package to a project. Regardless of what your preferred way of managing your dependencies, whether it's the UI, dotnet.exe, hand-editing the csproj, it all works the same.

Adding this feature as discussed in this thread completely breaks that. It's basically introducing a new concept for package installation, where if you use the UI/dotnet add you get a different behavior vs hand-editing.

In addition, I don't really see this pattern here in how other package managers have handled DevDependencies. In yarn, npm, development dependencies are a user decision, not a package author decision.

I'd rather see NuGet move in that direction, than introduce an inconsistent behavior.

AArnott commented 6 years ago

@nkolev92 For NPM, devDependencies being a consumer decision makes sense because the build happens in the same language as the product being built. So legitimately the same package could serve either purpose. But in MSBuild/.NET, a package either delivers runtime components or build components (or sometimes both), but the package author is very clearly aware in every case I know of which use case it's designed for.

Now, in some future day when MSBuild allows tasks to be defined in .NET Standard where dependencies come from the nuget package cache, I'll agree that at that point a runtime package may be merely a build dependency. But I don't know what that will look like yet.

nexussays commented 6 years ago

I lost the thread of the conversation with the MSBuildSDK tangent, but I wasn't clear on the proposed solution when manually adding PackageReference. From what I understood there was the idea of implicitly setting PrivateAssets and IncludeAssets if they aren't set in the csproj and developmentDependency is true in the nuspec. If that is correct, then I disagree with that approach; every PackageReference should behave the same way by default. If you manually add references you should know how you intend to use the package and set PrivateAssets or IncludeAssets appropriately.

Been waiting on this for a while, it will remove an annoying development thorn. Is someone actively working on it?

jainaashish commented 6 years ago

That's not correct, when you manually add a PackageReference then we don't do anything implicitly and treat it as a normal package dependency. Same is also being updated at the spec - https://github.com/NuGet/Home/wiki/DevelopmentDependency-support-for-PackageReference

PR is already out which should be merged soon so you should be able to see this working in upcoming 15.7 preview release.

nkolev92 commented 6 years ago

@AArnott Packages can still be both today. There's nothing enforcing that today. It won't work because of the tangential problem because of how build assets transitive flow is handled today, but it's still possible.

But as you point out, in our case they are different. There's no point in fragmenting this experience among PR/MsBuildSDKs. The MSBuildSDKs is the correct approach, because itself participates in restore, which allows for a more comprehensive experience.

Additionally, there's no enforcement on whether development dependency marked packages bring in developmen dependencies themselves. If they bring in some runtime dependencies, and the consumer accidentally uses those, it'll break the package when packed later on.

Sdks don't have that since they have to be self-contained.

Additionally with development dependency we would introduce a needless basic feature discrepancy among VS and commandline which we've worked very hard not to do.

We are basically adding an install gesture in PR, which is currently non-existent (other than in name ofc)

bricelam commented 6 years ago

I think the decision to use ExcludeAssets="Runtime" has some unintended consequences. See aspnet/EntityFrameworkCore#11437

jainaashish commented 6 years ago

Since we had to revert this change because of EF tools package broken with ExcludeAssets=Runtime, I'm reopening this issue. We'll bring it back in 15.8 with more validations.

mwpowellhtx commented 6 years ago

:+1: re: @bording 's responses.

So, gentlemen, what's the bottom line here? I contend that seamlessly supporting rolling up developmentDependencies is desirable, with a minimum of files or other assets that I need to touch, whether by hand of via tooling of any kind. PrivateAssets, etc, just introduces a lower signal to noise ratio, IMO, i.e. makes it harder, not easier, to discern whether a dependency ought to be rolled up in the NuGet package.

Because logic, so to say.

bugproof commented 5 years ago

I tried setting developmentDependency to true and it doesn't work. Only PrivateAssets work.

nkolev92 commented 5 years ago

@wrathofodin https://github.com/NuGet/Home/wiki/DevelopmentDependency-support-for-PackageReference

BinToss commented 3 months ago

I tried setting developmentDependency to true and it doesn't work. Only PrivateAssets work.

I also misunderstood the spec at https://learn.microsoft.com/en-us/nuget/reference/nuspec#developmentdependency.

To clarify any confusion...

<developmentDependency>true</developmentDependency> in a package's nuspec indicates to package managers that the package is a development dependency. See https://learn.microsoft.com/en-us/nuget/reference/nuspec#developmentdependency.

<!-- microsoft.sourcelink.github.nuspec -->
<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    ...
    <developmentDependency>true</developmentDependency>
    ...
  </metadata>
</package>

SDK-style projects generate their nuspec. You can set the value of developmentDependency via the MSBuild property DevelopmentDependency. See https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#pack-target-inputs.

<PropertyGroup>
  <DevelopmentDependency>true</DevelopmentDependency>
</PropertyGroup>

or

dotnet pack -p:DevelopmentDependency=true

When a package manager adds a "development dependency" package as a PackageReference, the PackageReference's PrivateAssets is set to all and compile is removed from IncludeAssets if applicable. compile assets refer to binaries in a package's lib directory.

dotnet add package Microsoft.SourceLink.GitHub --version 8.0.0
<!-- This package was published with <developmentDependency>true</developmentDependency> in its nuspec -->
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="all" />
dotnet add package Newtonsoft.Json --version 13.0.3
<!-- This package was NOT published with developmentDependency set. -->
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />

Adding DevelopmentDependency="true" or developmentDependency="true" to a PackageReference will do nothing.

I tried setting developmentDependency to true and it doesn't work. Only PrivateAssets work.

I also misunderstood the spec at https://learn.microsoft.com/en-us/nuget/reference/nuspec#developmentdependency.

To clarify any confusion...

<developmentDependency>true</developmentDependency> in a package's nuspec indicates to package managers that the package is a development dependency. See https://learn.microsoft.com/en-us/nuget/reference/nuspec#developmentdependency.

<!-- microsoft.sourcelink.github.nuspec -->
<?xml version="1.0" encoding="utf-8"?>
<package xmlns="http://schemas.microsoft.com/packaging/2013/05/nuspec.xsd">
  <metadata>
    ...
    <developmentDependency>true</developmentDependency>
    ...
  </metadata>
</package>

SDK-style projects generate their nuspec. You can set the value of developmentDependency via the MSBuild property DevelopmentDependency. See https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#pack-target-inputs.

<PropertyGroup>
  <DevelopmentDependency>true</DevelopmentDependency>
</PropertyGroup>

or

dotnet pack -p:DevelopmentDependency=true

When a package manager adds a "development dependency" package as a PackageReference, the PackageReference's PrivateAssets is set to all and compile is removed from IncludeAssets if applicable. compile assets refer to binaries in a package's lib directory.

dotnet add package Microsoft.SourceLink.GitHub --version 8.0.0
<!-- This package was published with <developmentDependency>true</developmentDependency> in its nuspec -->
<PackageReference Include="Microsoft.SourceLink.GitHub" Version="8.0.0" PrivateAssets="all" />
dotnet add package Newtonsoft.Json --version 13.0.3
<!-- This package was NOT published with developmentDependency set. -->
<PackageReference Include="Newtonsoft.Json" Version="13.0.3" />

Adding DevelopmentDependency="true" or developmentDependency="true" to a PackageReference will do nothing.

<!-- still treated as a normal dependency -->
<PackageReference 
  Include="Newtonsoft.Json" 
  Version="13.0.3" 
  DevelopmentDependency="true" 
/>

Instead you must use PrivateAssets="all" ExcludeAssets="compile".

<!-- treated as a development dependency -->
<PackageReference 
  Include="Newtonsoft.Json"
  Version="13.0.3"
  PrivateAssets="all"
  ExcludeAssets="compile"
/>

ExcludeAssets="compile" is unnecessary if the package does not contain any libs i.e. the nupkg does not have a "libs" directory. If the package has no compile assets, then it doesn't matter if the compile assets are included. Likewise, package managers won't modify the IncludeAssets of a development dependency if there are no libs to exclude—despite the example indicating IncludeAssets is modified regardless. It better serves as an example of a development dependency that mistakenly includes libs, thereby requiring its compile assets (libs) to be excluded.