Home: OutputPath logic in Pack targets in a customized build doesn't work properly

[Nirmal4G]

NuGet.Build.Tasks.Pack project in NuGet.Client repo

    <PropertyGroup>
      <PackageOutputPath Condition=" '$(PackageOutputPath)' == '' ">$(OutputPath)</PackageOutputPath>
      <RestoreOutputPath Condition=" '$(RestoreOutputPath)' == '' " >$(MSBuildProjectExtensionsPath)</RestoreOutputPath>
    </PropertyGroup>

File: NuGet.Build.Tasks.Pack.targets(149-152)

I don't know why but when I try to customise a build relying on Pack targets, the output path logic hits me with so many issues that it doesn't work out of the box.

There are two main issues here,

1. OutputPath should be BaseOutputPath since, you are generating single package for many targets at once, which is similar to NuSpecOutputPath (has BaseIntermediateOutputPath).
2. The output paths logic is placed inside a target, which means in a customised build, overriding the target or excluding that particular logic cause path issues.

Related issue: dotnet/sdk#10730

Fix/Solution: Move the logic out of the target and place it where NuSpecOutputPath property is.

Breaks anything?: N/A

[Nirmal4G]

Supersedes #9231

[nkolev92]

OutputPath should be BaseOutputPath since, you are generating single package for many targets at once, which is similar to NuSpecOutputPath (has BaseIntermediateOutputPath).

In which project types is this an issue?

The output paths logic is placed inside a target, which means in a customised build, overriding the target or excluding that particular logic cause path issues.

Can you give an actual example?

In https://github.com/NuGet/Home/issues/9231, I simply wanted a default so that when you install the SDK, you don't get errors such as PackageOutputPath not being set!

[nkolev92]

In SDK based projects the structure is and should remain:

bin\Debug.nupkg
bin\Debug\targetFrameworkName1.dll
bin\Debug\targetFrameworkName2.dll
obj\Debug.nuspec
obj\Debug\targetFrameworkName1.dll
obj\Debug\targetFrameworkName2.dll

The outputs should mimic one another.

So the non-sdk based project defaults should be:

App
bin\Debug*.dll, *.exe, *.nupkg
obj\Debug*.nuspec, *.dll

[Nirmal4G]

In the SDK project, the structure becomes

obj\Debug\*.nuspec
bin\Debug\<targetFrameworkName>\<Rid>\*.nupkg

If we just use the OutputPath in the pack targets without the PackageOutputPath being set in the SDK!

With BaseOutputPath the above problem goes away. But BaseOutputPath is not present in the common targets, only in SDK targets. See the issue here...?!

[Nirmal4G]

That's why I offered two solutions without needing to modify common targets

https://github.com/NuGet/NuGet.Client/pull/3270#discussion_r392676996

[nkolev92]

obj\Debug\\*.nuspec

Please provide a repro.
Pack runs in the outerbuild, no rid and tfms are set in the outer build.

[Nirmal4G]

Sorry about the mistake! _fixed the comment_

Only nupkg path is wrong. nuspec uses BaseIntermediateOutputPath which will always have correct path.

Here's the repro

OutputPath Repro

Pack any project and look at the directory tree. Then change the properties in Directory.Build.props and pack again to see the changes (w.r.t. the various fixes I proposed in the PR).

[nkolev92]

Ok, I finally understand your concern.

A few points however:

• No regression/changes in SDK based projects. That means in sdk based projects we default to OutputPath, the same way we always have.
• In non-SDK based projects, we can add a default that goes to $(BaseOutputPath)$(ConfiguratioN) to avoid putting the nupkg in platform specific folders.
• Avoid using both Platform and RuntimeIdentifier in SDK based projects, https://github.com/dotnet/sdk/issues/1553 In particular probably set the platform in full framework projects only.

Given that here are my recommendations.

• Don't change the default for SDK based projects, just lift them and do the same test.
• For Full framework projects, a good default would be baseoutput/configuration which is the equivalent of what OutputPath is in in the outer build in SDK based projects.

[Nirmal4G]

No regression/changes in SDK based projects. That means in sdk based projects we default to OutputPath, the same way we always have.

In SDK projects, single-targeting will put the output nupkg to bin\<platform>\Debug\<tfm>\<rid>\*.nupkg when we remove the property in the SDK targets. It'll still be in bin\Debug\<tfm>\*.nupkg at the least, which is confusing.

Package path should not change for any custom build except Configuration, since you might need to upload debug packages to private feed while release packages to public.

Don't change the default for SDK based projects, just lift them and do the same test.

Are you sure that's what we want?

The BaseOutputPath will unify package output into single directory on both sdk (both outer/inner builds) and non-sdk projects.

We can do the BaseOutputPath changes along with MSBuild's Common targets' changes (if they'll accept). Besides what the user has set will always takes precedence, we're merely adjusting the defaults to be deterministic.

There are proposals to unify build outputs. I think this change might play well with those.

[nkolev92]

I screwed up something in my test for SDK based projects.
Let's just do BaseOutputPath and configuration for both.

[Nirmal4G]

Let's just do BaseOutputPath and configuration for both.

It's practically done. Let me know if there are any issues with the tests. I have yet to add framework pack tests too but that depends on changes from MSBuild common targets.

I have asked MSBuild team on BaseOutputPath in common targets.

[nkolev92]

I posted a comment on the PR to clean up a thing or two.

Because the pack targets are installed and used with both newer and older VS versions, we can consider a different default for non-SDK based projects.
We are really only trying to improve things there.

Sorry it took us a while to fully understand to what you were proposing.
The inconsistency throughout the targets got me so many times while testing :)

[Nirmal4G]

The inconsistency throughout the targets got me so many times while testing :)

I have updated the repro projects link: https://github.com/NuGet/Home/issues/9234#issuecomment-604731414

we can consider a different default for non-SDK based projects.

VS team can update pack targets in the next revision, right?

If we're back porting changes just to have the default, we can set OutputPath for older versions. but this also depends on the answer from MSBuild team. If they too are back-porting BaseOutputPath to older versions then we can just use that.

[nkolev92]

VS team can update pack targets in the next revision, right?

Not sure what you by vs team, but NuGet owns these experiences.

If we're back porting changes just to have the default, we can set OutputPath for older versions. but this also depends on the answer from MSBuild team. If they too are back-porting BaseOutputPath to older versions then we can just use that.

I don't think there'll be any servicing...the need is not there.
So for old style csproj we should do something that works today.

[Nirmal4G]

NuGet owns these experiences

I suppose, we could wait for microsoft/msbuild#1664. PR to Add BaseOutputPath is at microsoft/msbuild#5238

So for old style csproj we should do something that works today.

If it's urgent, I'll open a separate PR for using OutputPath as a default. Is that ok?

[nkolev92]

If it's urgent, I'll open a separate PR for using OutputPath as a default. Is that ok?

If you want to keep the changes isolated, let's reopen #9231 and handle that in a different PR.
I am ok with doing both in the current PR though. I think both changes need to go in the same release.

[Nirmal4G]

Me too. One PR is enough for this change.

Still waiting on the MSBuild PR.