Home: Allow package authors to define build assets which work transitively (using buildTransitive assets group)

[nkolev92]

Consider changing the default transitive flowing behavior of the build assets.
SPEC

Related https://github.com/NuGet/Home/issues/6388

Related to https://github.com/NuGet/Home/issues/6206, although at pack time likely

[mungojam]

Feels related: this discussion

In summary, having a way to include compile in private assets as default unless project/package needs to expose it, i.e.: PrivateAssets="compile;contentfiles;analyzers;build" (I checked what the implicit default is and it seems to be contentfiles;analyzers;build).

[nkolev92]

@mungojam

I think #4125 covers what you're articulating.

We are considering bringing developmentDependency back, which should allow the author of PackageA to affect the default flowing behavior for the dependencies of PackageA.

PrivateAssets is basically a transitive flow control for the consumer of the package, while DevelopmentDependency would be a transitive flow control for the author the package.

[mungojam]

@nkolev92 It's similar to development dependency but more general. If I am using a package like Newtonsoft.Json in my package, I don't want it to be exported to consumers of my package unless I explicitly need to (or I'm creating a meta-package). It's more like a runtimeDependency rather than an apiDependency.

I wouldn't want Newtonsoft.Json to be part of the contract of my package because some day I might move to a different Json library.

It feels like this should be the default somehow.

[mungojam]

Just re-read the second bit of your comment. Perhaps I am mistaken, I haven't ever used DevelopmentDependency, but have assumed it is only for stopping dependencies going anywhere, i.e. PrivateAssets="All" which would be too extreme in my case as they are runtime dependencies that need to go up the chain and into bin folder.

[natemcmaster]

I think this is a good feature, worth pursuing, and one I wish we had from the beginning.

But since its a breaking change, my knee-jerk reaction to reading the spec on this is that it could cause compatibility issues with existing packages, like some of the ones we shipped in ASP.NET Core 1.0. They contain build props/targets written under the assumption that they only apply to one project file. For example, some using Microsoft.Extensions.Configuration.UserSecrets 1.x would get new build warnings. It may also cause some Visual Studio issues where it uses MSBuild properties set via PackageReference to change behavior (a feature I'm currently working on.)

The spec currently says:

Since this is a breaking change, we need to provide a switch for consumers to change back the dependency asset behavior.

Can you comment more on current thinking for this open question?

Related question: how would a user know they need to change back? New build errors are actually good. They force a user to figure out what changed. The more pernicious bugs will happen because some builds will continue working without failures/warnings, but the output of the build will change. i.e. new files in output, different build properties will be set, etc. How would we expect a user to figure out what caused these behavior changes?

cc @davidfowl

[nkolev92]

@natemcmaster

I will answer your other concerns within the next few days as I update the spec.

Just wanted to quickly reply to existing packages point.

The existing packages won't be impacted because the transitive dependency behavior is already embedded in the nuspec.

<group targetFramework=".NETStandard1.0">
        <dependency id="NETStandard.Library" version="1.6.1" exclude="Build,Analyzers" />
        <dependency id="Microsoft.CSharp" version="4.3.0" exclude="Build,Analyzers" />
        <dependency id="System.ComponentModel.TypeConverter" version="4.3.0" exclude="Build,Analyzers" />
        <dependency id="System.Runtime.Serialization.Primitives" version="4.3.0" exclude="Build,Analyzers" />
      </group>

I will make sure to call that out clearly in the spec.

[natemcmaster]

Just to clarify, my concerns are about how NuGet imports targets from P2P references. This is what you're proposing, right?

Given:

MyBuildTools.nupkg
   build/MyBuildTools.props

ClassLib.csproj
   -> PackageReference Include="MyBuildTools"

App.csproj
   -> ProjectReference Include="..\ClassLib\ClassLib.csproj"

Current:
✅ ClassLib.csproj imports build/MyBuildTools.props
:x: App.csproj does not import build/MyBuildTools.props

Proposed change:
✅ ClassLib.csproj imports build/MyBuildTools.props
✅ App.csproj imports build/MyBuildTools.props

[nkolev92]

@natemcmaster

Yeah that's the proposed change.

The user awareness is the hardest part.
We don't really have a precedent for breaking changes like this in NuGet.

Considered adding some sort of warning, making it an opt-in feature etc, each with it's own pros and cons.

[bording]

@nkolev92 Will this change also include analyzers, and not just build assets? #3697 was closed and points to this issue, but it was dealing with both build and analyzers.

[natemcmaster]

@bording IMO including analyzers too make senses.

What about contentFiles?

@nkolev92 thanks for clarifying. I think this is a more intuitive behavior. I've seen several users hit issues where the expected build assets to flow across p2p references, and when they didn't, adding a package ref to every project wasn't the most obvious solution.

Just throwing a crazy idea out there:

If I understand the context of this issue, one motivation is to close the gap between ProjectReference and PackageReference (https://github.com/NuGet/Home/issues/3401). Instead of breaking the behavior of ProjectReference, what if NuGet introduced a new MSBuild item type that could be resolved to either project or package?

<!-- In project -->
<NuGetReference Include="MyClassLib" Version="*" />

Like NuGet did in project.json, it could search first for projects in solution or sitting in a sibling folder. If not found, treat it as a package ref.

if File.Exists("../MyClassLib/MyClassLib.*proj")
    return <ProjectReference IncludeAssets="Build,Analyzers,Compile,Native">
else 
    return <PackageReference>

[nkolev92]

Despite the duality, I think it's still very important for the user to be able to tell what's a Project vs Package reference.
Also the entry bar would be high with a change like that.

Maybe @AArnott can share his opinion on analyzers.
Don't have the right context on why they were in exclude assets in the first place.

//cc @emgarten @rrelyea

[AArnott]

I would probably have been in favor of letting build assets be transitive in the initial design. But at this point the breaking change IMO isn't worth all the trouble. NuGet has been a really rough ride from 2.x to 4.x, and since about 4.1 or so, it's been really stable, to the point where I don't "pin" the version of nuget our projects build with any more. If nuget takes to changing defaults that change the way builds work, I'm going to go back to long build scripts that download nuget.exe of a specific version to pin the behavior again, and I really would rather just trust NuGet to not break over time.

So ya, whether it's analyzers or build assets, my vote is to keep what we have, because we already have it.

[xen2]

Interested in this feature.

In case the fact it's a breaking change is a blocker, it could be done in .nuspec?
i.e. MyBuildTools.nuspec could override the default PrivateAssets when it is referenced in another project.

Note: I am personally fine if implemented as a breaking change.

[jainaashish]

We're planning to change this default behavior in dev16 for both build and analyzers to flow transitively for package reference or p2p. It will not impact anything for existing packages. And even for new packages, Author can always control it through include/ exclude attributes.

Let us know if there are concerns/ feedback.

[AArnott]

Does this mean https://github.com/NuGet/Home/issues/6279 will be fixed as well? Before we make analyzers flow _more_, I want to stop them from flowing, which I can't do right now.

[jainaashish]

Yes, it will be fixed as well. Analyzers will become part of project.assets.json file which will allow project system to consume/skip.

[jainaashish]

I've updated the exisitng spec to reflect the new understanding and our next step. This is still in review but feel free to provide any early feedback.

[natemcmaster]

Yay! Time to update a bunch of packages to take advantage of this feature.

[clairernovotny]

Wait, to be clear, this will work across both package ref's and project refs? So a Proj A -> Proj B -> package C, where C has build assets, and the props/targets will get evaluated for A, and B?

[clairernovotny]

@terrajobst, I wonder if this will let me do evil things like switch up the assembly the compiler sees based on UseWPF/UseWindowsForms?

[natemcmaster]

I wonder if this will let me do evil things

🙈 haha, you could already do evil things with props/targets in packages. This lets you do even more evil things.

[clairernovotny]

Here's the situation with netcoreapp3 -- there's no way to distinguish between the platform and the app model (wpf/winforms).

I need to have a target for netcoreapp3 but only for WPF/WinForms. If the regular SDK, or linux, tries to build consume a binary that itself uses wpf/winforms (and has public surface area), it fails:

https://dev.azure.com/dotnet/ba70dafd-6b93-4176-b27f-975148db36bd/_build/results?buildId=8872

My goal would be to build two netcoreapp3 binaries: one without the surface area, and one with. For that TFM, I need an ItemGroup with a conditional Reference. The gotcha is that it has to work transitively or it'll hose everyone.

[natemcmaster]

So, yes, buildTransitive/ let's you workaround this.

As a side note, have you opened this issue against dotnet/sdk? I think they would be interested in this feedback on the wpf/winforms problems.

[clairernovotny]

Immo and I have talked a lot about this. It's currently by design and he thinks my situation is not at all common....I think it'll bite any MVVM library, among others. The alternative is to have two packages: Core and Core.WindowsDesktop. I don't like having to split that way though.

[jainaashish]

@onovotny buildTransitive will help you with your scenario and works across PackageRef or ProjectRef.

I'll also work on updating the NuGet docs to make it more readable. And also get a blogpost out soon.

[clairernovotny]

@jainaashish is this expected to be in .NET Core 3 preview 2? That'd be very helpful and unblock Rx.NET.

[jainaashish]

It's late for preview2 but it will be there in preview3.

[clairernovotny]

Okay, good to know, I guess that gives it time to get into VS as well.

For NuGet restores, if I install a nightly .NET Core SDK that has this (post-preview 2), will VS use that or do I really need the updated VS?

[jainaashish]

VS have its own code so you'd need to update VS to get the latest changes.

[terrajobst]

My general feel for the the design of .props and .targets ingestion is that this should be considered an escape hatch for advanced package authoring so that cases like @onovotny where conventions don't work the developer has a 100% control. For this to work, they build scripts must flow across P2P, so I'm highly supportive of this change.

As a side note, have you opened this issue against dotnet/sdk? I think they would be interested in this feedback on the wpf/winforms problems.

Yes, we should discuss this more but this problem doesn't have good solutions IMHO. It either requires making WPF/WinForms a different TFM, inventing some new profile mechanism, or generalizing RIDs so that people can vary API surface based on them. Personally, I think Oren's scenario where a developer needs to inject different code based on whether the project references WPF/WinForms is rare -- and I think are also likely indicators of bugs in the library. In Oren's case the library depends on UI controls. This hasn't bitten anyone in .NET Framework because everything is there, all the time. But technically this was always a very clear layer violation.

[xen2]

Great, we can finally get rid of those custom PrivateAssets!
Will this make it only in VS 2019 or also in a VS 2017 update?

[jainaashish]

For now, this will only be in VS 2019 (starting with VS 2019 preview3)

[ericstj]

Curious if you considered a bit in the nuspec metadata to indicate this behavior rather than forcing folks to copy their files into different directories. Your spec calls out that folks essentially need to duplicate the buildTransitive folder anyhow to work with old clients/packages.config. Seems to me like its a non-goal to enable folks to have different content in build and buildTransitive but giving them this option opens up that possibility.

[nkolev92]

@ericstj

We did consider the nuspec option, but it would've led to an unenvious pack experience.

Having both build/buildTransitive imported was a non-goal at that point.

[ericstj]

What does "unenvious pack experience" mean? If you can point me to that discussion I'll read up rather than rehash stuff here.

[nkolev92]

It would've been contentFiles like, where there is still no way to configure all the switches like language.
The precedent with duplication played a role in the decision.

Not sure if/where that write-up is.
Not sure how much @jainaashish looks at GitHub these days to help with some pointers.

[jainaashish]

We indeed discussed multiple solutions while designing this feature and one such case was to introduce a flag in nuspec. All discussed solutions might still be there in Spec PR in Engineering private repo. But as far as i remember nuspec solution was not suitable for a) if you need both transitive and non-transitive build assets in a packae (which was a real usecase for some customers) and b) would not have worked for packages.config.