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

PackageReference should support DevelopmentDependency metadata #4125

Closed
AArnott opened this issue Dec 18, 2016 · 66 comments

Comments

@AArnott
Copy link
Contributor

commented Dec 18, 2016

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.

@davidfowl

This comment has been minimized.

Copy link
Member

commented Dec 18, 2016

You're aiming to have this package excluded from the output package right?

@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2016

Correct: no dependency, no embedding.
And thus no restriction on using unstable packages while building a stable one.

@emgarten

This comment has been minimized.

Copy link
Collaborator

commented Dec 18, 2016

PrivateAssets is the equivalent.

<PackageReference Include="Nerdbank.GitVersioning" Version="1.5.28-rc">
   <PrivateAssets>all</PrivateAssets>
</PackageReference>
@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2016

Thanks @emgarten. Should that be a default then when the package's own nuspec sets DevelopmentDependency=true?

Also, I'm guessing that's not quite equivalent to what we have in project.json (in an MSBuild project). I can add a package dependency to a developmentDependency package and that propagates across project references (as I usually want), yet that doesn't limit my ability to compile those projects into stable packages that don't depend on it. Your PrivateAssets=all metadata suggests to me that it won't propagate across P2Ps.

@onovotny

This comment has been minimized.

Copy link

commented Dec 21, 2016

I need this as well for GitVersionTask for the same reason. The GitVersionTask nuspec has developmentDependency set to true.

When a project references it, it should default the <PrivateAssets>all</PrivateAssets> metadata on that package ref...or whatever is needed to ensure it doesn't get added to the generated nuspec with /t:pack

@davidfowl

This comment has been minimized.

Copy link
Member

commented Dec 21, 2016

@emgarten we need 2 things:

  • We need a way for a package to express what assets get exported when referenced in a project .e.g If I want all projects that reference me to have effectively <PrivateAssets>all</PrivateAssets> by default then I should be able to express that in the nuspec.
  • We should infer developmentDependency set to true as implicitly saying <PrivateAssets>all</PrivateAssets> .
@AArnott AArnott referenced this issue Jan 21, 2017
13 of 19 tasks complete
@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Feb 16, 2017

Can we at a minimum for Dev15 RTW get developmentDependency=true in a nuspec file to be honored by having the dotnet SDK project type not propagate the dependency in nuget packages it packs?

@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2017

@davidfowl points out that PrivateAssets=all at the consuming side should workaround this issue. This is also consistent with the documentation. But I tried it, and the dotnet pack step still emits a package dependency on it (with the default exclude="Build,Analyzers" attribute no less).

@AArnott AArnott referenced this issue Feb 17, 2017
4 of 4 tasks complete
@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

@AArnott pack should not emit a dependency when <PrivateAssets>all</PrivateAssets> is specified. Did you make sure you restored after you added the <PrivateAssets> tag to your dependency? Pack relies on reading the dependencies and their metadata from the assets file, so if the assets file doesn't reflect your changes, pack wouldn't know about them.

@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2017

Thanks, @rohit21agrawal. That was the trick. I was only running dotnet pack after my change rather than a restore as well.

@rohit21agrawal

This comment has been minimized.

Copy link
Contributor

commented Feb 17, 2017

Great! Are we good to close this issue now?

@AArnott

This comment has been minimized.

Copy link
Contributor Author

commented Feb 17, 2017

No. There's still the matter of developmentDependency being ignored that should have set PrivateAssets=all by default, which @davidfowl seems to agree with as well.

@kzu

This comment has been minimized.

Copy link

commented Feb 23, 2017

We're seeing this as well in .NETCore/.NETStandard projects. Adding a package reference to a development dependency does not add the <PrivateAssets>all<PrivateAssets metadata.

Repro:

1 - Create a .NETCore/.NETStandard project
2 - Add a package reference (via the NPM) to NuGet.Build.Packaging (which has developmentDependency=true)

Result: PackageReference does not have PrivateAssets metadata
Expected: same behavior as packages.config, which adds the reference as <package id="NuGet.Build.Packaging" developmentDependency="true" ...

This is a regression IMHO.

/cc @adalon @mrward @mhutch

@alpaix

This comment has been minimized.

Copy link
Collaborator

commented Mar 15, 2017

Bug 389581:[Feedback] Installing NuGet package that is marked as a development dependency doesn't add the PrivateAssets="All" attribute to the PackageReference element

@davidfowl

This comment has been minimized.

Copy link
Member

commented Mar 18, 2017

Should this really be an install time action? Shouldn't it be part of the task?

@onovotny

This comment has been minimized.

Copy link

commented Mar 24, 2017

Agree with @davidfowl. This shouldn't be an install-time action, the task should set it when it reads the package meta-data during restore. A user may choose to override this by explicitly setting the metadata on PackageRef, but not sure why anyone would really do that.

@jainaashish

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2018

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

This comment has been minimized.

Copy link
Member

commented Mar 12, 2018

@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

This comment has been minimized.

Copy link

commented Mar 26, 2018

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

@jainaashish jainaashish modified the milestones: 4.7, 4.8 Apr 3, 2018

@jainaashish

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2018

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

This comment has been minimized.

Copy link

commented May 13, 2018

👍 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.

@0xNOP

This comment has been minimized.

Copy link

commented Jun 27, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.