Build an apphost with hostfxr and hostpolicy linked in #35368

VSadov · 2020-04-23T22:01:37Z

This is a continuation of #35087

======

This PR introduces a few changes:

It builds most of hostfxr as a static library, with the shared library being linked with this static library and some shared-specific code
It builds hostpolicy as a static library
It abstracts how the host looks for hostfxr functions, either by loading a DLL and finding symbols there, or referencing symbols from the executable directly
Provides a minimal hostpolicy_resolver that merely references some needed hostpolicy functions

One of the concerns with this was about the size of the executable. This table summarizes it:

Executable name	Size in bytes
apphost-linux-x64	181472
static_apphost-linux-x64	570312

ghost · 2020-04-23T22:01:42Z

Tagging subscribers to this area: @vitek-karas, @swaroop-sridhar
Notify danmosemsft if you want to be subscribed.

swaroop-sridhar

I second @vitek-karas's comment on here on #35087 that it is best to leave the implementation and header files in their existing directories, and only create separate CMakeLists files

src/installer/corehost/cli/fxr/shared/CMakeLists.txt

VSadov · 2020-04-24T14:00:59Z

@swaroop-sridhar - I will look at the file moves.
A few files must be in static/shared to avoid conflicts (CMakeLists.txt, hostpolicy_resolver.cpp, ..), but most could be used from ../

davidfowl · 2020-04-24T14:38:39Z

@jkotalik on how this may affect IIS

VSadov · 2020-04-24T15:37:53Z

Scanning dependencies of target hostfxr_exports
  make[2]: *** No rule to make target `/Users/runner/runners/2.166.3/work/1/s/src/installer/corehost/generateexportedsymbols.awk', needed by `cli/fxr/shared/hostfxr.exports'.  Stop.

@janvorli - any idea what is missing for this to work? Why is it looking in corehost?

jkotalik · 2020-04-24T16:40:35Z

Yeah, I'm a bit confused on how this will interact with IIS as well.

In our IIS interop, we rely on calling into hostfxr via pinvoking to call host_fxr_main. By statically linking hostfxr as a lib, will the hostfxr dll now not be present?

janvorli · 2020-04-24T17:15:40Z

@janvorli - any idea what is missing for this to work? Why is it looking in corehost?

Let me take a peek.

janvorli · 2020-04-24T17:23:52Z

Ah, I see. The move of the functions.cmake to eng/native that happened some time ago should have moved the awk script there too. The generate_exports_file function now looks for the script at ${CMAKE_SOURCE_DIR}/${AWK_SCRIPT}. So for coreclr, it is in src/coreclr as the root CMakeLists.txt is there. For corehost, the ${CMAKE_SOURCE_DIR} is src/installer/corehost.

Can you please move the awk script to eng/native and modify the path to the script in functions.cmake by removing the ${CMAKE_SOURCE_DIR}/ ?

vitek-karas · 2020-04-24T18:48:56Z

@jkotalik @davidfowl Just to clarify - currently IIS hosting doesn't support single-file apps (even those in 3.1). Single-file in 5.0 is different from 3.1, but in terms of hosting support it's actually very similar - we still don't support hosting it via native host (IIS being one such native host). If this is something we think we should have, than we need to figure out what it means.

swaroop-sridhar

Overall the change looks good.
Can you please make sure that existing apphost, liobhost, comhost, are not affected and build exactly the same (when compared to building without these changes)? Thanks.

swaroop-sridhar · 2020-04-25T00:01:08Z

src/installer/corehost/hostfxr_iface.h

+#include "pal.h"
+#include "error_codes.h"
+
+class hostfxr


The class name must match the file name. So, the class should be called hostfxr_iface_t.
However, in keeping with the hostpolicy_resolver naming, this class (and file) should probably be called hostfxr_resolver_t.

Forgot about this. Why the "_t" part?
Could the type and file be both just hostfxr_resolver ?

Every type in the host ends with _t. While I don't particularly like it, we should follow the convention.

src/installer/corehost/cli/hostpolicy/static/CMakeLists.txt

swaroop-sridhar · 2020-04-25T00:06:22Z

src/installer/corehost/cli/apphost/static/CMakeLists.txt

+# The .NET Foundation licenses this file to you under the MIT license.
+# See the LICENSE file in the project root for more information.
+
+project(static_apphost)


@richlander @samsp-msft
Wanted to ask again if we want a better name for the static host, instead of static_apphost.exe?

We build two types of hosts:

Apphost linked with other host components code-named "static apphost" (this PR) -- this will ship in the Microsoft.netcoreapp.host package for Windows (and Mac).

Apphost linked with host and runtime components code-named "super apphost" (TBD) -- this will ship in the Microsoft.netcoreapp.host package for Linux.

@VSadov please see @jkotas comments here: #32823 (comment).
This static_apphost should be renamed to netcoreapphost or singlefilehost

The file is a template to be linked with app parts and renamed, so naming is not extremely important IMO.
singleapphost feels a bit better as it hints at the purpose of the file.
I assume the standalone apphost will not be used (or will not be recommended) for single-file scenarios once this is shipped.

I will change to singleapphost.

The feature is called SingleFile (e.g. -p:PublishSingleFile=true) so the name should be singlefile* for consistency.

singlefilehost ?

Actually, I meant singlefilehost in the first place.

singleapphost was a mistype.

src/installer/corehost/cli/fxr/static/hostpolicy_resolver.cpp

swaroop-sridhar · 2020-04-25T00:18:06Z

src/installer/corehost/cli/fxr/static/CMakeLists.txt

@@ -0,0 +1,45 @@
+# Licensed to the .NET Foundation under one or more agreements.


Given the upcoming change: #1154 (comment)
We could consider not building hostfxr_shared.lib etc, but simply building static_apphost.exe directly.
Again: This is just for consideration ... may simplify the build logic, not sure it is a good idea.

swaroop-sridhar

The static_apphost should be published in Microsoft.NETCore.App.Host package, along with
apphost, comhost, ijwhost, nethost.

This is part of the work to create an apphost that bundles both hostfxr and hostpolicy. The main distinction between the static and shared versions of hostfxr is that the static version contains a hostpolicy resolver that references the hostpolicy symbols directly rather than loading them from a DLL.

This change is part of the work to enable an apphost that bundles both hostfxr and hostpolicy. There's no distinction between hostpolicy that's built as a shared library and as a static library: the shared library is built by linking an empty object file with the static library.

Provide a hostfxr_iface class, that abstracts how the hostfxr functions called by the early stage in the hosting layer is resolved.

This provides two implementations of hostfxr_iface: one for the static apphost, which bundles hostfxr and hostpolicy, and another for the conventional apphost, which loads them dynamically on startup.

swaroop-sridhar

Please add the test-case from Add a test case VSadov/runtime#2
The hostfxr_iface renaming issue still needs to be resolved.

swaroop-sridhar

The static_apphost needs to be packaged into microsoft.netcore.app.* but I think this change can be done in a separate PR.

swaroop-sridhar · 2020-05-04T23:33:23Z

src/installer/corehost/cli/apphost/CMakeLists.txt

-if (CLR_CMAKE_TARGET_WIN32 AND (CLR_CMAKE_TARGET_ARCH_ARM OR CLR_CMAKE_TARGET_ARCH_ARM64))
-    target_link_libraries(apphost Advapi32.lib shell32.lib)
-endif()
+add_subdirectory(static)


I'm not sure "static" and "shared" are the best names here.
I think Leandro chose these because of static and shared libraries.
But "shared" is a bit confusing because (a) it seems to represent code shared between different kinds of builds, and (b) apphost is not a shared library.
Please consider renaming them to more representative names.

I do not like 'shared', but did not have a lot of good alternatives. It is an opposite of "static" and "super" - could it be "standalone"?

Sure, I think "standalone" is a good option. "linked vs standalone" or "static vs standalone" are good I think.

I will make it static vs. standalone. (static seems good enough)

swaroop-sridhar

Change looks good to me, except for a few issues noted above.
Thanks.

VSadov · 2020-05-05T00:01:30Z

I think I have added the publishing steps in 433db23

Are there other packages it needs to be included in?

swaroop-sridhar · 2020-05-05T00:45:19Z

I think I have added the publishing steps in 433db23

Are there other packages it needs to be included in?

Thanks I overlooked that change.

swaroop-sridhar · 2020-05-05T00:53:06Z

@wli3 @dsplaisted FYI

This change introduces the "superhost" as discussed in #32823.
This host will be a part of the Microsoft.NETCore.app.Host.* packages, and should be consumed by the SDK when publishing for single-file self-contained scenario.
Currently it statically links the apphost with hostpolicy and hostfxr, but subsequently the runtime will be linked with the host (for Linux).

Add a test case

swaroop-sridhar · 2020-05-05T07:03:03Z

/azp runtime

azure-pipelines · 2020-05-05T07:03:08Z

Command 'runtime' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

swaroop-sridhar · 2020-05-05T07:04:23Z

/azp run runtime

azure-pipelines · 2020-05-05T07:04:49Z

Azure Pipelines successfully started running 1 pipeline(s).

…loneApp)

ghost · 2020-05-05T21:48:30Z

Hello @VSadov!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

safern · 2020-05-06T05:19:59Z

@swaroop-sridhar @VSadov maybe this is the reason for the test failures on rolling builds on CI for the installer linux_musl_x64 leg?

https://dev.azure.com/dnceng/public/_build/results?buildId=632734&view=results

VSadov · 2020-05-06T05:37:17Z

what could be special abut linux_musl_x64 ?

safern · 2020-05-06T05:49:44Z

I don't know but also note that the PR leg is Debug and rolling build is Release... There was another break similar to this in the installer tests also only in linux_musl_x64 which is alpine.

#35755

Maybe we should add a Release leg to PRs as well as it seems like these scenarios behave differently in Debug and Release.

…t#35368)" This reverts commit f6e5219.

…" (#36076) This reverts commit f6e5219.

Dotnet-GitSync-Bot added the area-Host label Apr 23, 2020

VSadov mentioned this pull request Apr 23, 2020

Build an apphost with hostfxr and hostpolicy linked in #35087

Closed

VSadov force-pushed the staticHost branch 2 times, most recently from c8ec727 to e4882e4 Compare April 24, 2020 04:50

swaroop-sridhar reviewed Apr 24, 2020

View reviewed changes

janvorli reviewed Apr 24, 2020

View reviewed changes

src/installer/corehost/cli/fxr/shared/CMakeLists.txt Outdated Show resolved Hide resolved

VSadov force-pushed the staticHost branch 2 times, most recently from 3676d5c to e88b262 Compare April 24, 2020 21:12

swaroop-sridhar suggested changes Apr 25, 2020

View reviewed changes

VSadov force-pushed the staticHost branch from e88b262 to e43e73b Compare April 25, 2020 17:53

This was referenced Apr 27, 2020

Test timeline issue jaredpar/runfo#8

Closed

OSX machines are de-provisioned during CI / PR runs leading to failures #34472

Closed

VSadov force-pushed the staticHost branch from 56286c9 to a9ba245 Compare April 27, 2020 22:18

VSadov marked this pull request as draft April 28, 2020 23:04

ericstj mentioned this pull request Apr 29, 2020

Test failed: System.Text.RegularExpressions.Tests.RegexCacheTests.Ctor_Cache_Promote_entries fails with Timeout #13610

Closed

swaroop-sridhar mentioned this pull request Apr 30, 2020

[release/5.0.1xx-preview4] Single-File: Fix GenerateBundle Task dotnet/sdk#11502

Closed

lpereira added 5 commits May 1, 2020 15:10

corehost: Allow linking of hostfxr and hostpolicy with apphost

d942cf2

Provide a hostfxr_iface class, that abstracts how the hostfxr functions called by the early stage in the hosting layer is resolved.

dotnet: Teach the muxer binary about hostfxr_iface

ea2b6d7

apphost: Teach apphost about hostfxr_iface

0f8daea

This provides two implementations of hostfxr_iface: one for the static apphost, which bundles hostfxr and hostpolicy, and another for the conventional apphost, which loads them dynamically on startup.

swaroop-sridhar reviewed May 4, 2020

View reviewed changes

swaroop-sridhar approved these changes May 4, 2020

View reviewed changes

VSadov added 3 commits May 4, 2020 19:15

Merge pull request #2 from swaroop-sridhar/staticHostTest

8f3de42

Add a test case

renamed hostfxr_iface --> hostfxr_resolver_t

818f5c6

renamed shared --> standalone

d95d0ca

VSadov added 3 commits May 5, 2020 12:22

rename static_apphost --> singlefilehost

67cf394

Signing exclusions for singlefilehost

eb30f92

switched StaticHost tst to a different asset (mostly a copy of Standa…

8669c80

…loneApp)

This was referenced May 5, 2020

Single-File: Implement statically linked apphost #32823

Closed

Use SingleFileHost for self-contained single-file apps dotnet/sdk#11567

Closed

VSadov added the auto-merge label May 5, 2020

ghost merged commit f6e5219 into dotnet:master May 6, 2020

safern added a commit to safern/runtime that referenced this pull request May 8, 2020

Revert "Build an apphost with hostfxr and hostpolicy linked in (dotne…

6cc01d4

…t#35368)" This reverts commit f6e5219.

safern added a commit that referenced this pull request May 8, 2020

Revert "Build an apphost with hostfxr and hostpolicy linked in (#35368)…

3011955

…" (#36076) This reverts commit f6e5219.

VSadov mentioned this pull request May 15, 2020

Build an apphost with hostfxr and hostpolicy linked in #36230

Merged

VSadov mentioned this pull request Jun 24, 2020

Host with coreclr linked in #36847

Merged

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

This pull request was closed.

		@@ -0,0 +1,45 @@
		# Licensed to the .NET Foundation under one or more agreements.

Build an apphost with hostfxr and hostpolicy linked in #35368

Build an apphost with hostfxr and hostpolicy linked in #35368

Conversation

VSadov commented Apr 23, 2020 • edited Loading

ghost commented Apr 23, 2020

swaroop-sridhar left a comment

Choose a reason for hiding this comment

VSadov commented Apr 24, 2020 • edited Loading

davidfowl commented Apr 24, 2020

VSadov commented Apr 24, 2020

jkotalik commented Apr 24, 2020

janvorli commented Apr 24, 2020

janvorli commented Apr 24, 2020

vitek-karas commented Apr 24, 2020

swaroop-sridhar left a 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

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

swaroop-sridhar left a comment

Choose a reason for hiding this comment

swaroop-sridhar left a comment

Choose a reason for hiding this comment

swaroop-sridhar left a 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

swaroop-sridhar left a comment

Choose a reason for hiding this comment

VSadov commented May 5, 2020 • edited Loading

swaroop-sridhar commented May 5, 2020

swaroop-sridhar commented May 5, 2020

swaroop-sridhar commented May 5, 2020

azure-pipelines bot commented May 5, 2020

swaroop-sridhar commented May 5, 2020

azure-pipelines bot commented May 5, 2020

ghost commented May 5, 2020

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

safern commented May 6, 2020

VSadov commented May 6, 2020

safern commented May 6, 2020

VSadov commented Apr 23, 2020 •

edited

Loading

VSadov commented Apr 24, 2020 •

edited

Loading

VSadov commented May 5, 2020 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.