Add the FX_RESOLUTION Runtime property #644

swaroop-sridhar · 2019-12-07T00:09:21Z

As part of https://github.com/dotnet/core-setup/issues/4112, this change modifies hostpolicy
to pass an additional property describing framework-resolution
to the runtime.

For each framework resolved (if any), FX_RESOLUTION notes:
Framework: <name>, Requested: <min-requested-version>, Resolved: <actual-resolved-version>;

vitek-karas

Missing tests

vitek-karas · 2019-12-09T11:06:20Z

src/installer/corehost/cli/hostpolicy/coreclr.cpp

@@ -203,7 +203,8 @@ namespace
        _X("JIT_PATH"),
        _X("STARTUP_HOOKS"),
        _X("APP_PATHS"),
-        _X("APP_NI_PATHS")
+        _X("APP_NI_PATHS"),
+        _X("FX_RESOLUTION")


Nit: I would probably call this RESOLVED_FRAMEWORKS.

vitek-karas · 2019-12-09T11:25:43Z

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp

@@ -125,6 +125,26 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
        ++fx_curr;
    }

+    pal::string_t fx_resolution;
+    bool is_app = true;
+    for (const auto& fx : fx_definitions)


This can be folded into the existing loop which wills app_context_deps_str right above this as that already iterates over all FX definitions. There will need to be some changes around the "app" framework, but in general it should be done in one place as it's basically the same thing.

swaroop-sridhar · 2019-12-12T23:36:45Z

Thanks @vitek-karas, I've made the changes you suggested.

vitek-karas · 2019-12-13T10:45:13Z

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp

@@ -114,13 +114,29 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
    fx_definition_vector_t::iterator fx_end;
    resolver.get_app_fx_definition_range(&fx_begin, &fx_end);


This excludes the "App" when in libhost mode - so the loop below would actually skip the first framework.
We may need to tweak the behavior of this helper in correspondence with the loop.

vitek-karas · 2019-12-13T10:47:27Z

src/installer/test/HostActivation.Tests/FrameworkResolution/ResolvedFrameworksProperty.cs

+        }
+
+        [Fact]
+        public void ResolvedFrameworksPropertyIsComputedForPortableApps()


Can you please add a test for cases where the requested version doesn't match the resolved version?
Ideally also a test with multiple resolved frameworks, but that is a bit harder to do.

As part of core-setup/dotnet#4112, this change modifies hostpolicy to pass an additional property describing framework-resolution to the runtime. For each framework resolved (if any), FX_RESOLUTION notes: Framework: <name>, Requested: <min-requested-version>, Resolved: <actual-resolved-version>;

* Fix HostPolicy change to incorporate libhost mode * Update test with a case where requested and actual frameworks are different Add some more changes Test Test with different netcoreapp versions

vitek-karas · 2020-01-20T08:16:06Z

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp


-        app_context_deps_str += (*fx_curr)->get_deps_file();
-        ++fx_curr;
+        app_context_deps_str += fx->get_deps_file();


Before your change the app.deps.json was not included in this string when running in libhost mode (since only real frameworks should be reported then). Now it will include it.
Can you please adapt some of the existing libhost tests and validate this property and that it doesn't regress with this change?

vitek-karas · 2020-01-20T08:16:34Z

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp

@@ -114,15 +114,38 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
    fx_definition_vector_t::iterator fx_end;
    resolver.get_app_fx_definition_range(&fx_begin, &fx_end);


If I read it correctly these are not used anymore.

elinor-fung · 2020-01-21T18:17:21Z

src/installer/corehost/cli/hostpolicy/hostpolicy_context.cpp

    pal::string_t app_context_deps_str;
-    fx_definition_vector_t::iterator fx_curr = fx_begin;
-    while (fx_curr != fx_end)
+    bool is_app = host_mode != host_mode_t::libhost;


Doesn't the first element in fx_definitions represent the app even in libhost mode?

elinor-fung · 2020-01-21T18:20:07Z

src/installer/test/Assets/TestProjects/PortableApp21/PortableApp21.csproj

@@ -0,0 +1,14 @@
+<Project>
+
+  <Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />


Do we need to explicitly do the imports for some reason instead of just setting the Sdk attribute on the project?

stephentoub · 2020-02-17T14:51:53Z

@swaroop-sridhar, still working on this? Thanks.

swaroop-sridhar · 2020-02-18T21:06:33Z

@stephentoub Yes I'm working on this PR and the related #690 in the background.

swaroop-sridhar requested review from elinor-fung and vitek-karas December 7, 2019 00:09

swaroop-sridhar force-pushed the fxres branch 2 times, most recently from a7f27d2 to 8eb9794 Compare December 7, 2019 01:14

jkotas added the area-Host label Dec 7, 2019

vitek-karas requested changes Dec 9, 2019

View reviewed changes

vitek-karas requested changes Dec 13, 2019

View reviewed changes

swaroop-sridhar added 4 commits January 8, 2020 18:36

Address feedback

fb1dfbf

Address feedback by @vitek-karas

fde14bd

Addresss additional feedback

6b5ee8b

* Fix HostPolicy change to incorporate libhost mode * Update test with a case where requested and actual frameworks are different Add some more changes Test Test with different netcoreapp versions

swaroop-sridhar force-pushed the fxres branch from 9a740e0 to 6b5ee8b Compare January 18, 2020 02:42

vitek-karas requested changes Jan 20, 2020

View reviewed changes

elinor-fung reviewed Jan 21, 2020

View reviewed changes

vitek-karas mentioned this pull request Mar 11, 2020

Add new API to expose framework information about the current app #33472

Open

swaroop-sridhar closed this Mar 13, 2020

rseanhall mentioned this pull request Apr 28, 2020

How to locate coreclr.dll from hostfxr_initialize_for_runtime_config #34946

Closed

swaroop-sridhar mentioned this pull request Sep 4, 2020

Log details of app being launched by dotnet(.exe) to ETW/LTTng #3156

Open

ghost locked as resolved and limited conversation to collaborators Dec 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add the FX_RESOLUTION Runtime property #644

Add the FX_RESOLUTION Runtime property #644

swaroop-sridhar commented Dec 7, 2019 •

edited

Loading

vitek-karas left a comment

vitek-karas Dec 9, 2019

vitek-karas Dec 9, 2019

swaroop-sridhar commented Dec 12, 2019

vitek-karas Dec 13, 2019

vitek-karas Dec 13, 2019

vitek-karas Jan 20, 2020

vitek-karas Jan 20, 2020

elinor-fung Jan 21, 2020

elinor-fung Jan 21, 2020

stephentoub commented Feb 17, 2020

swaroop-sridhar commented Feb 18, 2020 •

edited

Loading

		@@ -114,13 +114,29 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
		fx_definition_vector_t::iterator fx_end;
		resolver.get_app_fx_definition_range(&fx_begin, &fx_end);

		@@ -114,15 +114,38 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a
		fx_definition_vector_t::iterator fx_end;
		resolver.get_app_fx_definition_range(&fx_begin, &fx_end);

		@@ -0,0 +1,14 @@
		<Project>

		<Import Project="Sdk.props" Sdk="Microsoft.NET.Sdk" />

Add the FX_RESOLUTION Runtime property #644

Add the FX_RESOLUTION Runtime property #644

Conversation

swaroop-sridhar commented Dec 7, 2019 • edited Loading

vitek-karas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swaroop-sridhar commented Dec 12, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub commented Feb 17, 2020

swaroop-sridhar commented Feb 18, 2020 • edited Loading

swaroop-sridhar commented Dec 7, 2019 •

edited

Loading

swaroop-sridhar commented Feb 18, 2020 •

edited

Loading