-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Do a two-phase publish to mimic VS behavior #29817
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
Changes from all commits
9d37b4c
27c090f
6995db7
a8c1bb7
0eb9b43
307202d
4614319
c8dbfff
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -23,6 +23,27 @@ namespace Microsoft.DotNet.Tools.Publish | |
| { | ||
| public class PublishCommand : RestoringCommand | ||
| { | ||
|
|
||
| /// <summary> | ||
| /// The list of properties that should be forwarded from the publish profile to the publish invocation. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// While we could forward along every property, the intent of this array is to mimic the behavior of VS, | ||
| /// whose Publish operation only forwards along a few properties. Of particular interest are properties that | ||
| /// are set very early on in the build (RID, Configuration, etc). The remainder will be imported during the | ||
| /// build via the Microsoft.Net.Sdk.Publish props and targets. | ||
| /// </remarks> | ||
| private static string[] PropertiesToForwardFromProfile = new [] { | ||
| MSBuildPropertyNames.CONFIGURATION, | ||
| MSBuildPropertyNames.LAST_USED_BUILD_CONFIGURATION, | ||
| MSBuildPropertyNames.LAST_USED_PLATFORM, | ||
| MSBuildPropertyNames.PLATFORM, | ||
| MSBuildPropertyNames.RUNTIME_IDENTIFIER, | ||
| MSBuildPropertyNames.RUNTIME_IDENTIFIERS, | ||
| MSBuildPropertyNames.TARGET_FRAMEWORK, | ||
| MSBuildPropertyNames.TARGET_FRAMEWORKS, | ||
| }; | ||
|
|
||
| private PublishCommand( | ||
| IEnumerable<string> msbuildArgs, | ||
| bool noRestore, | ||
|
|
@@ -43,30 +64,73 @@ public static PublishCommand FromParseResult(ParseResult parseResult, string msb | |
| parseResult.HandleDebugSwitch(); | ||
| parseResult.ShowHelpOrErrorIfAppropriate(); | ||
|
|
||
| var msbuildArgs = new List<string>() | ||
| { | ||
| "-target:Publish", | ||
| "--property:_IsPublishing=true" // This property will not hold true for MSBuild /t:Publish. VS should also inject this property when publishing in the future. | ||
| }; | ||
|
|
||
| IEnumerable<string> slnOrProjectArgs = parseResult.GetValueForArgument(PublishCommandParser.SlnOrProjectArgument); | ||
|
|
||
| CommonOptions.ValidateSelfContainedOptions(parseResult.HasOption(PublishCommandParser.SelfContainedOption), | ||
| parseResult.HasOption(PublishCommandParser.NoSelfContainedOption)); | ||
|
|
||
| msbuildArgs.AddRange(parseResult.OptionValuesToBeForwarded(PublishCommandParser.GetCommand())); | ||
| ReleasePropertyProjectLocator projectLocator = new ReleasePropertyProjectLocator(Environment.GetEnvironmentVariable(EnvironmentVariableNames.ENABLE_PUBLISH_RELEASE_FOR_SOLUTIONS) != null); | ||
| msbuildArgs.AddRange(projectLocator.GetCustomDefaultConfigurationValueIfSpecified(parseResult, MSBuildPropertyNames.PUBLISH_RELEASE, | ||
| slnOrProjectArgs, PublishCommandParser.ConfigurationOption) ?? Array.Empty<string>()); | ||
| msbuildArgs.AddRange(slnOrProjectArgs ?? Array.Empty<string>()); | ||
|
|
||
| bool noRestore = parseResult.HasOption(PublishCommandParser.NoRestoreOption) | ||
| || parseResult.HasOption(PublishCommandParser.NoBuildOption); | ||
|
|
||
|
|
||
| var publishProfileProperties = DiscoverPropertiesFromPublishProfile(parseResult); | ||
| var standardMSbuildProperties = CreatePropertyListForPublishInvocation(parseResult); | ||
| return new PublishCommand( | ||
| msbuildArgs, | ||
| // properties defined by the selected publish profile should override any other properties, | ||
| // so they should be added after the other properties. | ||
| standardMSbuildProperties.Concat(publishProfileProperties), | ||
| noRestore, | ||
| msbuildPath); | ||
| msbuildPath | ||
| ); | ||
|
|
||
| List<string> CreatePropertyListForPublishInvocation(ParseResult parseResult) { | ||
| var msbuildArgs = new List<string>() | ||
| { | ||
| "-target:Publish", | ||
| "--property:_IsPublishing=true" // This property will not hold true for MSBuild /t:Publish. VS should also inject this property when publishing in the future. | ||
| }; | ||
|
|
||
| IEnumerable<string> slnOrProjectArgs = parseResult.GetValueForArgument(PublishCommandParser.SlnOrProjectArgument); | ||
|
|
||
| CommonOptions.ValidateSelfContainedOptions(parseResult.HasOption(PublishCommandParser.SelfContainedOption), | ||
| parseResult.HasOption(PublishCommandParser.NoSelfContainedOption)); | ||
|
|
||
| msbuildArgs.AddRange(parseResult.OptionValuesToBeForwarded(PublishCommandParser.GetCommand())); | ||
| ReleasePropertyProjectLocator projectLocator = new ReleasePropertyProjectLocator(Environment.GetEnvironmentVariable(EnvironmentVariableNames.ENABLE_PUBLISH_RELEASE_FOR_SOLUTIONS) != null); | ||
| msbuildArgs.AddRange(projectLocator.GetCustomDefaultConfigurationValueIfSpecified(parseResult, MSBuildPropertyNames.PUBLISH_RELEASE, slnOrProjectArgs, PublishCommandParser.ConfigurationOption) ?? Array.Empty<string>()); | ||
| msbuildArgs.AddRange(slnOrProjectArgs ?? Array.Empty<string>()); | ||
|
|
||
| return msbuildArgs; | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Evaulates the project specified by the user and returns the list of properties that should be forwarded | ||
| /// to the actual call to the Publish MSBuild target. These properties are derived by the publish profile (if any) | ||
| /// specified by the user on the command line. If no publish profile is specified, this method returns an empty list. | ||
| /// If a publish profile is specified, it is loaded as a standalone file and specific properties are pulled out of it if they exist. | ||
| /// If it is specified but does not exist, we do not error because the current behavior of the build is to silently ignore | ||
| /// missing profiles. | ||
| /// </summary> | ||
| List<string> DiscoverPropertiesFromPublishProfile(ParseResult parseResult) | ||
baronfel marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| ReleasePropertyProjectLocator projectLocator = new ReleasePropertyProjectLocator(Environment.GetEnvironmentVariable(EnvironmentVariableNames.ENABLE_PUBLISH_RELEASE_FOR_SOLUTIONS) != null); | ||
| var cliProps = projectLocator.GetGlobalPropertiesFromUserArgs(parseResult); | ||
| var solutionOrProjectToPublish = parseResult.GetValueForArgument(PublishCommandParser.SlnOrProjectArgument); | ||
| var projectInstance = projectLocator.GetTargetedProject(solutionOrProjectToPublish, cliProps, includeSolutions: false); | ||
| // this can happen if the project wasn't loadable | ||
| if (projectInstance == null) | ||
| { | ||
| return new List<string>(); | ||
| } | ||
| var importedPropValue = projectInstance.GetPropertyValue(MSBuildPropertyNames.PUBLISH_PROFILE_IMPORTED); | ||
| if (!String.IsNullOrEmpty(importedPropValue) && bool.TryParse(importedPropValue, out var wasImported) && wasImported) { | ||
| try { | ||
| if (projectInstance.GetPropertyValue(MSBuildPropertyNames.PUBLISH_PROFILE_FULL_PATH) is {} fullPathPropValue) { | ||
| var properties = new ProjectInstance(fullPathPropValue).ToProjectRootElement().PropertyGroups.First().Properties; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While I haven't done the side-by-side comparison, I imagine that it would be faster to loop through the properties in the propertiesToForwardFromProfile, instead of loading in every property on the project into memory
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would prefer to use TryGetProjectInstance as race conditions could occur and cause failure if the project file is changed on disk in between this and the above lines of code |
||
| var propertiesToForward = properties.Where(p => PropertiesToForwardFromProfile.Contains(p.Name)); | ||
| return propertiesToForward.Select(p => $"--property:{p.Name}=\"{p.Value}\"").ToList(); | ||
| } | ||
| } catch (IOException) { | ||
| return new List<string>(); | ||
| } | ||
| } | ||
| return new List<string>(); | ||
| }; | ||
| } | ||
|
|
||
| public static int Run(ParseResult parseResult) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -680,7 +680,7 @@ public void PublishRelease_does_not_override_custom_Configuration_on_proj_and_lo | |
| .HaveStdOutContaining("PublishRelease"); | ||
|
|
||
| var releaseAssetPath = System.IO.Path.Combine(helloWorldAsset.Path, "bin", "Release", ToolsetInfo.CurrentTargetFramework, "HelloWorld.dll"); | ||
| Assert.False(File.Exists(releaseAssetPath)); // build will produce a debug asset, need to make sure this doesn't exist either. | ||
| Assert.False(File.Exists(releaseAssetPath)); // build will produce a debug asset, need to make sure this doesn't exist either. | ||
| } | ||
|
|
||
| [Theory] | ||
|
|
@@ -794,16 +794,8 @@ public void PublishRelease_interacts_similarly_with_PublishProfile_Configuration | |
| .Execute("/p:PublishProfile=test"); | ||
|
|
||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be preferable to add a test showing that publish profile proeprties are imported correctly, and for properties like configuration, they actually are changed (so |
||
| publishOutput.Should().Pass(); | ||
| var releaseAssetPath = System.IO.Path.Combine(helloWorldAsset.Path, "bin", "Release", ToolsetInfo.CurrentTargetFramework, rid, "HelloWorld.dll"); | ||
| if (config == "Debug") | ||
| { | ||
| Assert.True(File.Exists(releaseAssetPath)); // We ignore Debug configuration and override it, IF its custom though, we dont use publishrelease. | ||
| } | ||
| else | ||
| { | ||
| Assert.False(File.Exists(releaseAssetPath)); // build will produce a debug asset, need to make sure this doesn't exist either. | ||
| publishOutput.Should().HaveStdOutContaining("PublishRelease"); | ||
| } | ||
| var configAssetPath = System.IO.Path.Combine(helloWorldAsset.Path, "bin", config, ToolsetInfo.CurrentTargetFramework, rid, "HelloWorld.dll"); | ||
| Assert.True(File.Exists(configAssetPath)); // We cannot ignore Debug configuration and override it, because it's set as a globalproperty during publish. | ||
nagilson marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| } | ||
|
|
||
| [Fact] | ||
|
|
@@ -1098,7 +1090,7 @@ public void It_publishes_with_implicit_rid_with_rid_specific_properties(string e | |
| .Should() | ||
| .Pass() | ||
| .And | ||
| .NotHaveStdErrContaining("NETSDK1191"); // Publish Properties Requiring RID Checks | ||
| .NotHaveStdErrContaining("NETSDK1191"); // Publish Properties Requiring RID Checks | ||
| } | ||
|
|
||
| [Fact] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically this is a breaking change because you could have a publish profile in your directory that currently isn't being used by the .NET SDK, and so the Config, Rid, etc, could suddenly change with this change. IMO it would be better to take in .NET 8, but maybe this qualifies for a 300 band break. Either way, we would need a breaking change doc.