From 4715d55962d6ab12ece844b52cc3d94fd6c07ed5 Mon Sep 17 00:00:00 2001 From: Elinor Fung Date: Sun, 6 Aug 2023 22:06:59 -0700 Subject: [PATCH] Check for .deps.json when enumerating framework paths This adds a check for the framework's .deps.json instead of just the existence of the directory. To avoid extra file checks in the normal/happy path (where all framework version folder are valid), when resolving, it only does the check after resolving the best version match. And if that version directory isn't valid, it tries resolving again without it. Backport of #90035 --- .../FrameworkResolution/MultipleHives.cs | 16 +++- .../MultilevelSharedFxLookup.cs | 9 ++- .../HostActivation.Tests/NativeHostApis.cs | 20 +++-- .../tests/TestUtils/DotNetBuilder.cs | 40 ++++++--- src/native/corehost/fxr/framework_info.cpp | 81 ++++++++++--------- src/native/corehost/fxr/framework_info.h | 2 +- src/native/corehost/fxr/fx_resolver.cpp | 52 +++++++----- .../corehost/fxr/fx_resolver.messages.cpp | 4 +- src/native/corehost/fxr/hostfxr.cpp | 2 +- 9 files changed, 144 insertions(+), 82 deletions(-) diff --git a/src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs b/src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs index 0a12cd2640ce9b..5a91c88e3a8333 100644 --- a/src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs +++ b/src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs @@ -4,6 +4,7 @@ using Microsoft.DotNet.Cli.Build; using Microsoft.DotNet.Cli.Build.Framework; using System; +using System.IO; using System.Runtime.InteropServices; using Xunit; @@ -27,7 +28,8 @@ public void FrameworkHiveSelection_GlobalHiveWithBetterMatch() RunTest( runtimeConfig => runtimeConfig .WithFramework(MicrosoftNETCoreApp, "5.0.0")) - .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "5.1.2"); + .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "5.1.2") + .And.HaveStdErrContaining($"Ignoring FX version [5.0.0] without .deps.json"); } [Fact] @@ -37,7 +39,8 @@ public void FrameworkHiveSelection_MainHiveWithBetterMatch() RunTest( runtimeConfig => runtimeConfig .WithFramework(MicrosoftNETCoreApp, "6.0.0")) - .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "6.1.2"); + .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "6.1.2") + .And.HaveStdErrContaining($"Ignoring FX version [6.0.0] without .deps.json"); } [Fact] @@ -51,7 +54,8 @@ public void FrameworkHiveSelection_CurrentDirectoryIsIgnored() .WithRuntimeConfigCustomizer(runtimeConfig => runtimeConfig .WithFramework(MicrosoftNETCoreApp, "5.0.0")) .WithWorkingDirectory(SharedState.DotNetCurrentHive.BinPath)) - .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "5.2.0"); + .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, "5.2.0") + .And.HaveStdErrContaining($"Ignoring FX version [5.0.0] without .deps.json"); } private CommandResult RunTest(Func runtimeConfig) @@ -89,6 +93,12 @@ public SharedTestState() .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("6.1.2") .Build(); + // Empty Microsoft.NETCore.App directory - should not be recognized as a valid framework + // Version is the best match for some test cases, but they should be ignored + string netCoreAppDir = Path.Combine(DotNetMainHive.BinPath, "shared", Constants.MicrosoftNETCoreApp); + Directory.CreateDirectory(Path.Combine(netCoreAppDir, "5.0.0")); + Directory.CreateDirectory(Path.Combine(netCoreAppDir, "6.0.0")); + DotNetGlobalHive = DotNet("GlobalHive") .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("5.1.2") .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("6.2.0") diff --git a/src/installer/tests/HostActivation.Tests/MultilevelSharedFxLookup.cs b/src/installer/tests/HostActivation.Tests/MultilevelSharedFxLookup.cs index 788963fa33e49b..889ba0c7da383f 100644 --- a/src/installer/tests/HostActivation.Tests/MultilevelSharedFxLookup.cs +++ b/src/installer/tests/HostActivation.Tests/MultilevelSharedFxLookup.cs @@ -106,6 +106,9 @@ public MultilevelSharedFxLookup() _builtSharedUberFxDir = Path.Combine(_builtDotnet, "shared", "Microsoft.UberFramework", _sharedFxVersion); SharedFramework.CreateUberFrameworkArtifacts(_builtSharedFxDir, _builtSharedUberFxDir, SystemCollectionsImmutableAssemblyVersion, SystemCollectionsImmutableFileVersion); + // Empty Microsoft.NETCore.App directory - should not be recognized as a valid framework + Directory.CreateDirectory(Path.Combine(_exeSharedFxBaseDir, "9999.9.9")); + // Trace messages used to identify from which folder the framework was picked _hostPolicyDllName = Path.GetFileName(fixture.TestProject.HostPolicyDll); _exeSelectedMessage = $"The expected {_hostPolicyDllName} directory is [{_exeSharedFxBaseDir}"; @@ -237,7 +240,8 @@ public void SharedMultilevelFxLookup_Must_Verify_Folders_in_the_Correct_Order() .CaptureStdErr() .Execute() .Should().Pass() - .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.0"); + .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.0") + .And.HaveStdErrContaining("Ignoring FX version [9999.9.9] without .deps.json"); } [Fact] @@ -341,7 +345,8 @@ public void SharedMultilevelFxLookup_Must_Not_Roll_Forward_If_Framework_Version_ .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.0-dummy2") .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.2") .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.3") - .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.0-dummy3"); + .And.HaveStdOutContaining("Microsoft.NETCore.App 9999.0.0-dummy3") + .And.HaveStdErrContaining("Ignoring FX version [9999.9.9] without .deps.json"); } } } diff --git a/src/installer/tests/HostActivation.Tests/NativeHostApis.cs b/src/installer/tests/HostActivation.Tests/NativeHostApis.cs index d9feb540d70bce..659f8374619d9d 100644 --- a/src/installer/tests/HostActivation.Tests/NativeHostApis.cs +++ b/src/installer/tests/HostActivation.Tests/NativeHostApis.cs @@ -97,7 +97,7 @@ public SdkResolutionFixture(SharedTestState state) Directory.CreateDirectory(WorkingDir); - // start with an empty global.json, it will be ignored, but prevent one lying on disk + // start with an empty global.json, it will be ignored, but prevent one lying on disk // on a given machine from impacting the test. File.WriteAllText(GlobalJson, "{}"); @@ -117,12 +117,22 @@ public SdkResolutionFixture(SharedTestState state) foreach ((string fwName, string[] fwVersions) in ProgramFilesGlobalFrameworks) { foreach (string fwVersion in fwVersions) - Directory.CreateDirectory(Path.Combine(ProgramFilesGlobalFrameworksDir, fwName, fwVersion)); + AddFrameworkDirectory(ProgramFilesGlobalFrameworksDir, fwName, fwVersion); } foreach ((string fwName, string[] fwVersions) in LocalFrameworks) { foreach (string fwVersion in fwVersions) - Directory.CreateDirectory(Path.Combine(LocalFrameworksDir, fwName, fwVersion)); + AddFrameworkDirectory(LocalFrameworksDir, fwName, fwVersion); + + // Empty framework directory - this should not be recognized as a valid framework directory + Directory.CreateDirectory(Path.Combine(LocalFrameworksDir, fwName, "9.9.9")); + } + + static void AddFrameworkDirectory(string frameworkDir, string name, string version) + { + string versionDir = Path.Combine(frameworkDir, name, version); + Directory.CreateDirectory(versionDir); + File.WriteAllText(Path.Combine(versionDir, $"{name}.deps.json"), string.Empty); } } } @@ -230,7 +240,7 @@ public void Hostfxr_resolve_sdk2_without_global_json_and_disallowing_previews() [Fact] public void Hostfxr_resolve_sdk2_with_global_json_and_disallowing_previews() { - // With global.json specifying a preview, roll forward to preview + // With global.json specifying a preview, roll forward to preview // since flag has no impact if global.json specifies a preview. // Also check that global.json that impacted resolution is reported. @@ -511,7 +521,7 @@ public void Hostfxr_get_dotnet_environment_info_with_multilevel_lookup_only_self public void Hostfxr_get_dotnet_environment_info_global_install_path() { var f = new SdkResolutionFixture(sharedTestState); - + f.Dotnet.Exec(f.AppDll, new[] { "hostfxr_get_dotnet_environment_info" }) .CaptureStdOut() .CaptureStdErr() diff --git a/src/installer/tests/TestUtils/DotNetBuilder.cs b/src/installer/tests/TestUtils/DotNetBuilder.cs index d4cb58340ca6ed..d2d125fa581b83 100644 --- a/src/installer/tests/TestUtils/DotNetBuilder.cs +++ b/src/installer/tests/TestUtils/DotNetBuilder.cs @@ -51,8 +51,7 @@ public DotNetBuilder(string basePath, string builtDotnet, string name) public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockHostPolicy(string version) { // ./shared/Microsoft.NETCore.App/ - create a mock of the root framework - string netCoreAppPath = Path.Combine(_path, "shared", "Microsoft.NETCore.App", version); - Directory.CreateDirectory(netCoreAppPath); + string netCoreAppPath = AddFramework(Constants.MicrosoftNETCoreApp, version); // ./shared/Microsoft.NETCore.App//hostpolicy.dll - this is a mock, will not actually load CoreCLR string mockHostPolicyFileName = RuntimeInformationExtensions.GetSharedLibraryFileNameForCurrentPlatform("mockhostpolicy"); @@ -122,8 +121,7 @@ public DotNetBuilder RemoveHostFxr(Version version = null) public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockCoreClr(string version, Action customizer = null) { // ./shared/Microsoft.NETCore.App/ - create a mock of the root framework - string netCoreAppPath = Path.Combine(_path, "shared", "Microsoft.NETCore.App", version); - Directory.CreateDirectory(netCoreAppPath); + string netCoreAppPath = AddFramework(Constants.MicrosoftNETCoreApp, version); string hostPolicyFileName = RuntimeInformationExtensions.GetSharedLibraryFileNameForCurrentPlatform("hostpolicy"); string coreclrFileName = RuntimeInformationExtensions.GetSharedLibraryFileNameForCurrentPlatform("coreclr"); @@ -131,9 +129,9 @@ public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockCoreClr(string version, string currentRid = _repoDirectories.TargetRID; - NetCoreAppBuilder.ForNETCoreApp("Microsoft.NETCore.App", currentRid) + NetCoreAppBuilder.ForNETCoreApp(Constants.MicrosoftNETCoreApp, currentRid) .WithStandardRuntimeFallbacks() - .WithProject("Microsoft.NETCore.App", version, p => p + .WithProject(Constants.MicrosoftNETCoreApp, version, p => p .WithNativeLibraryGroup(null, g => g // ./shared/Microsoft.NETCore.App//coreclr.dll - this is a mock, will not actually run CoreClr .WithAsset((new NetCoreAppBuilder.RuntimeFileBuilder($"runtimes/{currentRid}/native/{coreclrFileName}")) @@ -158,19 +156,18 @@ public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockCoreClr(string version, /// Framework version /// Customization function for the runtime config /// - /// The added mock framework will only contain a runtime.config.json file. + /// The added mock framework will only contain a deps.json and a runtime.config.json file. /// public DotNetBuilder AddFramework( string name, string version, Action runtimeConfigCustomizer) { - // ./shared// - create a mock of effectively empty non-root framework - string path = Path.Combine(_path, "shared", name, version); - Directory.CreateDirectory(path); + // ./shared// - create a mock of the framework + string path = AddFramework(name, version); // ./shared///.runtimeconfig.json - runtime config which can be customized - RuntimeConfig runtimeConfig = new RuntimeConfig(Path.Combine(path, name + ".runtimeconfig.json")); + RuntimeConfig runtimeConfig = new RuntimeConfig(Path.Combine(path, $"{name}.runtimeconfig.json")); runtimeConfigCustomizer(runtimeConfig); runtimeConfig.Save(); @@ -193,6 +190,27 @@ public DotNetBuilder AddMockSDK( return this; } + /// + /// Add a minimal mock framework with the specified framework name and version + /// + /// Framework name + /// Framework version + /// Framework directory + /// + /// The added mock framework will only contain a deps.json. + /// + private string AddFramework(string name, string version) + { + // ./shared// - create a mock of effectively the framework + string path = Path.Combine(_path, "shared", name, version); + Directory.CreateDirectory(path); + + // ./shared///.deps.json - empty file + File.WriteAllText(Path.Combine(path, $"{name}.deps.json"), string.Empty); + + return path; + } + public DotNetCli Build() { return new DotNetCli(_path); diff --git a/src/native/corehost/fxr/framework_info.cpp b/src/native/corehost/fxr/framework_info.cpp index 0859f6e57c9d7e..fd2e8c73e79f38 100644 --- a/src/native/corehost/fxr/framework_info.cpp +++ b/src/native/corehost/fxr/framework_info.cpp @@ -34,7 +34,7 @@ bool compare_by_name_and_version(const framework_info &a, const framework_info & /*static*/ void framework_info::get_all_framework_infos( const pal::string_t& own_dir, - const pal::string_t& fx_name, + const pal::char_t* fx_name, std::vector* framework_infos) { std::vector hive_dir; @@ -42,49 +42,58 @@ bool compare_by_name_and_version(const framework_info &a, const framework_info & int32_t hive_depth = 0; - for (pal::string_t dir : hive_dir) + for (const pal::string_t& dir : hive_dir) { auto fx_shared_dir = dir; append_path(&fx_shared_dir, _X("shared")); - if (pal::directory_exists(fx_shared_dir)) + if (!pal::directory_exists(fx_shared_dir)) + continue; + + std::vector fx_names; + if (fx_name != nullptr) { - std::vector fx_names; - if (fx_name.length()) - { - // Use the provided framework name - fx_names.push_back(fx_name); - } - else - { - // Read all frameworks, including "Microsoft.NETCore.App" - pal::readdir_onlydirectories(fx_shared_dir, &fx_names); - } + // Use the provided framework name + fx_names.push_back(fx_name); + } + else + { + // Read all frameworks, including "Microsoft.NETCore.App" + pal::readdir_onlydirectories(fx_shared_dir, &fx_names); + } - for (pal::string_t fx_name : fx_names) - { - auto fx_dir = fx_shared_dir; - append_path(&fx_dir, fx_name.c_str()); + for (const pal::string_t& fx_name_local : fx_names) + { + auto fx_dir = fx_shared_dir; + append_path(&fx_dir, fx_name_local.c_str()); + + if (!pal::directory_exists(fx_dir)) + continue; + + trace::verbose(_X("Gathering FX locations in [%s]"), fx_dir.c_str()); - if (pal::directory_exists(fx_dir)) + std::vector versions; + pal::readdir_onlydirectories(fx_dir, &versions); + for (const pal::string_t& ver : versions) + { + // Make sure we filter out any non-version folders. + fx_ver_t parsed; + if (!fx_ver_t::parse(ver, &parsed, false)) + continue; + + // Check that the framework's .deps.json exists. + pal::string_t fx_version_dir = fx_dir; + append_path(&fx_version_dir, ver.c_str()); + if (!library_exists_in_dir(fx_version_dir, fx_name_local + _X(".deps.json"), nullptr)) { - trace::verbose(_X("Gathering FX locations in [%s]"), fx_dir.c_str()); - - std::vector versions; - pal::readdir_onlydirectories(fx_dir, &versions); - for (const auto& ver : versions) - { - // Make sure we filter out any non-version folders. - fx_ver_t parsed; - if (fx_ver_t::parse(ver, &parsed, false)) - { - trace::verbose(_X("Found FX version [%s]"), ver.c_str()); - - framework_info info(fx_name, fx_dir, parsed, hive_depth); - framework_infos->push_back(info); - } - } + trace::verbose(_X("Ignoring FX version [%s] without .deps.json"), ver.c_str()); + continue; } + + trace::verbose(_X("Found FX version [%s]"), ver.c_str()); + + framework_info info(fx_name_local, fx_dir, parsed, hive_depth); + framework_infos->push_back(info); } } @@ -97,7 +106,7 @@ bool compare_by_name_and_version(const framework_info &a, const framework_info & /*static*/ bool framework_info::print_all_frameworks(const pal::string_t& own_dir, const pal::string_t& leading_whitespace) { std::vector framework_infos; - get_all_framework_infos(own_dir, _X(""), &framework_infos); + get_all_framework_infos(own_dir, nullptr, &framework_infos); for (framework_info info : framework_infos) { trace::println(_X("%s%s %s [%s]"), leading_whitespace.c_str(), info.name.c_str(), info.version.as_str().c_str(), info.path.c_str()); diff --git a/src/native/corehost/fxr/framework_info.h b/src/native/corehost/fxr/framework_info.h index e4eea09cb14c22..d9111aef06b20b 100644 --- a/src/native/corehost/fxr/framework_info.h +++ b/src/native/corehost/fxr/framework_info.h @@ -17,7 +17,7 @@ struct framework_info static void get_all_framework_infos( const pal::string_t& own_dir, - const pal::string_t& fx_name, + const pal::char_t* fx_name, std::vector* framework_infos); static bool print_all_frameworks(const pal::string_t& own_dir, const pal::string_t& leading_whitespace); diff --git a/src/native/corehost/fxr/fx_resolver.cpp b/src/native/corehost/fxr/fx_resolver.cpp index 6091c9a5d4b552..216318b6047f09 100644 --- a/src/native/corehost/fxr/fx_resolver.cpp +++ b/src/native/corehost/fxr/fx_resolver.cpp @@ -174,9 +174,6 @@ namespace if (best_match == fx_ver_t()) { - // This is not strictly necessary, we just need to return version which doesn't exist. - // But it's cleaner to return the desider reference then invalid -1.-1.-1 version. - best_match = fx_ref.get_fx_version_number(); trace::verbose(_X("Framework reference didn't resolve to any available version.")); } else if (trace::is_enabled()) @@ -211,7 +208,8 @@ namespace pal::string_t selected_fx_version; fx_ver_t selected_ver; - for (pal::string_t dir : hive_dir) + pal::string_t deps_file_name = fx_ref.get_fx_name() + _X(".deps.json"); + for (pal::string_t& dir : hive_dir) { auto fx_dir = dir; trace::verbose(_X("Searching FX directory in [%s]"), fx_dir.c_str()); @@ -235,7 +233,7 @@ namespace fx_ref.get_fx_version().c_str()); append_path(&fx_dir, fx_ref.get_fx_version().c_str()); - if (pal::directory_exists(fx_dir)) + if (library_exists_in_dir(fx_dir, deps_file_name, nullptr)) { selected_fx_dir = fx_dir; selected_fx_version = fx_ref.get_fx_version(); @@ -258,27 +256,39 @@ namespace } fx_ver_t resolved_ver = resolve_framework_reference_from_version_list(version_list, fx_ref); - - pal::string_t resolved_ver_str = resolved_ver.as_str(); - append_path(&fx_dir, resolved_ver_str.c_str()); - - if (pal::directory_exists(fx_dir)) + while (resolved_ver != fx_ver_t()) { - if (selected_ver != fx_ver_t()) + pal::string_t resolved_ver_str = resolved_ver.as_str(); + pal::string_t resolved_fx_dir = fx_dir; + append_path(&resolved_fx_dir, resolved_ver_str.c_str()); + + // Check that the framework's .deps.json exists. To minimize the file checks done in the most common + // scenario (.deps.json exists), only check after resolving the version and if the .deps.json doesn't + // exist, attempt resolving again without that version. + if (!library_exists_in_dir(resolved_fx_dir, deps_file_name, nullptr)) { - // Compare the previous hive_dir selection with the current hive_dir to see which one is the better match - std::vector version_list; - version_list.push_back(resolved_ver); - version_list.push_back(selected_ver); + // Remove the version and try resolving again + trace::verbose(_X("Ignoring FX version [%s] without .deps.json"), resolved_ver_str.c_str()); + version_list.erase(std::find(version_list.cbegin(), version_list.cend(), resolved_ver)); resolved_ver = resolve_framework_reference_from_version_list(version_list, fx_ref); } - - if (resolved_ver != selected_ver) + else { - trace::verbose(_X("Changing Selected FX version from [%s] to [%s]"), selected_fx_dir.c_str(), fx_dir.c_str()); - selected_ver = resolved_ver; - selected_fx_dir = fx_dir; - selected_fx_version = resolved_ver_str; + if (selected_ver != fx_ver_t()) + { + // Compare the previous hive_dir selection with the current hive_dir to see which one is the better match + resolved_ver = resolve_framework_reference_from_version_list({ resolved_ver, selected_ver }, fx_ref); + } + + if (resolved_ver != selected_ver) + { + trace::verbose(_X("Changing Selected FX version from [%s] to [%s]"), selected_fx_dir.c_str(), resolved_fx_dir.c_str()); + selected_ver = resolved_ver; + selected_fx_dir = resolved_fx_dir; + selected_fx_version = resolved_ver_str; + } + + break; } } } diff --git a/src/native/corehost/fxr/fx_resolver.messages.cpp b/src/native/corehost/fxr/fx_resolver.messages.cpp index 960a1c42c5c37f..91539bc6903ee0 100644 --- a/src/native/corehost/fxr/fx_resolver.messages.cpp +++ b/src/native/corehost/fxr/fx_resolver.messages.cpp @@ -100,14 +100,14 @@ void fx_resolver_t::display_missing_framework_error( if (fx_dir.length()) { fx_ver_dirs = fx_dir; - framework_info::get_all_framework_infos(get_directory(fx_dir), fx_name, &framework_infos); + framework_info::get_all_framework_infos(get_directory(fx_dir), fx_name.c_str(), &framework_infos); } else { fx_ver_dirs = dotnet_root; } - framework_info::get_all_framework_infos(dotnet_root, fx_name, &framework_infos); + framework_info::get_all_framework_infos(dotnet_root, fx_name.c_str(), &framework_infos); // Display the error message about missing FX. if (fx_version.length()) diff --git a/src/native/corehost/fxr/hostfxr.cpp b/src/native/corehost/fxr/hostfxr.cpp index 4011dbc4397856..98fbb8e00bbd9c 100644 --- a/src/native/corehost/fxr/hostfxr.cpp +++ b/src/native/corehost/fxr/hostfxr.cpp @@ -425,7 +425,7 @@ SHARED_API int32_t HOSTFXR_CALLTYPE hostfxr_get_dotnet_environment_info( } std::vector framework_infos; - framework_info::get_all_framework_infos(dotnet_dir, _X(""), &framework_infos); + framework_info::get_all_framework_infos(dotnet_dir, nullptr, &framework_infos); std::vector environment_framework_infos; std::vector framework_versions;