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.

Sign up for GitHub

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

Don't Infer RID outside of dotnet publish for PublishSingleFile/PublishAot #29031

Merged
merged 12 commits into from
Dec 16, 2022
Merged

Don't Infer RID outside of dotnet publish for PublishSingleFile/PublishAot #29031

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
b6899d0
only do rid inference on publishaot etc if is publishing
nagilson Nov 10, 2022
c38a2f0
Don't infer rid any time outside of publish
nagilson Nov 16, 2022
c431666
Errors only happen if _IsPublishing
nagilson Nov 29, 2022
a711dbe
Make the AOT test call DotnetPublishCommand instead of PublishCommand
nagilson Nov 30, 2022
553fc3d
Fix an implicit RID test to also use dotnet publish instead
nagilson Nov 30, 2022
76ea51f
Fix the publish with RID require property test to expect it to pass o…
nagilson Nov 30, 2022
995b296
Make GlobalPropertyFlowTests not expect SC to give a RID on build.
nagilson Nov 30, 2022
b0251c6
Test RID doesnt go into build now
nagilson Nov 30, 2022
17ac493
Infer RID for SelfContained at any time. Should check if PublishAot e…
nagilson Dec 13, 2022
e3c84ca
Infer RID for PublishSelfContained no SelfContained as well, and fix …
nagilson Dec 13, 2022
c3a68ec
Respond to PR feedback on RID Inference for Publish Only
nagilson Dec 13, 2022
6f17fc9
Add a missing paranth to rid inference.
nagilson Dec 13, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.RuntimeIdentifierInference.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,17 +67,29 @@ Copyright (c) .NET Foundation. All rights reserved.
<SelfContained>$(PublishSelfContained)</SelfContained>
</PropertyGroup>

<!-- Automatically infer the RuntimeIdentifier for properties that require it.
SelfContained without a RID is a no-op and semantically hints towards the fact that it can change the behavior of build, publish, and friends.
... So, we infer the RID for SelfContained regardless of the context.

The other publish properties are specifically labelled Publish* and don't 'NEED' their RID unless we are doing a publish, so the RID inference
... for these properties is limited to publishing only scenarios.

Finally, library projects and non-executable projects have awkward interactions here so they are excluded.-->
<PropertyGroup Condition="'$(UseCurrentRuntimeIdentifier)' == ''">
<UseCurrentRuntimeIdentifier Condition="
'$(RuntimeIdentifier)' == '' and
'$(_IsExecutable)' == 'true' and '$(IsTestProject)' != 'true' and
'$(IsRidAgnostic)' != 'true' and
(
'$(SelfContained)' == 'true' or
'$(PublishReadyToRun)' == 'true' or
'$(PublishSingleFile)' == 'true' or
'$(PublishAot)' == 'true'
)">true</UseCurrentRuntimeIdentifier>
(
'$(SelfContained)' == 'true' or
('$(_IsPublishing)' == 'true' and
(
'$(PublishReadyToRun)' == 'true' or
'$(PublishSingleFile)' == 'true' or
'$(PublishAot)' == 'true'
)
)
)">true</UseCurrentRuntimeIdentifier>
</PropertyGroup>

<PropertyGroup Condition="'$(UseCurrentRuntimeIdentifier)' == 'true'">
Expand All @@ -87,7 +99,7 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup Condition="'$(_IsPublishing)' == 'true' and '$(PublishRuntimeIdentifier)' != ''">
<RuntimeIdentifier>$(PublishRuntimeIdentifier)</RuntimeIdentifier>
</PropertyGroup>

<PropertyGroup Condition="'$(PlatformTarget)' == ''">
<_UsingDefaultPlatformTarget>true</_UsingDefaultPlatformTarget>
</PropertyGroup>
Expand Down Expand Up @@ -179,15 +191,15 @@ Copyright (c) .NET Foundation. All rights reserved.
ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
FormatArguments="SelfContained"/>

<NETSdkError Condition="'$(PublishReadyToRun)' == 'true' and '$(RuntimeIdentifier)' == ''"
<NETSdkError Condition="'$(PublishReadyToRun)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(_IsPublishing)' == 'true'"
ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
FormatArguments="PublishReadyToRun"/>

<NETSdkError Condition="'$(PublishSingleFile)' == 'true' and '$(RuntimeIdentifier)' == ''"
<NETSdkError Condition="'$(PublishSingleFile)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(_IsPublishing)' == 'true'"
ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
FormatArguments="PublishSingleFile"/>

<NETSdkError Condition="'$(PublishAot)' == 'true' and '$(RuntimeIdentifier)' == ''"
<NETSdkError Condition="'$(PublishAot)' == 'true' and '$(RuntimeIdentifier)' == '' and '$(_IsPublishing)' == 'true'"
ResourceName="ImplicitRuntimeIdentifierResolutionForPublishPropertyFailed"
FormatArguments="PublishAot"/>

Expand Down
27 changes: 27 additions & 0 deletions src/Tests/Microsoft.NET.Build.Tests/GivenThatWeWantToBuildASelfContainedApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,33 @@ public void It_does_not_build_SelfContained_due_to_PublishSelfContained_being_tr
outputDirectory.Should().NotHaveFile("hostfxr.dll"); // This file will only appear if SelfContained.
}

[Theory]
[InlineData("PublishReadyToRun")]
[InlineData("PublishSingleFile")]
[InlineData("PublishSelfContained")]
[InlineData("PublishAot")]
public void It_builds_without_implicit_rid_with_RuntimeIdentifier_specific_during_publish_only_properties(string property)
{
var tfm = ToolsetInfo.CurrentTargetFramework;
var testProject = new TestProject()
{
IsExe = true,
TargetFrameworks = tfm,
};
testProject.AdditionalProperties[property] = "true";
testProject.RecordProperties("RuntimeIdentifier");
var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: property);

var buildCommand = new DotnetBuildCommand(testAsset);
buildCommand
.Execute()
.Should()
.Pass();

var properties = testProject.GetPropertyValues(testAsset.TestRoot, targetFramework: tfm);
properties["RuntimeIdentifier"].Should().Be("");
}

[Theory]
[InlineData("net7.0")]
public void It_builds_a_runnable_output_with_Prefer32Bit(string targetFramework)
Expand Down
4 changes: 2 additions & 2 deletions src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAHelloWorldProject.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Runtime.InteropServices;
using System.Security.Cryptography;
using System.Xml.Linq;
using FluentAssertions;
using Microsoft.DotNet.Cli;
using Microsoft.DotNet.Cli.Utils;
using Microsoft.Extensions.DependencyModel;
Expand Down Expand Up @@ -1081,7 +1082,6 @@ public void It_publishes_with_full_path_publish_profile()
[InlineData("--p:PublishReadyToRun=true")]
[InlineData("-p:PublishSingleFile=true")]
[InlineData("-p:PublishSelfContained=true")]
[InlineData("")]
public void It_publishes_with_implicit_rid_with_rid_specific_properties(string executeOptionsAndProperties)
{
var testProject = new TestProject()
Expand All @@ -1092,7 +1092,7 @@ public void It_publishes_with_implicit_rid_with_rid_specific_properties(string e
testProject.AdditionalProperties.Add("IsPublishable", "false");
var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: executeOptionsAndProperties);

var publishCommand = new PublishCommand(testAsset);
var publishCommand = new DotnetPublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute(executeOptionsAndProperties)
.Should()
Expand Down
5 changes: 3 additions & 2 deletions src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAnAotApp.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -655,10 +655,11 @@ public void It_publishes_with_implicit_rid_with_NativeAotApp(string targetFramew
testProject.AdditionalProperties["PublishAot"] = "true";
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
var publishCommand = new DotnetPublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute()
.Should().Pass();
.Should()
.Pass();
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/Tests/Microsoft.NET.Publish.Tests/RuntimeIdentifiersTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,7 @@ public void ImplicitRuntimeIdentifierOptOutCorrectlyOptsOut()

var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
var publishCommand = new DotnetPublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute()
.Should()
Expand Down Expand Up @@ -335,7 +335,7 @@ public void PublishSuccessfullyWithRIDRequiringPropertyAndRuntimeIdentifiersNoRu
testProject.AdditionalProperties["PublishReadyToRun"] = "true";
var testAsset = _testAssetsManager.CreateTestProject(testProject);

var publishCommand = new PublishCommand(testAsset);
var publishCommand = new DotnetPublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand
.Execute()
.Should()
Expand Down
1 change: 1 addition & 0 deletions src/Tests/Microsoft.NET.TestFramework/TestAssetsManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ public TestAsset CopyTestAsset(
/// <param name="callingMethod">Defaults to the name of the caller function (presumably the test).
/// Used to prevent file collisions on tests which share the same test project.</param>
/// <param name="identifier">Use this for theories.
/// Pass in the unique theory parameters that can indentify that theory from others.
/// The Identifier is used to distinguish between theory child tests. Generally it should be created using a combination of all of the theory parameter values.
/// This is distinct from the test project name and is used to prevent file collisions between theory tests that use the same test project.</param>
/// <param name="targetExtension">The extension type of the desired test project, e.g. .csproj, or .fsproj.</param>
Expand Down
28 changes: 17 additions & 11 deletions src/Tests/dotnet-build.Tests/GivenDotnetBuildBuildsCsproj.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,24 +254,30 @@ public void It_does_not_warn_on_rid_with_self_contained_options_prior_to_net6()

[Theory]
[InlineData("--self-contained")]
[InlineData("-p:PublishTrimmed=true")]
[InlineData("")]
public void It_builds_with_implicit_rid_with_rid_specific_properties(string executeOptionsAndProperties)
public void It_builds_with_implicit_rid_with_SelfContained(string executeOptions)
{
var testInstance = _testAssetsManager.CopyTestAsset("HelloWorld")
.WithSource()
.WithTargetFrameworkOrFrameworks("net6.0", false)
.Restore(Log);
var targetFramework = ToolsetInfo.CurrentTargetFramework;
var testProject = new TestProject()
{
IsExe = true,
TargetFrameworks = targetFramework
};

new DotnetBuildCommand(Log)
.WithWorkingDirectory(testInstance.Path)
.Execute(executeOptionsAndProperties)
testProject.RecordProperties("RuntimeIdentifier");
var testAsset = _testAssetsManager.CreateTestProject(testProject);


new DotnetBuildCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name))
.Execute(executeOptions)
.Should()
.Pass()
.And
.NotHaveStdOutContaining("NETSDK1031") // Self Contained Checks
.And
.NotHaveStdErrContaining("NETSDK1190"); // Check that publish properties don't interfere with build either
.NotHaveStdErrContaining("NETSDK1190"); // Check that publish properties don't interfere with build either

var properties = testProject.GetPropertyValues(testAsset.TestRoot, targetFramework: targetFramework);
Assert.NotEqual("", properties["RuntimeIdentifier"]);
}

[RequiresMSBuildVersionFact("17.4.0.41702")]
Expand Down
31 changes: 0 additions & 31 deletions src/Tests/dotnet.Tests/dotnet-msbuild/GivenDotnetOsArchOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,36 +143,5 @@ public void CommandsRunWithArchOption(string command)
.Should()
.Pass();
}

[Fact]
public void ItUsesImplicitRidWhenNoneIsSpecifiedForSelfContained()
Copy link
Member

Choose a reason for hiding this comment

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

Why are these tests getting deleted?

Maybe we're moving some of the RID inference from the CLI to the SDK, but in that case I would expect to also see some code in the CLI command parsing deleted together with these tests.

Copy link
Member Author

@nagilson nagilson Dec 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I deleted the CLI command parsing code when I first added the RID inference but didn't see these tests until now. These are artifacts from Sarah's change which gave --runtime-identifier if --self-contained in the CLI

{
CommandDirectoryContext.PerformActionWithBasePath(WorkingDirectory, () =>
{
var msbuildPath = "<msbuildpath>";
var currentRid = CommonOptions.GetCurrentRuntimeId();
var command = BuildCommand.FromArgs(new string[] { "--self-contained" }, msbuildPath);
command.GetArgumentsToMSBuild()
.Should()
.StartWith($"{ExpectedPrefix} -restore -consoleloggerparameters:Summary " +
$"-property:SelfContained=True -property:_CommandLineDefinedSelfContained=true");
});
}

[Fact]
public void ItDoesNotUseImplicitRidWhenOneIsSpecifiedForSelfContained()
{
CommandDirectoryContext.PerformActionWithBasePath(WorkingDirectory, () =>
{
var msbuildPath = "<msbuildpath>";
var currentRid = CommonOptions.GetCurrentRuntimeId();
var command = BuildCommand.FromArgs(new string[] { "--self-contained", "--runtime", "fake-rid" }, msbuildPath);
command.GetArgumentsToMSBuild()
.Should()
.StartWith($"{ExpectedPrefix} -restore -consoleloggerparameters:Summary " +
$"-property:RuntimeIdentifier=fake-rid -property:_CommandLineDefinedRuntimeIdentifier=true " +
$"-property:SelfContained=True -property:_CommandLineDefinedSelfContained=true");
});
}
}
}