Pack Task includes private package reference incorrectly

[clairernovotny]

In my utility package, I'm trying to update the MSBuild.Sdk.Extras PackageReference to add the PrivateAssets="all" metadata in my targets file so that the user doesn't have to remember this.

This is an attempt at a workaround until NuGet properly excludes developerDependency packages by default.

My workaround works for single target packages (TargetFramework), but fails for multitargeting (TargetFrameworks).

Looking at the MSBuild metadata items, the MSBuild.Sdk.Extras all appear to have the PrivateAssets="all" meta data attached, so I'm not sure why the pack targets is emitting them as dependencies.

To repro:
Checkout https://github.com/onovotny/MSBuildSdkExtras
Get commit 296ad3855a23ac3964774624bdf144bccc9a3968
Go to /Samples/ClasslibraryFromNuGet

Run msbuild /t:restore then msbuild /t:pack. The generated nuspec incorrectly lists the MSBuild.Sdk.Extras dependency.

I've attached a binlog showing that the metadata appears on the expected items.
msbuild.binlog.zip

[clairernovotny]

It appears the Restore isn't correctly populating the project.assets.json file with the correct metadata

Here's with/without the PrivateAssets="All" in the project file itself. In the second case, it is in targets that get included via a nuget package.

project.assets.json.txt
project.assets.orig.json.txt

I am guessing that the issue is that restore can't process targets from nuget's as they aren't available yet. The Pack targets thus cannot read the assets file for package reference data since it's not up-to-date. It has to get the PackageReference items from MSBuild to be correct.

[clairernovotny]

Note that this can help library authors workaround #4125 by including a targets file that updates the PackageReference item to add PrivateAssets="all" to it.

[emgarten]

project.assets.json looks correct to me

        "MSBuild.Sdk.Extras": {
            "suppressParent": "All",
            "target": "Package",
            "version": "[1.2.0-build.23, )"
          }

SuppressParent = PrivateAssets

I am not clear on what the issue is here, this package is suppose to be changing other PackageReferences and they aren't showing up correctly?

[clairernovotny]

I ran the restore twice, that's the correct one. The other one matches the binlog.

The issue is that a package has a targets file that updates the PackageRef Metadata to set privateassets=all. That is reflected in the build phase, but not in the generated package since the pack task creates the dependency list based off of the assets file instead of the current msbuild item state.

[emgarten]

the pack task creates the dependency list based off of the assets file instead of the current msbuild item state.

This is by design so that the packed nupkg matches the same behavior as the project.

[clairernovotny]

My point is that with msbuild, the project itself can be dynamic. The attributes of package reference can change after restore.

[davidfowl]

@onovotny I'm pretty sure that isn't supported. Wouldn't that be the same as allowing package references to be added as a result of restore?

[clairernovotny]

@davidfowl It's similar, but slightly different. Adding another package means you'd need another restore-pass to download it and process the generated props/targets.

This is about updating an existing PackageRef's metadata that controls whether it should be added as dependency or not.

Honestly, the only real reason I can think of where this is useful is as a workaround to #4125. If that issue was fixed so that developmentDependency packages weren't added as dependencies to a project using that package, I'd be all set.

[clairernovotny]

Note that if/when a NuGetSdkResolver comes around, NuGet will need another pass. It's perfectly reasonable to assume that SDK's include PackageReferences and other props/targets that might affect the lock file state.

[emgarten]

I understand the need for participating at a lower level like this, but I think it is a special case when the package is trying to modify how nuget itself works for the project instead of just being consumed by the project.

An SDK resolver would fix this, but in that case the user is intentionally referencing a package to modify the inputs to restore. With a normal package reference there is no way to tell if the package is intended to modify how NuGet works, or if it would be incorrect to treat the outputs of the package as inputs to NuGet. Which means we cannot add the feature requested in this issue without breaking things.

I suggest using this pattern to modify things before restore:

1. Add a targets that generates a <projectname>.csproj.<packageid>.g.targets file similar to the auto generated nuget ones so that MSBuild loads it up on the next pass.
2. This target should be set to run before build, restore, pack so that will always be there.
3. In the auto generated props/targets file update the project. This new file will not be excluded at restore time.
4. Fail before pack/build if the target was not used as an input to restore, and give a message saying that a 2nd restore is needed.

[clairernovotny]

Understood.... I think the real thing is to fix #4125 then as that's "how nuget works," and I was trying to change that to fix that package-side instead of NuGet-side.

[emgarten]

@nkolev92 for #4125, the pack scenario is a bit different.

We plan to trim dev dependencies when restoring higher level projects that do not need them. For pack we would need to mark these somehow (basically changing the inputs after getting the output) so that pack also knows that for the consumer of the package they should be ignored.

[clairernovotny]

I'm not sure I'm following. I don't understand why a development dependency should be marked as the nuget at all?

Or if that's the intended use of developmentDepenency, then we need another way, as a package, to say "we're a build time only tool, don't add me as a dependency".