-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Fix Publish with GeneratePackageOnBuild=true #3473
Fix Publish with GeneratePackageOnBuild=true #3473
Conversation
i'll wait for Monday in ask mode. Don't track that |
@nguerrera @peterhuene this is the PR i mentioned in stand up. I tested $(GeneratePackageOnBuild) + $(PackAsTool) + Publish/Build/Pack combination, however, $(NoBuild) started to fail. And at this point I really have no confidence this change will be safe. The biggest problem is, I am essentially keep changing the dependency, until all tests pass. I cannot come up with a "grand theory" to avoid the circular dependency when _GeneratePackageOnBuild is true. If we do nothing, we are at the starting point of 3.0 work. So no new regression from 3.0. But NuGet/Home#7801 , #3471 will be there forever. I don't think they are critical bugs, consider they could be workaround easily. But we do need a better error message when they happen. Or you might come up with the "grand theory" to avoid the circular dependency? |
@peterhuene @nguerrera what do you think |
Hmm. Let me investigate this to see if I can offer any insights. Sorry for the delayed response. |
I wonder if we should set Your changes make sense to me for The problem is Note that with these changes, This really is a cluster. @nguerrera your take? |
@peterhuene this should fix pack + GeneratePackageOnBuild=true, but it will not fix publish + GeneratePackageOnBuild=true ? |
@peterhuene or you propose current PR change + The current PR will break nobuild=true + packastool :( I need to make it more strange (do similar stuff as GeneratePackageOnBuild for NoBuild ) |
How is publish with Additionally, how will it break |
Scratch that, I didn't have your changes any longer as I was in the middle of making changes for another PR. After rebuilding with your changes, I get the expected behavior for So what doesn't work with your fix regarding publish? I was suggesting that we use your fix plus a change to |
This is the bug #3367. I added the https://github.com/dotnet/sdk/pull/3255/files and essentially regressed NuGet/Home#7801. |
Yes, I understand that. Perhaps I didn't make it clear that your changes seem fine, but |
with the following diff, it seems fixed the NoBuild error. But I feel after all these changes, the dependency code is too complicated. diff --git a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
index b09744f6..6b27441a 100644
--- a/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
+++ b/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.PackTool.targets
@@ -27,8 +27,8 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup>
<_ToolsSettingsFilePath>$(BaseIntermediateOutputPath)DotnetToolSettings.xml</_ToolsSettingsFilePath>
<SuppressDependenciesWhenPacking Condition=" '$(PackAsTool)' == 'true' ">true</SuppressDependenciesWhenPacking>
- <_PackToolPublishDependency Condition=" '$(GeneratePackageOnBuild)' != 'true' and $(IsPublishable) == 'true' ">_PublishBuildAlternative</_PackToolPublishDependency>
- <_PackToolPublishDependency Condition=" '$(GeneratePackageOnBuild)' == 'true' and $(IsPublishable) == 'true' ">$(_PublishNoBuildAlternativeDependsOn)</_PackToolPublishDependency>
+ <_PackToolPublishDependency Condition=" ('$(GeneratePackageOnBuild)' != 'true' and '$(NoBuild)' != 'true') and $(IsPublishable) == 'true' ">_PublishBuildAlternative</_PackToolPublishDependency>
+ <_PackToolPublishDependency Condition=" ('$(GeneratePackageOnBuild)' == 'true' or '$(NoBuild)' == 'true') and $(IsPublishable) == 'true' ">$(_PublishNoBuildAlternativeDependsOn)</_PackToolPublishDependency>
</PropertyGroup>
<Target Name="PackTool" DependsOnTargets="GenerateToolsSettingsFileFromBuildProperty;$(_PackToolPublishDependency);_PackToolValidation" Condition=" '$(PackAsTool)' == 'true' ">
|
cc8ab25
to
cbd4c76
Compare
I think Peter's suggestion could work. At this point, if 3.0 has not regressed anything, given that we are getting late in 3.0 and that this is super confusing, I would suggest deferring the change. The problem I see with dotnet pack setting a global property to fix something.
@rainersigwald Do you have any ideas here? |
Without Will's changes, I think there are multiple regressions in 3.0:
I can live without fixing it for msbuild /t:Pack (not ideal), but if VS invokes the Pack target directly then I think that would kibosh the proposed global property fix. |
Ah, I asked yesterday if there were regressions, and the answer was no, but nevermind what I said if there are. The regressions should be fixed! |
Not really, if we do nothing, there is no extra regression since 3.0
#3253 regressed NuGet/Home#7801, which is a 3.0 bug itself. Tested in 2.2.401
Tested in 2.2.401. it was there in 2.2 at least |
I thought for sure at least the
|
d1f828b
to
09101a5
Compare
09101a5
to
15de252
Compare
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.
15de252
to
5bf3be8
Compare
…0191105.3 (dotnet#3473) - Microsoft.AspNetCore.DeveloperCertificates.XPlat - 5.0.0-alpha1.19555.3
* master: (27 commits) Add DOTNET_ROOT to tests and fix dogfood script Remove duplicated key Installation script update (dotnet#13030) Use scanelf over ldconfig on musl-libc distros (dotnet#12642) Allow list more properties (dotnet#3459) Update dependencies from https://github.com/mono/linker build 20191106.1 (dotnet#3476) [master] Update dependencies from aspnet/websdk (dotnet#3466) Update dependencies from https://github.com/aspnet/AspNetCore build 20191105.3 (dotnet#3473) Update dependencies from https://github.com/dotnet/windowsdesktop build 20191105.1 (dotnet#3469) Update dependencies from https://github.com/dotnet/arcade build 20191104.3 (dotnet#3465) Update dependencies from https://github.com/aspnet/websdk build 20191103.2 (dotnet#3454) [master] Update dependencies from dotnet/arcade (dotnet#3416) Update dependencies from https://github.com/aspnet/AspNetCore build 20191104.3 (dotnet#3458) Update dependencies from https://github.com/dotnet/templating build 20191104.1 (dotnet#3457) [master] Update dependencies from Microsoft/msbuild (dotnet#3423) [master] Update dependencies from aspnet/AspNetCore (dotnet#3440) Update dependencies from https://github.com/dotnet/windowsdesktop build 20191104.2 (dotnet#3455) [master] Update dependencies from dotnet/core-setup (dotnet#3445) Update dependencies from https://github.com/dotnet/windowsdesktop build 20191102.6 (dotnet#3446) Update dependencies from https://github.com/aspnet/websdk build 20191102.2 (#3444) ...
Set GeneratePackageOnBuild=false due to an issue where when true it does not build. dotnet/sdk#3473 (comment)
* Include net5.0 into build (codecutout#5) Update build to use github actions Co-authored-by: Alex Davies <alex@codecutout.com> * Update release.yaml Set environment variable using the recommended approach Previous approach is deprecated due to security issues * Update release.yaml Use = instead of :: * Update release.yaml Set GeneratePackageOnBuild=false due to an issue where when true it does not build. dotnet/sdk#3473 (comment) Co-authored-by: Alex Davies <4279588+alex-davies@users.noreply.github.com> Co-authored-by: Alex Davies <alex@codecutout.com>
Description
The previous change https://github.com/dotnet/sdk/pull/3255/files caused the Build target to no longer be part of the dependency of the Publish target when
GeneratePackageOnBuild=true
. With this change, we edited the dependency targets graph to add the targets publish needs without adding _Build back to the list. We also added test coverage for the combination of related msbuild properties to prevent future regression.Customer Impact
dotnet publish
fails whenGeneratePackageOnBuild=true
Regression?
Yes. By https://github.com/dotnet/sdk/pull/3255/files, since dotne core SDK 3.0.100-preview 6
Risk
Low risk.
Test changes in this PR
Added
GeneratePackageOnBuild
+PackAsTool
+ Publish/Build/Pack all combinations in unit test.===============
Fix #3367
===============
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.
We want to exclude Build step regardless NoBuild flag is true or not.
I have GeneratePackageOnBuild + PackAsTool + Publish/Build/Pack all combinations covered in tests. Although I did discover GeneratePackageOnBuild+Pack #3471 does not work. But it was there since 2.0. And without nuget help, we cannot easily fix it.