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

Ensure Build target isn't invoked when NoBuild is set. #2938

Merged
merged 1 commit into from
Feb 15, 2019

Conversation

peterhuene
Copy link
Contributor

For dotnet pack --nobuild and dotnet publish --nobuild, if a target
inadvertently triggers the Build target, the build proceeds despite the user
explicitly requesting no build takes place.

This commit ensures the SDK errors if the Build target gets invoked when
NoBuild is true.

Fixes dotnet/cli#9581.

@peterhuene peterhuene added this to the 3.0.1xx milestone Feb 14, 2019
@peterhuene peterhuene requested a review from a team February 14, 2019 05:59
@nguerrera
Copy link
Contributor

nguerrera commented Feb 14, 2019

I think it would be good to look at https://github.com/dotnet/cli/issues/9656 together with/soon after this to make sure we're on solid footing with NoBuild.

@peterhuene
Copy link
Contributor Author

It would obviously be a breaking change to have a NoBuildForPublish and NoBuild (implying for pack). I was aware of the request, but didn't want to make that call yet.

I'd be happy to include that fix with this PR if we feel that's wise.

@nguerrera
Copy link
Contributor

I meant we should try to find a way to fix the issue without giving up on NoBuild everywhere. I just meant we should look at that while we’re looking at this. May require coordinated changes with NuGet.

@nguerrera
Copy link
Contributor

We should test this with GeneratePackageOnBuild

@peterhuene
Copy link
Contributor Author

Good catch. It fails with GeneratePackageOnBuild.

It seems wrong for GeneratePackageOnBuild to set NoBuild just to control GenerateNuspecDependsOn.

Now I remember why I had the PackAsTool check; it was the wrong granularity as I thought it was somehow setting NoBuild, but it was actually the tool pack tests setting GeneratePackageOnBuild.

I'll have to replace the PackAsTool check with a GeneratePackageOnBuild check and if a pack --nobuild accidentally triggers a build with GeneratePackageOnBuild set to true, then we won't catch it.

@peterhuene
Copy link
Contributor Author

peterhuene commented Feb 14, 2019

I meant we should try to find a way to fix the issue without giving up on NoBuild everywhere.

What do you mean by giving up on NoBuild?

Ah, I think you mean fixing it without having an additional property in the publish targets?

@peterhuene
Copy link
Contributor Author

peterhuene commented Feb 14, 2019

I think I get what you're saying.

How about this: we keep this commit, change the PackAsTool check to a GeneratePackageOnBuild check. File a bug against NuGet to stop setting NoBuild on GeneratePackageOnBuild by adding an additional condition to setting GenerateNuspecDependsOn based on GeneratePackageOnBuild.

I'll add a TODO comment linked to the issue with these changes. With both fixes in place (and a follow-up change from us to remove the GeneratePackageOnBuild check here), we should fix this and dotnet/cli#9656.

@nguerrera
Copy link
Contributor

Let me step back:

  1. We have the linked issue when publish with NoBuild is combined with GeneratePackageOnBuild.

  2. One proposal on the issue was to use something other than NoBuild for publish (to avoid collision with pack). I have consistently pushed back against this breaking change with poorer usability. Taking this approach, which I do not want to do, is what I meant by “giving up on NoBuild everywhere”.

But we still need to fix (1) somehow and I’m saying that we should look at this while we’re adding more NoBuild functionality.

@nguerrera
Copy link
Contributor

Typed that without seeing recent replies. Thumbs up on plan above.

For `dotnet pack --nobuild` and `dotnet publish --nobuild`, if a target
inadvertently triggers the `Build` target, the build proceeds despite the user
explicitly requesting no build takes place.

This commit ensures the SDK errors if the `Build` target gets invoked when
`NoBuild` is true.

Fixes dotnet/cli#9581.
@peterhuene
Copy link
Contributor Author

I've filed NuGet/Home#7801 and amended the commit as stated.

@peterhuene peterhuene merged commit 007ac8b into dotnet:master Feb 15, 2019
@peterhuene peterhuene deleted the no-build branch February 15, 2019 02:42
dsplaisted pushed a commit to dsplaisted/sdk that referenced this pull request Feb 19, 2020
…build 20190919.9 (dotnet#2938)

- Microsoft.NET.Sdk.Razor - 3.1.0-preview1.19469.9
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.

2 participants