search

CommunityToolkit / Microsoft.Toolkit.Win32

ARCHIVE - This repository contained XAML Islands wrapper controls and tooling for XAML Islands with WinUI 2, see readme for more info about XAML Islands with WinUI 3 and the WindowsAppSDK.
https://aka.ms/windowsappsdk
Other
383 stars 89 forks source link

Remove VCRTForwarders dependency #251

Closed sylveon closed 4 years ago

sylveon commented 4 years ago

Fixes #150

PR Type

What kind of change does this PR introduce?

What is the current behavior?

The current XamlHost builds against the app family and CRT despite being expected to run under a Win32 environment. This was worked around by pulling in DLLs which forward to the desktop CRT and manually forward declaring methods from the desktop family.

What is the new behavior?

It builds against the desktop family and CRT

PR Checklist

Please check if your PR fulfills the following requirements:

ghost commented 4 years ago

Thanks sylveon for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

sylveon commented 4 years ago

Was able to grab the NuGet package from the pipeline and I can correctly dogfood it: image

sylveon commented 4 years ago

This PR has been open for more than a month. Any chance at getting a review?

michael-hawker commented 4 years ago

@azchohfi @ocalvo @marb2000 we just updated the forwarders for another issue, so we still need them for certain scenarios, eh?

marb2000 commented 4 years ago

@sylveon, VC Forwarders are needed when your app is unpackaged and you are using at least one XAML control writing in C++ (this include WinUI 2). So, if you are using WinUI 2 your app will need to use the VC Forwarders (the 80% of the scenarios). For this reason, we added them. Does it make sense?

sylveon commented 4 years ago

They are not always needed.

If your projects and controls use the same undocumented configuration directive than in this PR (assuming only system XAML, not WinUI), they work with the desktop CRT. The only blocker to removing the forwarders then becomes this package.

Since this package is specifically for desktop apps where the desktop CRT will always be available, it makes no sense to force the forwarders upon all consumers.

People who still need the forwarders for WinUI or third party controls depending on the app CRT can still install them as always, and people who don't can completely remove them from their builds after this PR is merged.

marb2000 commented 4 years ago

@sylveon, I looked at the behavioral insights, and it shows that the majority of customers using Islands are using WinUI 2 too. Hence, removing the VCRTForwarders NuGet will make that majority of customers need to add it. The scenario that you describe is important, but it's an edge scenario. I believe that we should keep the VCRTForwarders reference given it has more impact and makes the thing easier for most developers.

Does this make sense?

sylveon commented 4 years ago

Developers using WinUI already need to manually add the VCRTForwarders package, because it is not specified as a dependency of this package on NuGet, so it is not automatically acquired when installing the package.

For those people, nothing changes. For people not using WinUI, it's one less dependency and potential headache.

marb2000 commented 4 years ago

No sure about it @sylveon, I have just tested it and I didn't need to load manually the VCRTForwarders in a WPF sample. I could follow these instructions successfully.

Actually, make sense I don't need it because we are adding the package reference:

 <PackageReference Include="Microsoft.VCRTForwarders.140" Version="1.0.6">
      <PrivateAssets>analyzers</PrivateAssets>
    </PackageReference>

Perhaps I'm missing something.

sylveon commented 4 years ago

The forwarders NuGet package is not required on .NET because .NET already includes its own (see https://github.com/windows-toolkit/Microsoft.Toolkit.Win32/issues/150#issuecomment-522936760). I am talking about the native (C++-only) scenario.

Moreover, the PackageReference you referred to is in the project file but does not result in it being specified as a dependency on the NuGet package (as pointed out on NuGet), so consumers do not automatically import the forwarders when installing Microsoft.Toolkit.Win32.UI.XamlApplication.

There is a second PackageReference in Microsoft.WinRT.Win32.targets: this file is part of the Microsoft.Toolkit.Win32.UI.SDK NuGet package, which is not referred anywhere by the guide you linked. I would be OK with adding it back here, since I don't use that package and it would not break the implicit dependency some people might have acquired due to it.

marb2000 commented 4 years ago

The forwarders NuGet package is not required on .NET because .NET already includes its own (see #150 (comment)). I am talking about the native (C++-only) scenario.

I don't think this is accurate. Any time you want to use a non-system C++ WinRT Component in a non-MSIX app you need to use the VCRTForwarders, the Windows Runtime will look for the VCLibs (the Store version of the VC++ Runtime Libraries), and because there is no Store (given it doesn't have a package.manifest), the VCRTForwarders will forward the calls to the VC++ Runtime Libraries installed on the machine. So .NET Core 3 and native apps (C++ apps) that requires to use, for instance WinUI 2 or Win2D, will need the forwarders.

Another topic is about why .NET apps don't require the VC++ Runtime Libraries even when they have parts written in C++. If I don't recall wrong, I think it's because .NET has a sort of "static" linked of a flavor of the VC++ Runtime Libraries, like WPF has.

About this:

There is a second PackageReference in Microsoft.WinRT.Win32.targets: this file is part of the Microsoft.Toolkit.Win32.UI.SDK NuGet package, which is not referred anywhere by the guide you linked

In the doc , there is a step that requires to use the Microsoft.Toolkit.Wpf.UI.XamlHost that has a dependency on the Microsoft.Toolkit.UI.XamlHost NuGet, and this NuGet has a dependency on Microsoft.Toolkit.Win32.UI.SDK.

So we circle back to the main point, If we remove the dependency of the VCRTForwarders package, we will make the most developers need to install it manually, versus a having residual number of developers that need to deal with this issue. Do you see here the tradeoff? 😉

sylveon commented 4 years ago

So why not remove it from Microsoft.Toolkit.Win32.UI.XamlApplication but keep the PackageReference in Microsoft.Toolkit.Win32.UI.SDK? Then it breaks no consumers that I can think of (people using XamlApplication only already need to manually install the forwarders currently, and people who use the SDK package still have the forwarders) and enables my scenario.

sylveon commented 4 years ago

At this point I don't even care anymore because I managed to completely get rid of the XamlApplication package from my app.