Fix output path issues with customized builds #3270

Nirmal4G · 2020-02-29T01:23:56Z

This PR Fixes NuGet/Home#9234

This fixes a host of path related issues when doing a customized build with Pack targets.
It also obsoletes the PackageOutputPath in the SDK's DefaultOutputPaths targets.
It also ensures non-sdk style projects doesn't have to specify some default paths.

Requires dotnet/msbuild#5238

dominoFire

Thank you so much for your contribution. We need some tests for this change.

src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets

Nirmal4G · 2020-03-09T16:56:37Z

I have been going through the tests. Why are all tests written with project directory as the PackageOutputPath and only obj as NuSpecOutputPath instead of bin\{<Platform>\}Debug and obj\Debug as these were previous defaults?

Is it okay if I left them as it is?

The reason all the tests pass because all of them overrides the output paths from the command line invocation. So, No matter what we set they'll all pass. But when I rewrote the logic for invoking Pack cmd using the defaults, they all failed since they were hard coded for the above mentioned logic.

Do I fix all of the tests or add a default path test separately while keeping the others working?

nkolev92 · 2020-03-09T18:50:55Z

I have been going through the tests. Why are all tests written with project directory as the PackageOutputPath and only obj as NuSpecOutputPath instead of bin{}Debug and obj\Debug as these were previous defaults?

I don't think there's a super special reason for this. It's all at the mercy of the person writing the tests.

I'd don't change the tests, they are a good test that the packageoutputpath behavior is preserved.

nkolev92

There are lots of formatting changes in the targets file.

The way I see it, I don't tihnk this change set needs to be bigger than 10 lines.

Can you please fix the formatting changes without a functional impact.

Thanks!

nkolev92

Hey @Nirmal4G,

We really appreciate all the effort you have put into this PR, especially your analysis which has been extremely detailed and spans more than just NuGet.

One of the things that would really help us expedite code reviews and the validation process is keeping PRs small and focused on solving one problem at the time. Therefore, we’d prefer it if you could move changes unrelated to the pack code in question to separate PRs as appropriate:

o Test code refactoring
o Build script improvements.

Doing this will also ensure that we can identify and revert potential regressions quickly without pulling out all the improvements that you have been kindly working on.

Again, on behalf of the @NuGet/nuget-client team thank you for your contributions, and we are looking forward to working with you on many more.

build/common.ps1

src/NuGet.Core/NuGet.Commands/MSBuildPackTargetArgs.cs

...uGet.Core/NuGet.Commands/RestoreCommand/RequestFactory/DependencyGraphSpecRequestProvider.cs

src/NuGet.Core/NuGet.Commands/RestoreCommand/Utility/NoOpRestoreUtilities.cs

src/NuGet.Core/NuGet.ProjectModel/ProjectStyle.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/DotnetToolTests.cs

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/MsbuildIntegrationTestFixture.cs

This fixes a host of path related issues when doing a customized build with Pack targets. It also obsoletes the 'PackageOutputPath' in the SDK's 'DefaultOutputPaths' targets. It also ensures non-sdk style projects doesn't have to specify some default paths.

The Default is now implied and points to the same directory. It is now redundant, and by removing it, the tests should not fail.

- Checks the default output paths from Project/Solution builds. - Check both the default and custom paths from continuous Project/Solution builds.

The duplicate logic is not needed. (edited anyway but will push if anyone approves)

Nirmal4G · 2020-10-12T18:30:52Z

I have simplified this PR to only contain the PackageOutputPath changes only for SDK style projects. The testing for non-SDK style projects has been separated into a new PR which I'll open once this is merged.

nkolev92

Thanks @Nirmal4G

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs

Nirmal4G · 2020-10-16T19:57:09Z

@nkolev92 All green, good to go!

nkolev92 · 2020-10-16T20:55:52Z

@dominoFire your review is blocking the PR, can you please take a look.

You can dismiss it if you'd rather not re-review.

Already addressed.

Nirmal4G requested review from aortiz-msft, cristinamanum, dominoFire, donnie-msft, dtivel, heng-liu, kartheekp-ms, nkolev92, rrelyea, zivkan and zkat as code owners February 29, 2020 01:23

Nirmal4G closed this Feb 29, 2020

Nirmal4G deleted the patch-1 branch February 29, 2020 01:30

Nirmal4G restored the patch-1 branch February 29, 2020 01:30

Nirmal4G reopened this Feb 29, 2020

Nirmal4G force-pushed the patch-1 branch from 5fb7f57 to 2199237 Compare March 4, 2020 13:22

dominoFire previously requested changes Mar 4, 2020

View reviewed changes

dominoFire added the Approved for CI label Mar 4, 2020

nkolev92 reviewed Mar 5, 2020

View reviewed changes

zivkan added the Community PRs created by someone not in the NuGet team label Mar 5, 2020

nkolev92 self-assigned this Mar 6, 2020

nkolev92 reviewed Mar 6, 2020

View reviewed changes

src/NuGet.Core/NuGet.Build.Tasks.Pack/NuGet.Build.Tasks.Pack.targets Show resolved Hide resolved

Nirmal4G requested a review from dominoFire March 6, 2020 02:31

dominoFire added Approved for CI and removed Approved for CI labels Mar 6, 2020

Nirmal4G force-pushed the patch-1 branch from 432d582 to c1aad87 Compare March 7, 2020 14:47

nkolev92 reviewed Mar 9, 2020

View reviewed changes

Nirmal4G requested a review from nkolev92 March 11, 2020 15:42

nkolev92 reviewed Aug 5, 2020

View reviewed changes

Nirmal4G marked this pull request as draft August 5, 2020 11:14

Nirmal4G mentioned this pull request Sep 1, 2020

Build NuGet.Client even if response file is detected #3623

Merged

Nirmal4G force-pushed the patch-1 branch from b53cea6 to 9a8fbf2 Compare September 3, 2020 09:32

This was referenced Sep 3, 2020

Improve test fixtures and related code #3637

Merged

Clarify Restore task differences in both IDE and CLI contexts #3638

Closed

Fix 'dotnet pack' tests when testing under VS IDE NuGet/Home#9996

Closed

Nirmal4G force-pushed the patch-1 branch 6 times, most recently from 4f8f76e to 054ccbc Compare October 3, 2020 10:45

Nirmal4G added 4 commits October 10, 2020 06:19

Remove default value to PackageOutputPath

47c07d2

The Default is now implied and points to the same directory. It is now redundant, and by removing it, the tests should not fail.

Added new Tests to check default output paths

ca3415b

- Checks the default output paths from Project/Solution builds. - Check both the default and custom paths from continuous Project/Solution builds.

Remove duplicate logic inside the target

aaa1371

The duplicate logic is not needed. (edited anyway but will push if anyone approves)

Nirmal4G force-pushed the patch-1 branch from 054ccbc to aaa1371 Compare October 12, 2020 18:20

Nirmal4G marked this pull request as ready for review October 12, 2020 18:23

Nirmal4G requested a review from a team as a code owner October 12, 2020 18:23

Nirmal4G requested a review from nkolev92 October 12, 2020 18:23

nkolev92 approved these changes Oct 16, 2020

View reviewed changes

test/NuGet.Core.FuncTests/Dotnet.Integration.Test/PackCommandTests.cs Show resolved Hide resolved

nkolev92 added Approved for CI and removed Approved for CI labels Oct 16, 2020

nkolev92 merged commit b6a116e into NuGet:dev Oct 19, 2020

Nirmal4G deleted the patch-1 branch October 20, 2020 17:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix output path issues with customized builds #3270

Fix output path issues with customized builds #3270

Nirmal4G commented Feb 29, 2020 •

edited

Loading

dominoFire left a comment

Nirmal4G commented Mar 9, 2020

nkolev92 commented Mar 9, 2020

nkolev92 left a comment

nkolev92 left a comment

Nirmal4G commented Oct 12, 2020

nkolev92 left a comment

Nirmal4G commented Oct 16, 2020

nkolev92 commented Oct 16, 2020

Fix output path issues with customized builds #3270

Fix output path issues with customized builds #3270

Conversation

Nirmal4G commented Feb 29, 2020 • edited Loading

dominoFire left a comment

Choose a reason for hiding this comment

Nirmal4G commented Mar 9, 2020

nkolev92 commented Mar 9, 2020

nkolev92 left a comment

Choose a reason for hiding this comment

nkolev92 left a comment

Choose a reason for hiding this comment

Nirmal4G commented Oct 12, 2020

nkolev92 left a comment

Choose a reason for hiding this comment

Nirmal4G commented Oct 16, 2020

nkolev92 commented Oct 16, 2020

Nirmal4G commented Feb 29, 2020 •

edited

Loading