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

Default new runtime feature switches #23932

Merged
2 commits merged into from
Jul 16, 2020
Merged

Default new runtime feature switches #23932

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
262692b
Default new runtime feature switches
eerhardt Jul 14, 2020
bc18095
PR feedback
eerhardt Jul 15, 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
10 changes: 9 additions & 1 deletion ...ft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.Components.Wasm.targets
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@ Copyright (c) .NET Foundation. All rights reserved.
<!-- Trimmer defaults -->
<PublishTrimmed Condition="'$(PublishTrimmed)' == ''">true</PublishTrimmed>
<TrimMode Condition="'$(TrimMode)' == ''">link</TrimMode>
<TrimmerRemoveSymbols Condition="'$(TrimmerRemoveSymbols)' == ''">false</TrimmerRemoveSymbols>
Copy link
Member Author

Choose a reason for hiding this comment

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

The SDK defaults TrimmerRemoveSymbols based on DebuggerSupport. If DebuggerSupport is false, then the linker will remove the symbols. Since the Blazor SDK removes symbols from the publish output, I thought it was reasonable to always default TrimmerRemoveSymbols=false so even if DebuggerSupport is false in Release mode, we still get the linked symbols, in case the developer wants to archive the linked symbols.

cc @sbomer @vitek-karas

Copy link
Member

@sbomer sbomer Jul 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Seems reasonable to me - I doubt that processing symbols significantly changes link time. Curious that the logic to remove symbols from publish isn't shared.

Copy link
Contributor

Choose a reason for hiding this comment

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

If the trimmer already has support for removing symbols, it might be useful to just piggy back on it. Would it help if we removed BlazorWebAssemblyEnableDebugging and defaulted DebuggerSupport to false?

Copy link
Member Author

Choose a reason for hiding this comment

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

Would it help if we removed BlazorWebAssemblyEnableDebugging

I think this makes sense - might as well use the established properties in the core SDK and the runtime.

and defaulted DebuggerSupport to false?

Is there ever a case where you'd want to preserve .pdbs for later debugging or perf sampling? I guess if users wanted symbols (but wanted to trim out the debug-only code for smaller apps) they could just set TrimmerRemoveSymbols themselves. I guess the current default behavior in Blazor WASM is to not get symbols. So I think that would be fine.

Copy link
Member

Choose a reason for hiding this comment

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

In general I agree, only one concern:

  • People will probably not realize that the build doesn't produce PDBs at the beginning
  • They will only find out once they actually need the PDB for anything... but then it's too late as the deployed builds of the app won't have it
  • Making it opt-in solves the problem only partially (for those who can redeploy and repro the issue)

I guess it depends on how we think people we use the PDBs for Blazor (and if they bring any value).

For example for server .NET Core apps (ASP.NET) I personally would definitely vote for always producing PDBs and just not bundling them to the final package.

Copy link
Member Author

@eerhardt eerhardt Jul 15, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I guess it depends on how we think people we use the PDBs for Blazor (and if they bring any value).

One place this brings value is when we start producing linker warnings. If the pdb is available, the linker will give source and line info in the warning. The developer will see the exact line of code causing the warning. That's super useful IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

My thinking is that having the .pdbs available somewhere - e.g. in the obj folder - by default, even if they are removed from the published assets, is useful. Honestly, I'm not sure why dotnet publish puts symbols in the publish folder by default.

https://github.com/dotnet/sdk/blob/44e5d1aa5ee51726096156f9c0d49e3b90c33f01/src/Tasks/Microsoft.NET.Build.Tasks/targets/Microsoft.NET.Publish.targets#L484

Copy link
Contributor

Choose a reason for hiding this comment

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

My thinking is that having the .pdbs available somewhere - e.g. in the obj folder - by default, even if they are removed from the published assets, is useful.

Those should still be around. We don't prevent creating the pdbs, just stop it from being copied to the publish output.

If the pdb is available, the linker will give source and line info in the warning

Couldn't the linker still use pdbs and then discard these?

Copy link
Member Author

Choose a reason for hiding this comment

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

Couldn't the linker still use pdbs and then discard these?

If TrimmerRemoveSymbols is set to true, the linker won't update the symbols for the linked assemblies. Having the pre-trimmed symbols won't do you any good if you are using the trimmed assemblies - they won't match.

Copy link
Member

Choose a reason for hiding this comment

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

Re source line info from linker: For this the linker needs the pdbs on the input - it doesn't matter if the linker itself will not write a pdb for this feature. That said I agree that having the pdbs (after linking) around somewhere is a good thing.


<!-- Runtime feature defaults to trim unnecessary code -->
<EventSourceSupport Condition="'$(EventSourceSupport)' == ''">false</EventSourceSupport>
<UseSystemResourceKeys Condition="'$(UseSystemResourceKeys)' == ''">true</UseSystemResourceKeys>
<EnableUnsafeUTF7Encoding Condition="'$(EnableUnsafeUTF7Encoding)' == ''">false</EnableUnsafeUTF7Encoding>
Comment on lines +31 to +33
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I know it's not related to this change, but it just caught my eye... the naming is so inconsistent. We have "SomethingSupport" then "UseSomething" and then also "EnableSomething". Would be nice to have some system to this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The difference between "SomethingSupport" and "EnableSomething" is what the default behavior is. By default, "EventSource", "Debugger", and "HttpActivityPropagation" are all supported by default. UTF7 Encoding is disabled by default, and you need to enable it to use it.

Since the default is enabled, it wouldn't make sense to say EnableDebugger. The corresponding word would be DisableDebugger, but I got feedback in dotnet/runtime#37288 (comment) to make it a positive name, and not a negative name. So I followed the same naming with EventSource and HttpActivityPropagation.

Side Note on EnableUnsafeUTF7Encoding: ILLink.Substitutions + default feature switch values don't work very well with the way we link the runtime libraries during the build. We would need a way to ignore the substitution during the runtime build, but still embed it into the assembly. I had a conversation with @sbomer about this, and we decided that for now, setting EnableUnsafeUTF7Encoding to the default value here is probably the best option for 5.0.

UseSystemResourceKeys is a bit of an outlier. It isn't a "support" or an "enable". It kind of fits in an "Other" category with InvariantGlobalization.

Any suggestions on how to make this better?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe EnableSystemResourceTrim - but it's not exactly the same...
Honestly I'm just glad we obviously thought about it 😄

Re having susbtitutions which are not applied during library build - I would have to think about this some more, but I'm definitely willing to add something to the linker to make this possible. CoreFx is special in that we ship the linked binaries, but I do expect other libraries to run linker for the purposes of analysis - so they might run into similar problem.

<HttpActivityPropagationSupport Condition="'$(HttpActivityPropagationSupport)' == ''">false</HttpActivityPropagationSupport>
<DebuggerSupport Condition="'$(DebuggerSupport)' == '' and '$(Configuration)' == 'Release'">false</DebuggerSupport>

<StaticWebAssetBasePath Condition="'$(StaticWebAssetBasePath)' == ''">/</StaticWebAssetBasePath>
<BlazorCacheBootResources Condition="'$(BlazorCacheBootResources)' == ''">true</BlazorCacheBootResources>
Expand Down Expand Up @@ -414,7 +422,7 @@ Copyright (c) .NET Foundation. All rights reserved.
Condition="'%(ResolvedFileToPublish.RelativePath)' != 'web.config' AND !$([System.String]::Copy('%(ResolvedFileToPublish.RelativePath)').Replace('\','/').StartsWith('wwwroot/'))" />

<!-- Remove pdbs from the publish output -->
<ResolvedFileToPublish Remove="@(ResolvedFileToPublish)" Condition="'$(BlazorWebAssemblyEnableDebugging)' != 'true' AND '%(Extension)' == '.pdb'" />
<ResolvedFileToPublish Remove="@(ResolvedFileToPublish)" Condition="'$(DebuggerSupport)' == 'false' AND '%(Extension)' == '.pdb'" />
</ItemGroup>

<ItemGroup Condition="'@(ResolvedFileToPublish->AnyHaveMetadataValue('RelativePath', 'web.config'))' != 'true'">
Expand Down