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

Native host warnings fix #52109

Merged
merged 13 commits into from
May 15, 2021
Merged

Native host warnings fix #52109

merged 13 commits into from
May 15, 2021

Conversation

LakshanF
Copy link
Member

@LakshanF LakshanF commented Apr 30, 2021

Addressed the ComponentActivator suppressed warnings by bubbling them up to entry points. We were not rooting ComponentActivator and InMemoryAssemblyLoader that can be called by native hosts. Rooted these 2 types behind a feature switch similar to StartupHookProvider. These code should be used in rare circumstances with trimmed applications and the msbuild property related to this indicate that and we will generate a warning if trimming with the property set to true.

The full details are as below:

  1. In the default .NET application (non-trim) scenario, the runtime libraries, specifically this API, will behave as normal and a native host can call into this API to create native delegates to managed APIs and expect it to work. Specifically, developers should not see any behavioral differences to what they expect from this feature in normal usage. The vast majority of developers who use these native hosting entry points should not trim their applications and hence the the below steps will not be visible or impact them in any way.
  2. The library will have the 'dangerous when trimming API marker' on the entry points. We explicitly root these entry points so that they will not be trimmed away for the very rare scenarios (step 4 below) where a developer wants to leverage this functionality and also want to trim their application.
  3. When a developer trims an application, we use the msbuild property to define what would happen to the API. We plan to set the default property of the msbuild property to unroot the API (since this API is meant to be called from unmanaged callers only, there will be no reference from managed code to this API). The 'unroot' behavior is defined when IsSupported is set to false (these APIs will not work). The default behavior will also mean that for all developers who want to trim their application AND do not use these native host entry points, they will not see any trimmer warnings related to this API.
  4. A developer has the ability to set the msbuild property to keep the API as is (IsSupported will be true) when trimming. We assume the developer knows how to make the trimmed application work in this case (specifically, the managed API they are going to call from native side will exist). The trimmer warnings will now light up on this scenario to give some indication of the potential problem to the developer. Aside: the warning message should be something like "really???" but we use technical words along the line of "method might be missing..." since we expect developers who leverage this - calling this API on a self-contained trimmed applications - to know what they are doing and want to trim their application.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

Native host warnings fix
@LakshanF
Copy link
Member Author

LakshanF commented May 4, 2021

@elinor-fung, we expect trimming C++/CLI Assembly scenarios to be extremely rare and hence the property being prefixed with underscore. I added some description to the PR header to indicate the flow

@vitek-karas
Copy link
Member

Re C++/CLI - I think we should be careful here. There are two distinct scenarios which have very different probabilities:

  • Load C++/CLI into a native process (the process starts as native, the C++/CLI is called through native and thus has to initialize runtime on its own). This is basically never going to happen with trimming. Having the feature switch for just this - I would absolutely agree we can "hide" it.
  • Load C++/CLI into a managed process from managed code (Assembly.Load or direct assembly-ref). I personally don't know if this codepath hits the APIs in this review. If it doesn't - we're fine - only the first case exist. If it does go through the same code paths - that will change things a lot. For example every single WPF app falls into this category - we absolutely want to support trimmed self-contained WPF apps in the future.

I think we need to find out which codepath is used when loading C++/CLI from managed code - the answer to that will answer what we need to do here.

@elinor-fung
Copy link
Member

@LakshanF here's that basic example of a C# app using a C++/CLI library: https://github.com/elinor-fung/basic-cpp-cli
It shows some ways that the library can be loaded - through p/invoke or NativeLibrary which should go through the code path in this PR and through Assembly.Load or a direct reference which should not go through the code path in this PR.

docs/design/features/IJW-activation.md Outdated Show resolved Hide resolved
docs/workflow/trimming/feature-switches.md Outdated Show resolved Hide resolved
@@ -51,6 +51,7 @@ enum StatusCode
HostPropertyNotFound = 0x800080a4,
CoreHostIncompatibleConfig = 0x800080a5,
HostApiUnsupportedScenario = 0x800080a6,
HostFeatureDisabled = 0x800080a7,
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Should have been there in my commit

LakshanF and others added 4 commits May 6, 2021 07:46
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
…rs.xml

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
…rs.xml

Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
Co-authored-by: Vitek Karas <vitek.karas@microsoft.com>
Copy link
Member

@elinor-fung elinor-fung left a comment

Choose a reason for hiding this comment

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

Would it be worth adding a hosting test that turns off the ComponentActivator feature switch? Or is something like that already wrapped up in broader testing somewhere?

@LakshanF LakshanF merged commit 127be4b into dotnet:main May 15, 2021
@LakshanF LakshanF deleted the NativeHostWrn branch May 15, 2021 20:26
@karelz karelz added this to the 6.0.0 milestone May 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jun 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants