Skip to content

Add support for new feature switch #14432

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

Merged
mateoatr merged 7 commits into from
Nov 13, 2020
Merged

Add support for new feature switch #14432

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
2975de6
Add StartupHookSupport feature-switch
Nov 4, 2020
9e749e5
Disable startup hooks on trimmed apps by default
Nov 5, 2020
e0d05e0
Fix property condition
Nov 5, 2020
3ec30d7
Add new feature switch to test
Nov 11, 2020
cb4df10
Add feature switch to csproj
Nov 11, 2020
d1d9db8
Update default condition
Nov 12, 2020
f614776
Test that .NET 5 remains unchanged
Nov 12, 2020
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
1 change: 1 addition & 0 deletions src/Assets/TestProjects/KitchenSink/TestApp/TestApp.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
<TieredCompilation>true</TieredCompilation>
<TieredCompilationQuickJit>true</TieredCompilationQuickJit>
<TieredCompilationQuickJitForLoops>true</TieredCompilationQuickJitForLoops>
<StartupHookSupport>false</StartupHookSupport>
<EventSourceSupport>false</EventSourceSupport>
<UseSystemResourceKeys>true</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding>false</EnableUnsafeUTF7Encoding>
Expand Down
10 changes: 10 additions & 0 deletions src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.ILLink.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,16 @@ Copyright (c) .NET Foundation. All rights reserved.
<_LinkSemaphore>$(IntermediateOutputPath)Link.semaphore</_LinkSemaphore>
</PropertyGroup>

<!-- We disable startup hooks for trimmed apps here so that the feature
Copy link
Copy Markdown
Member

@MichalStrehovsky MichalStrehovsky Nov 5, 2020

Choose a reason for hiding this comment

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

Should this go into a props file so that it can actually be overriden from a project?

Copy link
Copy Markdown
Member

@eerhardt eerhardt Nov 5, 2020

Choose a reason for hiding this comment

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

It can't go into a .props file because then the project wouldn't get a chance to set PublishTrimmed before this check happens.

So instead, the check below should be

<PropertyGroup Condition="'$(StartupHookSupport)' == '' AND '$(PublishTrimmed)' == 'true'>

which will allow for both to be set in the project file.

switch can flow to the runtimeconfig.json. Startup hooks are disabled
by default since they may require assemblies, types or members that
could be removed by the linker, causing a trimmed app to crash. -->
<PropertyGroup Condition="'$(StartupHookSupport)' == '' And
'$(PublishTrimmed)' == 'true' And
'$(_TargetFrameworkVersionWithoutV)' >= '6.0'">
<StartupHookSupport>false</StartupHookSupport>
</PropertyGroup>

<!--
============================================================
ILLink
Expand Down
6 changes: 5 additions & 1 deletion src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Sdk.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -400,6 +400,11 @@ Copyright (c) .NET Foundation. All rights reserved.
Condition="'$(TieredCompilationQuickJitForLoops)' != ''"
Value="$(TieredCompilationQuickJitForLoops)" />

<RuntimeHostConfigurationOption Include="System.StartupHookProvider.IsSupported"
Condition="'$(StartupHookSupport)' != ''"
Value="$(StartupHookSupport)"
Trim="true" />

<RuntimeHostConfigurationOption Include="System.Text.Encoding.EnableUnsafeUTF7Encoding"
Condition="'$(EnableUnsafeUTF7Encoding)' != ''"
Value="$(EnableUnsafeUTF7Encoding)"
Expand All @@ -412,7 +417,6 @@ Copyright (c) .NET Foundation. All rights reserved.
<RuntimeHostConfigurationOption Include="System.Threading.ThreadPool.MaxThreads"
Condition="'$(ThreadPoolMaxThreads)' != ''"
Value="$(ThreadPoolMaxThreads)" />

</ItemGroup>

<!--
Expand Down
1 change: 1 addition & 0 deletions src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToPublishAProjectWithAllFeatures.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ public void It_publishes_the_project_correctly(string targetFramework, string []
""System.Runtime.TieredCompilation"": true,
""System.Runtime.TieredCompilation.QuickJit"": true,
""System.Runtime.TieredCompilation.QuickJitForLoops"": true,
""System.StartupHookProvider.IsSupported"": false,
""System.Text.Encoding.EnableUnsafeUTF7Encoding"": false,
""System.Threading.ThreadPool.MinThreads"": 2,
""System.Threading.ThreadPool.MaxThreads"": 9,
Expand Down
35 changes: 35 additions & 0 deletions src/Tests/Microsoft.NET.Publish.Tests/GivenThatWeWantToRunILLink.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using Microsoft.NET.TestFramework.Assertions;
using Microsoft.NET.TestFramework.Commands;
using Microsoft.NET.TestFramework.ProjectConstruction;
using Newtonsoft.Json.Linq;
using Xunit;
using Xunit.Abstractions;

Expand Down Expand Up @@ -372,6 +373,40 @@ public void ILLink_verify_analysis_warnings_hello_world_app(string targetFramewo
Assert.True(!missingWarnings.Any() && !extraWarnings.Any(), errorMessage);
}

[Theory]
[InlineData("net5.0")]
Copy link
Copy Markdown
Member

@eerhardt eerhardt Nov 11, 2020

Choose a reason for hiding this comment

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

Since net5.0 has already shipped, Should we be considering this a breaking change when users start building their existing net5.0 apps with the 6.0 SDK?

Copy link
Copy Markdown
Member

@vitek-karas vitek-karas Nov 11, 2020

Choose a reason for hiding this comment

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

This is great point - we should probably only change the default for net6.0 and above. Everything else can stay the same, just changing the default value of the feature switch when trimming should only happen for net6.0 apps and above.

[InlineData("net6.0")]
Comment thread
mateoatr marked this conversation as resolved.
public void StartupHookSupport_is_false_by_default_on_trimmed_apps(string targetFramework)
{
var projectName = "HelloWorld";
var rid = EnvironmentInfo.GetCompatibleRid(targetFramework);

var testProject = CreateTestProjectForILLinkTesting(targetFramework, projectName);
var testAsset = _testAssetsManager.CreateTestProject(testProject, identifier: projectName);

var publishCommand = new PublishCommand(Log, Path.Combine(testAsset.TestRoot, testProject.Name));
publishCommand.Execute($"/p:RuntimeIdentifier={rid}", "/p:PublishTrimmed=true")
.Should().Pass();

string outputDirectory = publishCommand.GetOutputDirectory(targetFramework: targetFramework, runtimeIdentifier: rid).FullName;
string runtimeConfigFile = Path.Combine(outputDirectory, $"{projectName}.runtimeconfig.json");
string runtimeConfigContents = File.ReadAllText(runtimeConfigFile);


if (Version.TryParse(targetFramework.TrimStart("net".ToCharArray()), out Version parsedVersion) &&
parsedVersion.Major >= 6)
{
JObject runtimeConfig = JObject.Parse(runtimeConfigContents);
runtimeConfig["runtimeOptions"]["configProperties"]
["System.StartupHookProvider.IsSupported"].Value<bool>()
.Should().Be(false);
}
else
{
runtimeConfigContents.Should().NotContain("System.StartupHookProvider.IsSupported");
}
}

[Theory]
[InlineData("netcoreapp3.0")]
public void ILLink_accepts_root_descriptor(string targetFramework)
Expand Down