Effective PrivateAssets of centrally managed transitive dependencies should be an intersection of parent dependencies

[marcin-krystianc]

Bug

Fixes: NuGet/Home#12270

Regression? Last working version: no

Description

The existing code was using bitwise OR for calculating the effective value of PrivateAssets for central managed transitive dependencies. But instead, it should perform bitwise AND to get the intersection of PrivateAssets values of relevant parent dependencies.

Also, during Pack, the asset of BuildTransitive was becoming Build | BuildTransitive so a new overload was added to get only the specified values.

The unit test was also updated to check more combinations of things.

PR Checklist

• PR has a meaningful title

• PR has a linked issue.

• Described changes

• Tests

  • Automated tests added
  • OR
  • Test exception
  • OR
  • N/A

• Documentation

  • Documentation PR or issue filled
  • OR
  • N/A

[marcin-krystianc]

Next one is #4954

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.

[jeffkl]

@NuGet/nuget-client this is ready for review

[zivkan]

Putting on my API design review hat on, do we need this new API?

Instead, can the caller add buildTransitive to the IEnumerable<string> before calling the existing public API?

Not that I'm an expert on NuGet.LibraryModel, but my gut reaction is that it's a bit of a leaky abstraction to add the bool parameter, and therefore might not be a good API design.

[jeffkl]

The problem is that the assets file contains only BuildTransitive and when its read by pack, the value comes back as both. In this code path, we are going to be using the value in the .nuspec so it should really only contain what the user specified. Since its a transitive dependency, Build should not flow unless specified right?

Given that, I couldn't come up with a better API change, I need this method to return the true values. I am open to suggestions though so please let me know.

[zivkan]

The problem is that the assets file contains only BuildTransitive and when its read by pack, the value comes back as both

That sounds like a bug to me.

When I do <PackageReference Include="Package1" Version="1.0.0" IncludeAssets="compile;runtime;build;buildtransitive" /> and restore, in the assets file I see "include": "Runtime, Compile, Build, BuildTransitive". So, the assets file does seem capable of specifying Build and BuildTransitive independently.

When I try packing a project (that does not use CPM) with PackageReferences modifiers like PrivateAssets="none", PrivateAssets="analyzers", or the above IncludeAssets, the generated nuspec contains the include and exclude attributes that I'd expect, including being capable of specifying Build and BuildTransitive independently.

This has me wondering, is this PR trying to solve the problem for CPM in a different way that it's already implemented for non-CPM projects?

Perhaps I don't understand your comment very well. I certainly don't know the code being modified, so if there are indeed practical constraints that aren't obvious from a theoretical knowledge of the asset selection feature, then I'm unaware. But I don't understand why reading the assets file will parse BuildTransitive as both Build and BuildTransitive, when the assets file is capable of representing both. From a pack point of view, it also doesn't appear to be a limitation of non-CPM projects.

[nkolev92]

There's a related bug: NuGet/Home#9672

Build transitive is implemented in a way that might not be the most intuitive.
build and buidltransitive aren't exclusive like all the other asset types.

if a package has build and buildtransitive files, only buildtransitive will be used.

By extension, if you write PrivateAssets="buildtransitive", pretty sure it tries to suppress both build and buildtransitive

[marcin-krystianc]

Yeah, this is super confusing. Accordign to my tests this reference:

<PackageReference Include="PackageWithBuildTransitive" Version="1.0.0" IncludeAssets="Build" ExcludeAssets="BuildTransitive" />

doesn't include Build assets although it is explicitly requested.

[zivkan]

Probably getting off-topic for this PR now, but does this also mean that the use case for NuGet/Home#6938 isn't going to work? Even with that feature implemented, the package trying to include its dependency's build assets isn't going to work as expected?

[nkolev92]

build/buildtransitive and contentfiles are the "weird' asset types. The rest work.
So technically will work NuGet/Home#6938, but we'd need to fix the other behaviors.

[jeffkl]

So what's the consensus here? With my code change, I feel that i get the expected result. Without changing it, it stays as confusing as ever. It only applies when someone tries to exclude BuildTransitive independently which I would imagine isn't too common.

[nkolev92]

I think it'd be great if the build/buildtransitive chnage is fixed in all scenarios.
Having it change only for transitive pinning is a probably not enough?

[nkolev92]

Given that we're doing the intersection, isn't an early exit is incorrect now?

[marcin-krystianc]

I think this the code is correct, maybe we could improve its readability by reversing the condition and write it instead like this?:

if (suppressParent != LibraryIncludeFlags.All)
{
	yield return new LibraryDependency()
	{
		LibraryRange = new LibraryRange(centralPackageVersion.Name, centralPackageVersion.VersionRange, LibraryDependencyTarget.Package),
		ReferenceType = LibraryDependencyReferenceType.Transitive,
		VersionCentrallyManaged = true,
		IncludeType = dependenciesIncludeFlags[centralPackageVersion.Name],
		SuppressParent = suppressParent,
	}; 
}

[jeffkl]

If the intersection yields LibraryIncludeFlags.All then exiting in this case means that the LibraryDependency will be left out of the centralTransitiveDependencyGroups section of the project.assets.json file and then during Pack it will be left out of the .nuspec. This would only happen if all parents of a package set PrivateAssets="All" which means they aren't in the .nuspec and neither should this transitively pinned package.

[nkolev92]

My bad, I thought this was in the foreach loop. I misread it.

I agree with @marcin-krystianc that flipping the condition might be more readable than using a continue.

[nkolev92]

There's a related bug: NuGet/Home#9672

Build transitive is implemented in a way that might not be the most intuitive.
build and buidltransitive aren't exclusive like all the other asset types.

if a package has build and buildtransitive files, only buildtransitive will be used.

By extension, if you write PrivateAssets="buildtransitive", pretty sure it tries to suppress both build and buildtransitive

[marcin-krystianc]

Given that there is no clear consensus about "Build" and "BuildTransitive" assets and the fact the problem is not strictly connected to the CPM - maybe it should be addressed in another PR?

[marcin-krystianc]

Yeah, this is super confusing. Accordign to my tests this reference:

<PackageReference Include="PackageWithBuildTransitive" Version="1.0.0" IncludeAssets="Build" ExcludeAssets="BuildTransitive" />

doesn't include Build assets although it is explicitly requested.

[ghost]

This PR has been automatically marked as stale because it has no activity for 7 days. It will be closed if no further activity occurs within another 90 days of this comment. If it is closed, you may reopen it anytime when you're ready again, as long as you don't delete the branch.