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

Add support for new feature switch #14432

Merged
merged 7 commits into from
Nov 13, 2020
Merged

Conversation

mateoatr
Copy link
Contributor

@mateoatr mateoatr commented Nov 4, 2020

This adds SDK support for StartupHookSupport, a new feature switch that allows users to disable startup hooks.

Related to #14385.

@vitek-karas
Copy link
Member

@samsp-msft for naming of the new property StartupHookSupport - to enable/disable startup hooks.

I don't know if we picked a pattern - based on some of the others maybe it should be EnableStartupHooks?

@jkotas
Copy link
Member

jkotas commented Nov 4, 2020

Should this change also include turning this on by default when PublishTrimmed is set? Or is it still up to the debate what we want to do for it?

@mateoatr
Copy link
Contributor Author

mateoatr commented Nov 5, 2020

Should this change also include turning this on by default when PublishTrimmed is set? Or is it still up to the debate what we want to do for it?

Going through the discussion on dotnet/runtime#44050 a second time, I think it's settled that we should turn this on by default whenever PublishTrimmed is used. If there's no debate I'll make the change in this PR.

@mateoatr mateoatr requested a review from sbomer November 5, 2020 06:41
@@ -22,6 +22,15 @@ 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
Member

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
Member

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.

@eerhardt
Copy link
Member

eerhardt commented Nov 5, 2020

based on some of the others maybe it should be EnableStartupHooks?

My understanding of the existing naming pattern is that EnableXXX is used when the feature should be off by default - EnableUnsafeBinaryFormatterSerialization and EnableUnsafeUTF7Encoding. While XXXSupport is used when the feature is on by default - DebuggerSupport and EventSourceSupport.

Following this pattern kind of poses a problem here since StartupHook's default is based on whether PublishTrimmed is true or not.

I think my main concern is that whatever we decide, we should be consistent between the runtimeconfig/AppContext name and the MSBuild property name. Don't use EnableXXX for MSBuild while the runtimeconfig setting is named System.StartupHookProvider.IsSupported. (Basically, don't do what we did with System.Net.Http.EnableActivityPropagation => HttpActivityPropagationSupport.)

@vitek-karas
Copy link
Member

I like @eerhardt view on this. Just comment about the default settings: The default is somewhat tricky, as you could also argue that EventSourceSupport is off by default for Blazor projects.
But we could use the "the common default for the feature" - so the typical setting across all scenarios. Or maybe even - we really want people to not use this - in which case use Enable otherwise use Support.

@vitek-karas
Copy link
Member

Add the new feature switch to the "all feature switches" test in

public void It_publishes_the_project_correctly(string targetFramework, string [] expectedPublishFiles)

@vitek-karas
Copy link
Member

I filed #14475 to track the default values for feature switches in trimmed apps

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

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

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
Member

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.

@mateoatr mateoatr merged commit 2fae2fa into dotnet:master Nov 13, 2020
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.

5 participants