Added linker hint for DirectWriteForwarder #4353
Conversation
ghost
requested review from
|
This wasn't missed.It wasn't included - that's all. 😄 We didn't want to include a lot of things by default - so we included what we thought was a reasonable min-set at that time. If every WPF app in .NET 5 or .NET 6 is encountering a crash during trimming due to the lack of this entry, then it makes sense to add it here. It will cause bloat for self-contained apps, but it will make trimming more reliable. It may be a good trade-off to make. If only some WPF apps are encountering this crash, but they represent nearly all of the real-world WPF apps, then it may still make sense think about what our defaults ought to be and be somewhat reflective about this decision. Questions like (i) what other assemblies should be included in this list of "needed by real world apps", (ii) should that list be incorporated into WPF directly as liker hints or should it be added as a documentation somewhere, (iii) should such a list perhaps be incorporated into MSBuild targets in the trimming related logic in the SDK someplace that only real-world apps can opt-into etc. should be considered and answered. If this is a one-off problem encountered by only a few apps, then I don't think it should be added here at all. I'm sure there is some way for apps to directly specify linker hints like this - for e.g. /cc @danmoseley, @dsplaisted |
|
@vatsan-madhavan Sorry, couldn't see in your PR if it was intentional or missed 😄. The issue is for an exception that causes the application to crash when a user tries to copy the content of a TextBox (I didn't test for other controls or manipulations other than Copy). Also, the exception is almost impossible to diagnose (See the issue description), which makes it more difficult for the developer to fix it, contrary to #4321, which points to the missing assembly. I wouldn't expect DirectWriteForwarder to change a lot in the future so there probably won't be another assembly included for trimming like this. I would expect any real-word application to come accross this exception. I think the size trade-off is very minimized by the size of the dll (23,5 Kb).
I was able to fix my application using I'm happy to close this PR if the WPF team judges that it's not worth it but IMO, it is. (I might be biased because I spent lot of time finding the source of the exception 😄 ) |
|
I should also mention that the size of |
|
cc @eerhardt who can better answer your question/provide guidance. |
|
I tried a simple textbox and it crashes immediately on Ctrl+C. Something as simple as this: <Window x:Class="test.MainWindow"
xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
xmlns:d="http://schemas.microsoft.com/expression/blend/2008"
xmlns:mc="http://schemas.openxmlformats.org/markup-compatibility/2006"
xmlns:local="clr-namespace:test"
mc:Ignorable="d"
Title="MainWindow" Height="450" Width="800">
<Grid>
<TextBox HorizontalAlignment="Center" Text="TextBox" TextWrapping="Wrap" VerticalAlignment="Center"/>
</Grid>
</Window>Including this it as a dependency of DirectWriteForwarder makes sense given that's the assembly responsible for most of text functionality. I think it will also affect any other non-text copy/paste scenario given this is the crash stack I'm seeing: Ultimately the team should sign off on this approach - to me this looks to be something that will affect any WPF app and so looks like a reasonable fix. |
|
cc @mateoatr - who is working on analyzing C++/CLI in https://github.com/mono/linker. Once that work is done, this ILLinkTrim.xml workaround file should be able to be removed. |
pchaurasia14
added
the
Community Contribution
|
@ThomasGoulet73 since the related issues are closed, and as mentioned in #1392, it looks like we can safely remove this workaround. Can you check this and modify the PR accordingly? |
|
I'm not sure. The best way to check would be to remove it and test if it still works. cc @agocke |
|
@ThomasGoulet73 - We tried to repro the original issue mentioned in this PR and it looks like the issue has already been addressed by changes in dotnet/linker#676. I think we should close this PR. Additionally, we need to create another PR that reverts the changes in #1387 . |
|
@ThomasGoulet73 any comments regarding this? |
|
@rchauhan18 @dipeshmsft: I'm still able to reproduce the issue. Here's a simple repro: MainWindow.xamlWpfApp1.csprojWhen publishing with |
|
@ThomasGoulet73 I have created one sample application (Wpfapp1) with the details given by you. I am not able to repo it. Dotnet versionVisual Studio versionPublish Command |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
Thanks @ThomasGoulet73 for your contribution. |
ghost
locked as resolved and limited conversation to collaborators
Fixes Issue #3903
Description
I added a linker hint for a dependency of DirectWriteForwarder. The linker does not currently C++/CLI assembly (See dotnet/linker#676). This assembly might have been missed by #1387 because the exception does not occur at startup.
Customer Impact
Adds a 23,5 Kb dll when using the linker.
Regression
I don't think it ever worked.
Testing
I tested using a simple repro that failed with
<TrimMode>link</TrimMode>and it worked with<TrimmerRootAssembly Include="System.Runtime.InteropServices" />.Risk
I don't think there is a risk, it only sets an assembly as never trimmed when a WPF application is trimmed.