set ExcludeRestorePackageImports to true for inner build targets in pack

[rohit21agrawal]

Fixes: NuGet/Home#3927

Also fixes NuGet/Home#5281

This makes sure that the targets from PackageReferences don't affect the contents of the resulting nupkg.

[AArnott]

the targets from PackageReferences don't affect the contents of the resulting nupkg

This seems overly broad in its goal. What if the whole point of the package you're referencing is to affect the contents of the resulting nupkg? That, for example, is the whole point of Nerdbank.MSBuildExtension which dramatically manipulates the contents of a package.

[AArnott]

Adding global properties to an MSBuild task invocation like this will cause MSBuild to re-build the transitive closure of targets for the target(s) you're asking to build. If _GetBuildOutputFilesWithTfm depends on other targets (or other targets have BeforeTargets or AfterTargets set on this one) I'm concerned that you'll end up double-building them as a result of this. And as MSBuild targets often have non-idempotent side effects and dramatic impact on build perf, we should strive to keep the global properties the same everywhere we can.
Was the set of global properties you had before this change identical to those used for inner-build invocations from the dotnet SDK targets?

[rohit21agrawal]

@AArnott can you provide more context on what is it that Nerdbank.MSBuildExtension does?

The only impact this change will be on files that go into lib folder , source files and pdbs (for symbol packages) and framework assembly references - those things a PackageReference won't be able to change.

[dasMulli]

If I were to implement an msbuild task distributed via NuGet that creates content files to be packaged as <Content Include="$(IntermediateOutputPath)\generatedFoo\fooConfig.json" Pack="true" BuildAction="Content" PackCopyToOutputDirectory="true" /> (may be misspelling sth), would these never have a chance of being generated or never be included? Or would they be included if it were a target imported from a buildMultiTargeting folder?
Also, would this affect targets that generate code files and emit <Compile> items from a target? - e.g. a target reading a JSON dictionary file to generate a class with static string values?

[dasMulli]

Note that Grpc.Core has this target in their net* specific build folder. Not sure how it will be affected but it may even be beneficial. see https://github.com/dotnet/cli/issues/6679

[rohit21agrawal]

@dasMulli with the change i have in place, the scenario you described would still work as is. However, i am still debating whether it's acceptable to do so or not.

For Grpc.Core either the package author should update the package and set Pack to false on each of those Content items, or the consumer of the package should add Exclude="Build" on the PackageReference declaration. The consumer could even set "ExcludeRestorePackageImports=true" in their csproj to get rid of the targets from PackageReferences.

The question here is what is the right default?

[dasMulli]

cc @jskeet fyi

[rohit21agrawal]

@dasMulli @AArnott I also don't get why a PackageReference (say A.nupkg) wants to include Content in the referencing project (say B). Given that dotnet restore is transitive, A.nupkg will be a part of the restore graph, and if it's authored correctly, it can use contentFiles to add the content to the consuming project.

[jskeet]

Cc @jtattermusch

[AArnott]

@rohit21agrawal said:

The only impact this change will be on files that go into lib folder , source files and pdbs (for symbol packages) and framework assembly references - those things a PackageReference won't be able to change.

PackageReferences can change what goes into the lib folder. They can basically do anything they want to via their imported .targets file. And Nerdbank.MSBuildExtension in particular does just that: it redirects all outputs from the lib folder to another folder (tools or build) in the package, and adds more files as well.

I also don't get why a PackageReference (say A.nupkg) wants to include Content in the referencing project (say B).

My scenario isn't adding "Content". But it does dramatically impact other elements of the package that references Nerdbank.MSBuildExtension. And all referencing projects should set PrivateAssets="all" to the PackageReference because my package is merely a build-time assistant.

[rohit21agrawal]

i am closing this PR for now as we need to revise the scenario and come up with a better solution that fits the needs of the community.