Expose function for locating hostfxr by elinor-fung · Pull Request #5522 · dotnet/core-setup

elinor-fung · 2019-03-21T06:41:20Z

Add nethost library
Implement nethost_get_hostfxr_path
Add basic test for (a native host using) nethost

sdmaclea · 2019-03-21T21:29:32Z

+bool fxr_resolver::try_get_path(const host_mode_t mode, const pal::string_t& root_path, pal::string_t* out_dotnet_root, pal::string_t* out_fxr_path)
 {
    pal::string_t fxr_dir;
-#if FEATURE_APPHOST || FEATURE_LIBHOST


We go to great lengths to avoid breaking changes to the muxer. I wonder if we should keep some #if here. Something like:

#if !FEATURE_MUXER

Or maybe it would be better to leave the interface unchanged. Keep most of the original code and interface. I see no significant functional change to this file, just refactoring for cleanup.

bool resolve_fxr_path(const pal::string_t& root_path, pal::string_t* out_dotnet_root, pal::string_t* out_fxr_path)

Except two line removed above.

I think I'm missing what the breaking change is here?

The functionality / logic should be the same. For nethost, we need to be able to resolve using either the muxer behaviour or the apphost/libhost behaviour, so I removed the preprocessor conditions here. But corehost.cpp is setting the mode based on the defines such that the behaviour should be the same (and com/ijwhost, which have FEATURE_LIBHOST, are always using host_mode_t::libhost)

This isn't an exported function. There is no breaking change here.

We spent months getting approval to make changes to the muxer. We had to justify changes to

Add the manifest

Add logging to file support

Add support for SemVer 2.0.0

We could logically reason that none of these were breaking.

This PR is introducing a non-functional change to the muxer. I agree it shouldn't be breaking, but it does change muxer code. It is introducing a switch and dead code which was not compiled for the muxer before.

If there was a functional reason for the code change, it is OK to keep the switch. I would still think about restoring an #if to eliminate the dead code for the muxer. I'll let @vitek-karas decide if I am being overly cautious.

We spent months getting approval to make changes to the muxer.

Not for comhost which had much more impactful functional changes - the very notion of libhost for instance.

Not for comhost which had much more impactful functional changes - the very notion of libhost for instance.

Only because we didn't mention they affected the muxer. If there are changes to the muxer, we must review them carefully. @jeffschwMSFT wants us to make sure there is sufficient coverage to guarantee the muxer works properly for every shipping .NET Core version.

Only because we didn't mention they affected the muxer.

That just isn't true. The impact of comhost on the muxer was discussed extensively.

I agree that coverage is an important consideration. I am hoping the start of the native testing here will allow us to do just that. @vitek-karas also mentioned you had started work on some kind of native testing. Is your work applicable here?

That just isn't true

Offline

Is your work applicable here?

The unit and E2E tests are checked in. In my opinion they were sufficient to test the SemVer 2.0.0 work.

I agree we need to be extra careful. That said we should not shy away from refactorings and other code changes which will improve the maintainability of the code base.

This particular change - I'm perfectly fine with it. The dead code concern is valid, but I don't think it's important enough to pollute the code with additional ifdefs and such. Yes - it will make dotnet.exe slightly larger, but the difference should be trivial - dotnet.exe size is not a big concern as long as it's reasonable. It will also make other hosts slightly larger, and in this case it matters a bit more - we should keep these hosts as small as possible. But the code we're adding is very small.

Re testing: We absolutely need to add more coverage for hosts - in general, but dotnet.exe and hostfxr specifically.

The tests @elinor-fung added for this specific feature are OKish. We will need to add a much larger coverage around the "finding hostfxr" algorithms. But it might be better to do that once we have agreement on 3.0 behavior - see dotnet/designs#58.

As for backward compatibility - we are testing this end-to-end as well. SDK has several tests targeting down-level apps. There's also AppCompat testing which is adding coverage for down-level scenarios.

Since it turned out we didn't need the muxer behaviour in nethost after all, I went back to the preprocessor conditions.

AaronRobinsonMSFT · 2019-03-21T22:37:00Z

+bool fxr_resolver::try_get_path(const host_mode_t mode, const pal::string_t& root_path, pal::string_t* out_dotnet_root, pal::string_t* out_fxr_path)
 {
    pal::string_t fxr_dir;
-#if FEATURE_APPHOST || FEATURE_LIBHOST


This isn't an exported function. There is no breaking change here.

vitek-karas · 2019-03-22T11:49:26Z

+bool fxr_resolver::try_get_path(const host_mode_t mode, const pal::string_t& root_path, pal::string_t* out_dotnet_root, pal::string_t* out_fxr_path)
 {
    pal::string_t fxr_dir;
-#if FEATURE_APPHOST || FEATURE_LIBHOST


I agree we need to be extra careful. That said we should not shy away from refactorings and other code changes which will improve the maintainability of the code base.

This particular change - I'm perfectly fine with it. The dead code concern is valid, but I don't think it's important enough to pollute the code with additional ifdefs and such. Yes - it will make dotnet.exe slightly larger, but the difference should be trivial - dotnet.exe size is not a big concern as long as it's reasonable. It will also make other hosts slightly larger, and in this case it matters a bit more - we should keep these hosts as small as possible. But the code we're adding is very small.

Re testing: We absolutely need to add more coverage for hosts - in general, but dotnet.exe and hostfxr specifically.

The tests @elinor-fung added for this specific feature are OKish. We will need to add a much larger coverage around the "finding hostfxr" algorithms. But it might be better to do that once we have agreement on 3.0 behavior - see dotnet/designs#58.

As for backward compatibility - we are testing this end-to-end as well. SDK has several tests targeting down-level apps. There's also AppCompat testing which is adding coverage for down-level scenarios.

vitek-karas · 2019-03-22T11:55:25Z

+        #define NETHOST_API __declspec(dllimport)
+    #endif
+
+    #define NETHOST_CALLTYPE __stdcall


This is different from all the other libraries (hostfxr/hostpolicy) which I think use _cdecl. @AaronRobinsonMSFT is the expert here.
I'm not against this - I think having __stdcall is better for interoperability (Which is the main goal here) - I just want to make sure that this is intentional and intentionally different.

@vitek-karas Bah... Yes, you are right. The rest of the core-setup API is __cdecl. I agree that using __stdcall is more consistent with coreclr.dll, comhost, ijwhost, and the proposed winrthost exports. I will say this is annoying since all of hostpolicy and hostfxr are __cdecl. So much annoyance here. Thanks for pointing this out. I think we should keep with __stdcall since it aligns with all non "internal" core-setup scenarios better. I consider interacting directly with hostfxr and hostpolicy more "internal" or at the very least "advanced" as compared to COM, IJW, WinRT, and the native loading scenarios which users could experiment with.

Agreed - and I like the consistency among the "public" hosts. hostpolicy is not going to be used externally (it's just too hard to use directly). hostfxr uses will be advanced and so some complexity is OK.

@elinor-fung In a previous review @vitek-karas mentioned the need for documentation of these kinds of contracts. I think the fact that we are starting to coalesce around common user scenarios using __stdcall on x86 is an important contract that should be documented and officially defined. I have observed you reading through the documentation of core-setup, is there an existing doc that could contain these kind of details or is a new one needed?

I'd say we need a new one. Maybe what gets created for #5481 would be a reasonable place (if it is a doc for all exposed APIs - I could also see doing a separate 'advanced' hostfxr/hostpolicy doc and 'public' *host doc)

vitek-karas · 2019-03-22T11:56:23Z

+//
+// Return value:
+//   0 on success, otherwise failure
+//


👍 - it's great that you wrote this as a header which we can give out to external users of the library.

vitek-karas · 2019-03-22T11:59:04Z

+    using char_t = char;
+#endif
+
+using nethost_get_hostfxr_path_result_fn = void(*)(const char_t * hostfxr_path);


I know a proposed this in the design doc, but now I'm a bit hesitant.
Using the callback makes it harder to use from some languages. It means the caller has to store the value in a global variable, which introduces issues.

Maybe we should switch to the caller supplied buffer. The downside is that if the buffer is two small and two calls are required, we pay the price of searching the disk twice...
Not sure - but I slightly prefer the caller supplied buffer as it makes the caller much more flexible. And if we add a comment to supply something like MAX_PATH buffer on the first call, it should almost never happen that we would need a second call.

Using the callback makes it harder to use from some languages. It means the caller has to store the value in a global variable, which introduces issues.

I would agree with this sentiment.

Switched to a buffer + size supplied by the caller.

vitek-karas · 2019-03-22T12:26:01Z

+    pal::string_t root_path;
+    if (assembly_path == nullptr)
+    {
+        mode = host_mode_t::muxer;


Sorry - this is my bad.
We can't use the muxer behavior here. The muxer behavior assumes that the dotnet.exe is called and thus it searches next to it to find hostfxr - and nowhere else. That's effectively useless for the nethost scenarios, because the nethost will typically run in the cotext of some application, which will not have the hostfxr next to it.

We will need to add some more tweaks. I think the proper behavior should be:

assembly_path is nullptr: basically the apphost behavior but without the "root_path", so don't look next to itself. But do use DOTNET_ROOT and do look in global locations.

assembly_path specified: exactly like apphost behavior - this is already below.

Updated to stop using the muxer behaviour and follow what you outlined above.

- Look at environment variable and global locations when called without an assembly path (app/libhost behaviour minus app-local search) - Switch back to preprocessor conditions for getting fxr path now that nethost doesn't need different behaviours - Update/add tests for changed behaviour

vitek-karas

Aside from the tracing setup it looks good!

vitek-karas · 2019-03-23T12:09:41Z

+            {
+                result.Should().Fail()
+                    .And.ExitWith(1)
+                    .And.HaveStdOutContaining($"{GetHostFxrPath} failed: 0x{CoreHostLibMissingFailure.ToString("x")}");


You could additionally validate that the error written out is expected - through enabling tracing.

vitek-karas · 2019-03-23T12:17:31Z

+        {
+            if (!RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
+            {
+                // We don't have a good way of hooking into how the product looks for global installations yet.


We will need something here eventually... 👍

vitek-karas · 2019-03-25T17:40:50Z

                .And.HaveStdOutContaining($"hostfxr_path: {hostFxrPath}".ToLower());
        }

+        private class TestRegistryKeyOverride : IDisposable


Can you please expose this to all tests and refactor the one in SDK resolution to use the same class? It does exactly the same thing.

Pulled out into RegisteredInstallKeyOverride class and switched other tests to it.

jkotalik · 2019-03-25T21:29:55Z

+//   The full search for the hostfxr library is done on every call. To minimize the need
+//   to call this function multiple times, pass a large result_buffer (e.g. PATH_MAX).
+//
+extern "C" NETHOST_API int NETHOST_CALLTYPE nethost_get_hostfxr_path(


Is it expected that native hosters carry this dll with them rather than it being app local? Ex if I have a global module, and I wanted to find hostfxr, I would need to have nethost.dll sit side by side with my global module?

Yes, the expectation is that a native host application will ship this library as part of the application itself.

I think I need a bit of clarification. The expectation is that a published app will contain the nethost.dll, like:

published/ nethost.dll myapp.dll

Also, is the nethost.dll opt-in or opt-out?

The nethost.dll is to be part of the native host - so in the case of ASP.NET it will be part of the ASP.NET IIS module.

.NET Core app doesn't need it, since the apphost or muxer will find the hostfxr for it and if it needs the path to hostfxr at runtime it can simply ask for already loaded module.

non .NET Core app which uses hostfxr to host .NET Core (this is what we call "native host") will include it in itself.

Basically the ASP.NET IIS module has the "locate hostfxr" code copied into it today, with this it would just carry it as a separate dll.

We don't have a delivery vehicle yet, but we're thinking native (C/C++) NuGet package and probably just raw .zip with the library and header file for all other cases (Linux/macOS, non-C/C++ languages).

Our IIS module today installs via an msi, so I'd imagine we should probably include it SxS with the module.

Once we get new bits from core-setup, I will start prototyping and see if all scenarios we used to support still work with nethost.dll.

@jkotalik Just to clarify for me

include it SxS with the module

This means that you would package the nethost with your module and install it into the same folder on the target machine... right?

Re the compatibility - if that's not the case (that is you find a case where it works differently then your existing code), please let us know. It's using the same code as apphost and so if there's a problem in nethost it's likely there is a problem in the other "hosts" as well.

This means that you would package the nethost with your module and install it into the same folder on the target machine... right?

Yes I believe that is the best course of action.

Re the compatibility - if that's not the case (that is you find a case where it works differently then your existing code), please let us know. It's using the same code as apphost and so if there's a problem in nethost it's likely there is a problem in the other "hosts" as well.

I'm less concerned about that than some of the backups we did. For example, if all else fails with finding dotnet, we search in C:\PF\dotnet as a backup. So we will see!

* Add nethost library and implement nethost_get_hostfxr_path * Add basic tests for nethost_get_hostfxr_path Commit migrated from dotnet/core-setup@6001915

elinor-fung force-pushed the nethost-hostfxrPath branch 2 times, most recently from ae108e2 to a87235d Compare March 21, 2019 07:01

elinor-fung changed the title ~~[WIP] Expose function for locating hostfxr~~ Expose function for locating hostfxr Mar 21, 2019

elinor-fung requested review from sdmaclea and vitek-karas March 21, 2019 18:23

elinor-fung added 5 commits March 21, 2019 14:05

Empty implementation of nethost

80826c1

Implement nethost_get_hostfxr_path

a840455

Add basic test for nethost

bfff90f

Set rpath

986153b

Update licence header

7501fd8

elinor-fung force-pushed the nethost-hostfxrPath branch from 5b62024 to 7501fd8 Compare March 21, 2019 21:07

sdmaclea reviewed Mar 21, 2019

View reviewed changes

AaronRobinsonMSFT reviewed Mar 21, 2019

View reviewed changes

jkoritzinsky mentioned this pull request Mar 22, 2019

.NET Core WinRT Host #5527

Merged

Collapse CLI_CMAKE_APP/COM/IJW/NETHOST_VER to CLI_CMAKE_COMMON_HOST_VER

e728a62

vitek-karas suggested changes Mar 22, 2019

View reviewed changes

jkoritzinsky reviewed Mar 22, 2019

View reviewed changes

Comment thread src/corehost/cli/nethost/CMakeLists.txt

elinor-fung added 2 commits March 22, 2019 15:21

Switch to caller-supplied buffer instead of callback function

206eed4

elinor-fung force-pushed the nethost-hostfxrPath branch from e659a61 to 206eed4 Compare March 23, 2019 00:40

elinor-fung added 2 commits March 22, 2019 19:16

Fix exit code in tests to work with unix

459e372

Drop nethost.h next to built lib

de05524

vitek-karas suggested changes Mar 23, 2019

View reviewed changes

Set up tracinig. Update tests to handle registered global installations.

4673b68

vitek-karas approved these changes Mar 25, 2019

View reviewed changes

Add shared class for overriding registered install registry key in tests

1dbdece

jkotalik reviewed Mar 25, 2019

View reviewed changes

elinor-fung merged commit 6001915 into dotnet:master Mar 25, 2019

elinor-fung deleted the nethost-hostfxrPath branch March 25, 2019 23:59

jkotalik mentioned this pull request Mar 26, 2019

React to hostfxr locating logic dotnet/aspnetcore#8838

Closed

vitek-karas mentioned this pull request Jan 30, 2020

Move test_fx_ver under the cli/test folder dotnet/runtime#3515

Closed

Conversation

elinor-fung commented Mar 21, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sdmaclea Mar 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vitek-karas left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sdmaclea Mar 21, 2019 •

edited

Loading