Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GeneratePackageOnBuild should not set NoBuild. #7801

Closed
peterhuene opened this issue Feb 14, 2019 · 4 comments · Fixed by NuGet/NuGet.Client#2822
Closed

GeneratePackageOnBuild should not set NoBuild. #7801

peterhuene opened this issue Feb 14, 2019 · 4 comments · Fixed by NuGet/NuGet.Client#2822
Assignees
Milestone

Comments

@peterhuene
Copy link

peterhuene commented Feb 14, 2019

Details about Problem

See dotnet/cli#9656 and dotnet/cli#9581 for some additional context.

We have two problems in the .NET Core SDK for 3.0:

  • The first is that setting GeneratePackageOnBuild in a project file fails to publish with a simple dotnet publish if the project hasn't already been built. The root of the problem is that the publish targets also use NoBuild to support the --no-build option and expect the property to signify this. However, the pack targets are setting NoBuild to true when GeneratePackageOnBuild is also true (see this line in the pack targets). This inadvertently turns dotnet publish into dotnet publish --no-build when GeneratePackageOnBuild is true.
  • The second is that the SDK fails to detect a build has occurred even though --no-build was specified (for publish in this case, but should also apply to pack). This would prevent unintentional changes to the build output files that are about to be packed or published when --no-build is specified.

We'd like to solve both issues, but doing so requires a change to both the .NET Core SDK and the pack targets.

On the SDK side, we're adding a check to see if CoreBuild gets invoked with NoBuild set to true and we error in that case. This will catch bugs that inadvertently cause a build with NoBuild set (dotnet/cli#9581). However, because NoBuild gets set when GeneratePackageOnBuild is true, we have to skip the check when GeneratePackageOnBuild is true, for now at least.

On the NuGet side, would it be possible to stop turning on NoBuild when GeneratePackageOnBuild is true and instead condition setting GenerateNuspecDependsOn based on GeneratePackageOnBuild?

That is to say remove the set of NoBuild entirely (see source link above) and condition GenerateNuspecDependsOn like this:

...
<PropertyGroup Condition="'$(NoBuild)' == 'true' or '$(GeneratePackageOnBuild)' == 'true'">
    <GenerateNuspecDependsOn>$(GenerateNuspecDependsOn)</GenerateNuspecDependsOn>
</PropertyGroup>
<PropertyGroup Condition="'$(NoBuild)' != 'true' and '$(GeneratePackageOnBuild)' != 'true'">
    <GenerateNuspecDependsOn>Build;$(GenerateNuspecDependsOn)</GenerateNuspecDependsOn>
</PropertyGroup>
...

As far as I can determine, that should be a backwards compatible change on your side, while fixing dotnet/cli#9656.

Environment information

.NET Core SDK (reflecting any global.json):
 Version:   3.0.100-preview-010000
 Commit:    e46d7b966e

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  10.14
 OS Platform: Darwin
 RID:         osx.10.14-x64
 Base Path:   /Users/peterhuene/src/sdk/.dotnet/sdk/3.0.100-preview-010000/

Host (useful for support):
  Version: 3.0.0-preview-27218-01
  Commit:  d40b87f29d

.NET Core SDKs installed:
  3.0.100-preview-010000 [/Users/peterhuene/src/sdk/.dotnet/sdk]

.NET Core runtimes installed:
  Microsoft.AspNetCore.App 3.0.0-preview-18579-0056 [/Users/peterhuene/src/sdk/.dotnet/shared/Microsoft.AspNetCore.App]

To install additional .NET Core runtimes or SDKs:
  https://aka.ms/dotnet-download
@rrelyea
Copy link
Contributor

rrelyea commented Feb 15, 2019

This is difficult to do in 5.0...and we are busy driving towards ZBB for that. Would love to have somebody dig into this in the next few weeks.

@nkolev92
Copy link
Member

I hit some related issues while messing with the 3.0.0 SDK.

@peterhuene

The plan is the fix the issues for 3.0 only, correct?

We're nearing escrow for 5.1, and if this is 3.0 only then there's no need to try to argue for it in 5.1 escrow

@peterhuene
Copy link
Author

That's correct, only 3.0 and landing in P6 would be fine.

@nkolev92
Copy link
Member

@peterhuene The first NuGet insertion into CLI/SDK that has 5.2.0 in the version will have this change.

I'll try to remember to ping you when that happens :)
Save bet is 3 weeks down the line.

wli3 pushed a commit to wli3/sdk that referenced this issue May 22, 2019
NuGet/Home#7801

since nuget no longer sets NoBuild on GeneratePackageOnBuild we need to do similar logic to get the previous state.
wli3 pushed a commit to wli3/sdk that referenced this issue May 22, 2019
NuGet/Home#7801

since nuget no longer sets NoBuild on GeneratePackageOnBuild we need to do similar logic to get the previous state.
wli3 pushed a commit to dotnet/sdk that referenced this issue May 22, 2019
* Fix GeneratePackageOnBuild with PackAsTool both on

NuGet/Home#7801

since nuget no longer sets NoBuild on GeneratePackageOnBuild we need to do similar logic to get the previous state.
wli3 pushed a commit to dotnet/sdk that referenced this issue Jun 5, 2019
* Fix GeneratePackageOnBuild with PackAsTool both on

NuGet/Home#7801

since nuget no longer sets NoBuild on GeneratePackageOnBuild we need to do similar logic to get the previous state.
wli3 pushed a commit to dotnet/sdk that referenced this issue Jun 5, 2019
* Fix GeneratePackageOnBuild with PackAsTool both on

NuGet/Home#7801

since nuget no longer sets NoBuild on GeneratePackageOnBuild we need to do similar logic to get the previous state.
wli3 pushed a commit to wli3/sdk that referenced this issue Jul 27, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, dotnet#2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.

> Will it work this time?

It will, since I have GeneratePackageOnBuild + PackAsTool + Publish/Build/Pack all combinations covered in tests. Although I did discover GeneratePackageOnBuild+Pack dotnet#3471 does not work. But it was there since 2.0. And without nuget help, we cannot easily fix it.
wli3 pushed a commit to wli3/sdk that referenced this issue Aug 5, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, dotnet#2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.
wli3 pushed a commit to wli3/sdk that referenced this issue Aug 5, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, dotnet#2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.
wli3 pushed a commit to wli3/sdk that referenced this issue Aug 5, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, dotnet#2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.
wli3 pushed a commit to wli3/sdk that referenced this issue Aug 5, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, dotnet#2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.
wli3 pushed a commit to dotnet/sdk that referenced this issue Aug 6, 2019
What I did is essentially write a different dependency graph in GeneratePackageOnBuild = true to avoid circular dependency . Original bug. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. Instead of the logic is in nuget, it is in SDK now. Publish should not skip Build  if GeneratePackageOnBuild is true. Or it will mean no build even use Publish verb.

But revert 3255 means, #2114 will be back. The solution is to break Pack’s dependency on Build in GeneratePackageOnBuild. But it still need Publish. So I need to let Pack depend directly on no build version of publish.

> Why cannot directly use “_PublishNoBuildAlternative”?

We want to exclude Build step regardless NoBuild flag is true or not.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants