Skip to content

Do not set NoBuild with GeneratePackageOnBuild because property reuse breaks publish#2822

Merged
nkolev92 merged 6 commits into
devfrom
dev-nkolev92-fix7801
Apr 24, 2019
Merged

Do not set NoBuild with GeneratePackageOnBuild because property reuse breaks publish#2822
nkolev92 merged 6 commits into
devfrom
dev-nkolev92-fix7801

Conversation

@nkolev92

@nkolev92 nkolev92 commented Apr 24, 2019

Copy link
Copy Markdown
Member

Bug

Fixes: NuGet/Home#7801
Regression: No

  • Last working version:
  • How are we preventing it in future:

Fix

Details:

GeneratePackageOnBuild setting NoBuild prevents basic projects from publishing.

This is a backwards compatible change.

I have added a test to confirm that this actually fixes the aforementioned problem.
We're "technically" testing a different product here, so there's potential that this could be a noisy test (highly unlikely).

//cc @peterhuene

Will merge this when dev switches to 5.2.

Testing/Validation

Tests Added: Yes
Reason for not adding tests:
Validation:

@nkolev92 nkolev92 changed the title Try to remove the no build dependency Do not set NoBuild with GeneratePackageOnBuild because property reuse breaks publish Apr 24, 2019
@peterhuene

Copy link
Copy Markdown

Thanks for the fix and heads-up as there is a change I need to make to the .NET Core SDK targets once we ingest this fix in the SDK.

@nkolev92

Copy link
Copy Markdown
Member Author

@peterhuene Do you need this only in 3.0?

I assume it's ok to target Preview 6?

@peterhuene

Copy link
Copy Markdown

Only in 3.0 and preview 6 is fine.

ProjectFileUtils.WriteXmlToFile(xml, stream);
}
// Run and assert.
msbuildFixture.RunDotnet(workingDirectory, $"publish {projectFile}");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the msbuildfixture utility. What is it asserting? I guess that the command ran successfully. Can it return the process exit code, so we can write the assert in the test, to make it more obvious to people reading the test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It asserts the exit code.

By default it is asserting that it completed successfully.

It also returns a command runner result.

I can add another assert to make it clearer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

GeneratePackageOnBuild should not set NoBuild.

3 participants