From 47450a6d6af03443672601389afc20ee3cfad178 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Tue, 28 Feb 2023 18:57:36 -0800 Subject: [PATCH 1/2] Stop storing the parsed RID fallback graph --- .../corehost/hostpolicy/deps_format.cpp | 80 ++++++++++++------- src/native/corehost/hostpolicy/deps_format.h | 5 ++ .../corehost/hostpolicy/deps_resolver.h | 18 +++-- src/native/corehost/hostpolicy/hostpolicy.cpp | 49 +++++++++--- .../hostpolicy/hostpolicy_context.cpp | 6 +- .../corehost/hostpolicy/hostpolicy_context.h | 2 +- .../corehost/hostpolicy/hostpolicy_init.h | 2 - 7 files changed, 106 insertions(+), 56 deletions(-) diff --git a/src/native/corehost/hostpolicy/deps_format.cpp b/src/native/corehost/hostpolicy/deps_format.cpp index f9ffd4f16ed368..8aed3455365aaa 100644 --- a/src/native/corehost/hostpolicy/deps_format.cpp +++ b/src/native/corehost/hostpolicy/deps_format.cpp @@ -39,6 +39,55 @@ namespace return path; } + + void populate_rid_fallback_graph(const json_parser_t::value_t& json, deps_json_t::rid_fallback_graph_t& rid_fallback_graph) + { + const auto& json_object = json.GetObject(); + if (json_object.HasMember(_X("runtimes"))) + { + for (const auto& rid : json[_X("runtimes")].GetObject()) + { + auto& vec = rid_fallback_graph[rid.name.GetString()]; + const auto& fallback_array = rid.value.GetArray(); + vec.reserve(fallback_array.Size()); + for (const auto& fallback : fallback_array) + { + vec.push_back(fallback.GetString()); + } + } + } + + if (trace::is_enabled()) + { + trace::verbose(_X("The rid fallback graph is: {")); + for (const auto& rid : rid_fallback_graph) + { + trace::verbose(_X("%s => ["), rid.first.c_str()); + for (const auto& fallback : rid.second) + { + trace::verbose(_X("%s, "), fallback.c_str()); + } + trace::verbose(_X("]")); + } + trace::verbose(_X("}")); + } + } +} + +deps_json_t::rid_fallback_graph_t deps_json_t::get_rid_fallback_graph(pal::string_t& deps_path) +{ + rid_fallback_graph_t rid_fallback_graph; + + trace::verbose(_X("Getting RID fallback graph for deps file... %s"), deps_path.c_str()); + if (!bundle::info_t::config_t::probe(deps_path) && !pal::realpath(&deps_path, true)) + return rid_fallback_graph; + + json_parser_t json; + if (!json.parse_file(deps_path)) + return rid_fallback_graph; + + populate_rid_fallback_graph(json.document(), rid_fallback_graph); + return rid_fallback_graph; } void deps_json_t::reconcile_libraries_with_targets( @@ -367,36 +416,7 @@ void deps_json_t::load_self_contained(const pal::string_t& deps_path, const json }; reconcile_libraries_with_targets(deps_path, json, package_exists, get_relpaths); - - const auto& json_object = json.GetObject(); - if (json_object.HasMember(_X("runtimes"))) - { - for (const auto& rid : json[_X("runtimes")].GetObject()) - { - auto& vec = m_rid_fallback_graph[rid.name.GetString()]; - const auto& fallback_array = rid.value.GetArray(); - vec.reserve(fallback_array.Size()); - for (const auto& fallback : fallback_array) - { - vec.push_back(fallback.GetString()); - } - } - } - - if (trace::is_enabled()) - { - trace::verbose(_X("The rid fallback graph is: {")); - for (const auto& rid : m_rid_fallback_graph) - { - trace::verbose(_X("%s => ["), rid.first.c_str()); - for (const auto& fallback : rid.second) - { - trace::verbose(_X("%s, "), fallback.c_str()); - } - trace::verbose(_X("]")); - } - trace::verbose(_X("}")); - } + populate_rid_fallback_graph(json, m_rid_fallback_graph); } bool deps_json_t::has_package(const pal::string_t& name, const pal::string_t& ver) const diff --git a/src/native/corehost/hostpolicy/deps_format.h b/src/native/corehost/hostpolicy/deps_format.h index 52348d394f7952..f529bf353acc75 100644 --- a/src/native/corehost/hostpolicy/deps_format.h +++ b/src/native/corehost/hostpolicy/deps_format.h @@ -61,6 +61,11 @@ class deps_json_t return m_deps_file; } +public: // static + // Get the RID fallback graph for a deps file. + // Parse failures or non-existent files will return an empty fallback graph + static rid_fallback_graph_t get_rid_fallback_graph(pal::string_t& deps_path); + private: void load_self_contained(const pal::string_t& deps_path, const json_parser_t::value_t& json, const pal::string_t& target_name); void load_framework_dependent(const pal::string_t& deps_path, const json_parser_t::value_t& json, const pal::string_t& target_name, const rid_fallback_graph_t& rid_fallback_graph); diff --git a/src/native/corehost/hostpolicy/deps_resolver.h b/src/native/corehost/hostpolicy/deps_resolver.h index 53f6c3843ae696..1d32e10bcae298 100644 --- a/src/native/corehost/hostpolicy/deps_resolver.h +++ b/src/native/corehost/hostpolicy/deps_resolver.h @@ -173,6 +173,15 @@ class deps_resolver_t } } +public: // static + static pal::string_t get_fx_deps(const pal::string_t& fx_dir, const pal::string_t& fx_name) + { + pal::string_t fx_deps = fx_dir; + pal::string_t fx_deps_name = fx_name + _X(".deps.json"); + append_path(&fx_deps, fx_deps_name.c_str()); + return fx_deps; + } + private: void setup_shared_store_probes( const std::vector& shared_stores); @@ -194,14 +203,7 @@ class deps_resolver_t return *m_fx_deps[0]; } - static pal::string_t get_fx_deps(const pal::string_t& fx_dir, const pal::string_t& fx_name) - { - pal::string_t fx_deps = fx_dir; - pal::string_t fx_deps_name = fx_name + _X(".deps.json"); - append_path(&fx_deps, fx_deps_name.c_str()); - return fx_deps; - } - +private: // Resolve order for TPA lookup. bool resolve_tpa_list( pal::string_t* output, diff --git a/src/native/corehost/hostpolicy/hostpolicy.cpp b/src/native/corehost/hostpolicy/hostpolicy.cpp index 100128f4ad64ad..8918a7bfc5abba 100644 --- a/src/native/corehost/hostpolicy/hostpolicy.cpp +++ b/src/native/corehost/hostpolicy/hostpolicy.cpp @@ -97,7 +97,7 @@ namespace } int create_hostpolicy_context( - hostpolicy_init_t &hostpolicy_init, + const hostpolicy_init_t &hostpolicy_init, const int argc, const pal::char_t *argv[], bool breadcrumbs_enabled, @@ -825,6 +825,33 @@ SHARED_API int HOSTPOLICY_CALLTYPE corehost_unload() return StatusCode::Success; } +namespace +{ + pal::string_t get_root_deps_file(const hostpolicy_init_t& init) + { + if (init.is_framework_dependent) + { + const fx_definition_t& root_fx = get_root_framework(init.fx_definitions); + return deps_resolver_t::get_fx_deps(root_fx.get_dir(), root_fx.get_name()); + } + + // For self-contained, the root deps file is the app's deps file + if (!init.deps_file.empty()) + return init.deps_file; + + const std::shared_ptr context = get_hostpolicy_context(/*require_runtime*/ true); + if (bundle::info_t::is_single_file_bundle()) + { + const bundle::runner_t* app = bundle::runner_t::app(); + const pal::string_t& app_root = app->base_path(); + + return get_deps_from_app_binary(app->base_path(), context->application); + } + + return get_deps_from_app_binary(get_directory(context->application), context->application); + } +} + SHARED_API int HOSTPOLICY_CALLTYPE corehost_resolve_component_dependencies( const pal::char_t *component_main_assembly_path, corehost_resolve_component_dependencies_result_fn result) @@ -844,20 +871,19 @@ SHARED_API int HOSTPOLICY_CALLTYPE corehost_resolve_component_dependencies( // IMPORTANT: g_init is static/global and thus potentially accessed from multiple threads // We must only use it as read-only here (unlike the run scenarios which own it). - // For example the frameworks in g_init.fx_definitions can't be used "as-is" by the resolver - // right now as it would try to re-parse the .deps.json and thus modify the objects. + const hostpolicy_init_t &init = g_init; // The assumption is that component dependency resolution will only be called // when the coreclr is hosted through this hostpolicy and thus it will // have already called corehost_main_init. - if (!g_init.host_info.is_valid(g_init.host_mode)) + if (!init.host_info.is_valid(init.host_mode)) { trace::error(_X("Hostpolicy must be initialized and corehost_main must have been called before calling corehost_resolve_component_dependencies.")); return StatusCode::CoreHostLibLoadFailure; } // If the current host mode is libhost, use apphost instead. - host_mode_t host_mode = g_init.host_mode == host_mode_t::libhost ? host_mode_t::apphost : g_init.host_mode; + host_mode_t host_mode = init.host_mode == host_mode_t::libhost ? host_mode_t::apphost : init.host_mode; // Initialize arguments (basically the structure describing the input app/component to resolve) arguments_t args; @@ -900,15 +926,18 @@ SHARED_API int HOSTPOLICY_CALLTYPE corehost_resolve_component_dependencies( // TODO Review: Since we're only passing the one component framework, the resolver will not consider // frameworks from the app for probing paths. So potential references to paths inside frameworks will not resolve. - // The RID graph still has to come from the actual root framework, so take that from the g_init.root_rid_fallback_graph, - // which stores the fallback graph for the app's root framework. + // The RID graph still has to come from the actual root framework, so get the deps file + // for the app's root framework and read in the graph. + static deps_json_t::rid_fallback_graph_t root_rid_fallback_graph = + deps_json_t::get_rid_fallback_graph(get_root_deps_file(init)); + deps_resolver_t resolver( args, component_fx_definitions, /* additional_deps_serialized */ nullptr, // Additional deps - don't use those from the app, they're already in the app - shared_store::get_paths(g_init.tfm, host_mode, g_init.host_info.host_path), - g_init.probe_paths, - &g_init.root_rid_fallback_graph, + shared_store::get_paths(init.tfm, host_mode, init.host_info.host_path), + init.probe_paths, + &root_rid_fallback_graph, true); pal::string_t resolver_errors; diff --git a/src/native/corehost/hostpolicy/hostpolicy_context.cpp b/src/native/corehost/hostpolicy/hostpolicy_context.cpp index 11fc7883a3a7e7..cc0bc2116c69d3 100644 --- a/src/native/corehost/hostpolicy/hostpolicy_context.cpp +++ b/src/native/corehost/hostpolicy/hostpolicy_context.cpp @@ -135,7 +135,7 @@ namespace } } -int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const arguments_t &args, bool enable_breadcrumbs) +int hostpolicy_context_t::initialize(const hostpolicy_init_t &hostpolicy_init, const arguments_t &args, bool enable_breadcrumbs) { application = args.managed_application; host_mode = hostpolicy_init.host_mode; @@ -160,10 +160,6 @@ int hostpolicy_context_t::initialize(hostpolicy_init_t &hostpolicy_init, const a return StatusCode::ResolverInitFailure; } - // Store the root framework's rid fallback graph so that we can - // use it for future dependency resolutions - hostpolicy_init.root_rid_fallback_graph = resolver.get_root_deps().get_rid_fallback_graph(); - probe_paths_t probe_paths; // Setup breadcrumbs. diff --git a/src/native/corehost/hostpolicy/hostpolicy_context.h b/src/native/corehost/hostpolicy/hostpolicy_context.h index bec4630762fe03..608a6374195745 100644 --- a/src/native/corehost/hostpolicy/hostpolicy_context.h +++ b/src/native/corehost/hostpolicy/hostpolicy_context.h @@ -30,7 +30,7 @@ struct hostpolicy_context_t host_runtime_contract host_contract; - int initialize(hostpolicy_init_t &hostpolicy_init, const arguments_t &args, bool enable_breadcrumbs); + int initialize(const hostpolicy_init_t &hostpolicy_init, const arguments_t &args, bool enable_breadcrumbs); }; #endif // __HOSTPOLICY_CONTEXT_H__ diff --git a/src/native/corehost/hostpolicy/hostpolicy_init.h b/src/native/corehost/hostpolicy/hostpolicy_init.h index 590f0101d16920..5b0aeafc4e7750 100644 --- a/src/native/corehost/hostpolicy/hostpolicy_init.h +++ b/src/native/corehost/hostpolicy/hostpolicy_init.h @@ -25,8 +25,6 @@ struct hostpolicy_init_t pal::string_t host_command; host_startup_info_t host_info; - std::unordered_map> root_rid_fallback_graph; - static bool init(const host_interface_t* input, hostpolicy_init_t* init); static void init_host_command(const host_interface_t* input, hostpolicy_init_t* init); From 9db83dde614e6aa96d52254a0b7c897de00599bb Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Wed, 1 Mar 2023 14:33:32 -0800 Subject: [PATCH 2/2] PR feedback --- .../StandaloneAppActivation.cs | 2 +- .../corehost/hostpolicy/deps_format.cpp | 25 +++++++++++++------ src/native/corehost/hostpolicy/deps_format.h | 2 +- 3 files changed, 19 insertions(+), 10 deletions(-) diff --git a/src/installer/tests/HostActivation.Tests/StandaloneAppActivation.cs b/src/installer/tests/HostActivation.Tests/StandaloneAppActivation.cs index d76559cb06e456..3559f473589955 100644 --- a/src/installer/tests/HostActivation.Tests/StandaloneAppActivation.cs +++ b/src/installer/tests/HostActivation.Tests/StandaloneAppActivation.cs @@ -85,7 +85,7 @@ public void Running_Publish_Output_Standalone_EXE_with_no_DepsJson_and_no_Runtim .Should().Pass() .And.HaveStdOut($"Hello World!{Environment.NewLine}{Environment.NewLine}.NET {sharedTestState.RepoDirectories.MicrosoftNETCoreAppVersion}{Environment.NewLine}") .And.HaveStdErrContaining($"Runtime config does not exist at [{fixture.TestProject.RuntimeConfigJson}]") - .And.HaveStdErrContaining($"Could not locate the dependencies manifest file [{fixture.TestProject.DepsJson}]"); + .And.HaveStdErrContaining($"Dependencies manifest does not exist at [{fixture.TestProject.DepsJson}]"); } [Fact] diff --git a/src/native/corehost/hostpolicy/deps_format.cpp b/src/native/corehost/hostpolicy/deps_format.cpp index 8aed3455365aaa..739cd04bfe1e7c 100644 --- a/src/native/corehost/hostpolicy/deps_format.cpp +++ b/src/native/corehost/hostpolicy/deps_format.cpp @@ -72,18 +72,28 @@ namespace trace::verbose(_X("}")); } } + + bool deps_file_exists(pal::string_t& deps_path) + { + if (bundle::info_t::config_t::probe(deps_path) || pal::realpath(&deps_path, /*skip_error_logging*/ true)) + return true; + + trace::verbose(_X("Dependencies manifest does not exist at [%s]"), deps_path.c_str()); + return false; + } } -deps_json_t::rid_fallback_graph_t deps_json_t::get_rid_fallback_graph(pal::string_t& deps_path) +deps_json_t::rid_fallback_graph_t deps_json_t::get_rid_fallback_graph(const pal::string_t& deps_path) { rid_fallback_graph_t rid_fallback_graph; - trace::verbose(_X("Getting RID fallback graph for deps file... %s"), deps_path.c_str()); - if (!bundle::info_t::config_t::probe(deps_path) && !pal::realpath(&deps_path, true)) + + pal::string_t deps_path_local = deps_path; + if (!deps_file_exists(deps_path_local)) return rid_fallback_graph; json_parser_t json; - if (!json.parse_file(deps_path)) + if (!json.parse_file(deps_path_local)) return rid_fallback_graph; populate_rid_fallback_graph(json.document(), rid_fallback_graph); @@ -449,17 +459,16 @@ bool deps_json_t::has_package(const pal::string_t& name, const pal::string_t& ve void deps_json_t::load(bool is_framework_dependent, const pal::string_t& deps_path, const rid_fallback_graph_t& rid_fallback_graph) { m_deps_file = deps_path; - m_file_exists = bundle::info_t::config_t::probe(deps_path) || pal::realpath(&m_deps_file, true); + m_file_exists = deps_file_exists(m_deps_file); - json_parser_t json; if (!m_file_exists) { - // If file doesn't exist, then assume parsed. - trace::verbose(_X("Could not locate the dependencies manifest file [%s]. Some libraries may fail to resolve."), deps_path.c_str()); + // Not existing is valid m_valid = true; return; } + json_parser_t json; if (!json.parse_file(m_deps_file)) return; diff --git a/src/native/corehost/hostpolicy/deps_format.h b/src/native/corehost/hostpolicy/deps_format.h index f529bf353acc75..b1521299ed2c52 100644 --- a/src/native/corehost/hostpolicy/deps_format.h +++ b/src/native/corehost/hostpolicy/deps_format.h @@ -64,7 +64,7 @@ class deps_json_t public: // static // Get the RID fallback graph for a deps file. // Parse failures or non-existent files will return an empty fallback graph - static rid_fallback_graph_t get_rid_fallback_graph(pal::string_t& deps_path); + static rid_fallback_graph_t get_rid_fallback_graph(const pal::string_t& deps_path); private: void load_self_contained(const pal::string_t& deps_path, const json_parser_t::value_t& json, const pal::string_t& target_name);