Move PackageOutputPath initialization to NuGet's Pack targets

[Nirmal4G]

This PR Fixes #10730

• This should free the SDK from having to manage NuGet Pack's PackageOutputPath property.
• This was possible because of the BaseOutputPath that'll be used instead of OutputPath in the Pack targets.
• It does depend on NuGet's Fix (OutputPath logic in Pack targets in a customized build doesn't work properly NuGet/Home#9234).

[wli3]

@Nirmal4G the build is failing. And please expect long delay for the review since temporary there is not enough people in my team

[Nirmal4G]

As I mentioned above, this PR depends on NuGet's fix -- NuGet/NuGet.Client#3270, which is now done. And the change is now live in v5.9!

[marcpopMSFT]

We missed the window for 5.0.2xx (our fault, we're working on improving our PR triage process) and any remaining changes to .2xx would require approval. I retargeted so we could get a successful build, signoff, and try to get this in early in 5.0.3xx so you aren't waiting like last time.

[dsplaisted]

There is some risk with this. It looks like if Platform or PlatformName is defined, then it will change the package output path to not include those values. That's probably a bug in the NuGet change that may need to be fixed. @nkolev92

It also changes it so that the default is defined later in evaluation, importantly, after Directory.Build.targets. This might break some custom logic in Directory.Build.targets.

I think this change is OK, but we need to see if the NuGet logic should include the PlatformName in the path if it's not AnyCPU, and document this as a potential breaking change.

EDIT: Also, we can't get this into 5.0.2xx.

[Nirmal4G]

It looks like if Platform or PlatformName is defined, then it will change the package output path to not include those values.

Yes. Refer NuGet issue and PR conversations on why we did the output path like that.

This might break some custom logic in Directory.Build.targets.

No. Maybe. But only in rare cases. Can you give me a specific example?

Besides why do we even want Platform as the package produced is per-project that have all the targets?

[dsplaisted]

This might break some custom logic in Directory.Build.targets.

No. Maybe. But only in rare cases. Can you give me a specific example?

Any Directory.Build.targets file that tries to read or update the PackageOutputPath could be broken by this.

Besides why do we even want Platform as the package produced is per-project that have all the targets?

Platform is something like x86 or x64. We don't support multi-targeting between those values, nor creating a package that has artifacts from multiple platforms.

It looks like if Platform or PlatformName is defined, then it will change the package output path to not include those values.

Yes. Refer NuGet issue and PR conversations on why we did the output path like that.

I looked at the NuGet PR and it looks like it was discussed here. But it doesn't seem to me like it was fully appreciated that this will move the .nupkg output path for non AnyCPU projects. It hasn't happened yet because the change to NuGet isn't active yet because we haven't removed the default from the SDK.

If we do want to make this change, then that would be another breaking change we'd need to document. If it's up to me than I don't think we should move this path. @nkolev92, what are your thoughts?

[Nirmal4G]

Any Directory.Build.targets file that tries to read or update the PackageOutputPath could be broken by this.

Only if it uses previously defined PackageOutputPath which we removed.

<!-- Using a previously defined 'PackageOutputPath' to update itself -->
<PackageOutputPath>$([System.IO.Path]::Combine('$(AnotherCommonPath)', '$(PackageOutputPath)'))</PackageOutputPath>

<!-- Using a previously defined 'PackageOutputPath' to update any other output path -->
<NuspecOutputPath>$([System.IO.Path]::Combine('$(AnotherCommonPath)', '$(PackageOutputPath)'))</NuspecOutputPath>

<!-- Adding a previously defined `PackageOutputPath` to a list of paths that'll be used for collecting the build outputs or something else -->
<AllBuildOutputs>$(AllBuildOutputs);$(PackageOutputPath)</AllBuildOutputs>

The first two cases are very VERY rare. But the last one is a sensible use case and is quite common.

We don't support multi-targeting between those values, nor creating a package that has artifacts from multiple platforms.

Because of this, you have separated nupkg output by platform but why not nuspec too? Currently nuspec outputs in BaseIntermediateOutputPath and is not separated by platform.

So, in conclusion, we should...

1. Use final output paths in both pack targets and DefaultOutputPaths targets to include platform.

2. Keep the defaults in the pack targets thus freeing Sdk from managing NuGet output paths but potentially lose some Directory.Build.targets benefits.

3. Do 2nd but also include final output paths in DefaultOutputPaths targets thus keeping the Directory.Build.targets benefits.

I do prefer the first option. I already had the patch but dropped it due to final output paths appending TFM and RID. Since, we're initializing NuGet output paths here, I guess we don't need to worry about TFM and RID appending. But I also prefer the third option as it screams compatibility!

[Nirmal4G]

@nkolev92 (you) said it's 1 package per project and the pack target acts on all build outputs which means it also includes platform targets too, right?

But according to @dsplaisted Pack targets doesn't support packing all platform targets.

I never checked it or never had the chance. So, I'm really confused by this.

[nkolev92]

Sorry for the delay.

Pack runs once per project. Multitargeting is only supported through TargetFramework for NuGet. The Platform and PlatformName are pretty much ignored by NuGet when used directly.
pack should be reading the values in the outer build I think.

There's a lot of context here, so I'm not sure what the recommendation is.

@dsplaisted are you suggesting that <PackageOutputPath Condition="'$(PackageOutputPath)' == ''">$(BaseOutputPath)$(Configuration)\</PackageOutputPath> on NuGet side should include the Platform? And if enabled that'd be a regression.

My general sentiment is the same from the original thread. Try to retain as much parity as possible. We really don't want to break anyone's build.

[marcpopMSFT]

Old PR review: This PR is more than a year old and does not appear to have active engagement. A reviewer has been notified offline to review and take action. If no action is taken, this PR will be closed in 14 days. Please comment if you want this PR left open for further investigation.

[Nirmal4G]

Since the BaseOutputPath was added to built-in targets, removing this would have changed PackageOutputPath to the original intended default.

The OutputPath is same as BaseOutputPath when you consider only .NET Framework which was the only framework when NuGet was built and PackageOutputPath got introduced. If anything, whoever wrote this line forgot to change it to BaseOutputPath when they moved the output path logic to SDK targets. It is well evident when you look at git blame.

This should have gone in v3, then it was moved to v5 but the window was missed by the team. It's not too late now but yes, this is a breaking change unless guarded behind MSBuild feature flag.

[marcpopMSFT]

At this point, it's probably not worth the breaking change. Holler if you disagree and want us to resync with nuget on this.