Check for <framework_name>.deps.json when enumerating framework paths (#90035)
authorElinor Fung <elfung@microsoft.com>
Mon, 7 Aug 2023 05:06:59 +0000 (22:06 -0700)
committerGitHub <noreply@github.com>
Mon, 7 Aug 2023 05:06:59 +0000 (22:06 -0700)
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.

src/installer/tests/HostActivation.Tests/FrameworkResolution/MultipleHives.cs
src/installer/tests/HostActivation.Tests/NativeHostApis.cs
src/installer/tests/TestUtils/DotNetBuilder.cs
src/native/corehost/fxr/framework_info.cpp
src/native/corehost/fxr/framework_info.h
src/native/corehost/fxr/fx_resolver.cpp
src/native/corehost/fxr/fx_resolver.messages.cpp
src/native/corehost/fxr/hostfxr.cpp

index 16ad7c3..1fb612e 100644 (file)
@@ -52,7 +52,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.FrameworkResolution
                     .WithTfm(tfm)
                     .WithFramework(MicrosoftNETCoreApp, requestedVersion),
                 multiLevelLookup)
-                .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, resolvedVersion);
+                .ShouldHaveResolvedFramework(MicrosoftNETCoreApp, resolvedVersion)
+                .And.HaveStdErrContaining($"Ignoring FX version [{requestedVersion}] without .deps.json");
         }
 
         [Fact]
@@ -157,7 +158,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.FrameworkResolution
                 new TestSettings().WithCommandLine("--list-runtimes"),
                 multiLevelLookup,
                 testApp: null)
-                .Should().HaveStdOut(expectedOutput);
+                .Should().HaveStdOut(expectedOutput)
+                .And.HaveStdErrContaining("Ignoring FX version [9999.9.9] without .deps.json");
         }
 
         [Theory]
@@ -180,7 +182,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.FrameworkResolution
                 new TestSettings().WithCommandLine("--info"),
                 multiLevelLookup,
                 testApp: null)
-                .Should().HaveStdOutContaining(expectedOutput);
+                .Should().HaveStdOutContaining(expectedOutput)
+                .And.HaveStdErrContaining("Ignoring FX version [9999.9.9] without .deps.json");
         }
 
         [Theory]
@@ -210,7 +213,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.FrameworkResolution
                 multiLevelLookup)
                 .Should().Fail()
                 .And.HaveStdErrContaining(expectedOutput)
-                .And.HaveStdErrContaining("https://aka.ms/dotnet/app-launch-failed");
+                .And.HaveStdErrContaining("https://aka.ms/dotnet/app-launch-failed")
+                .And.HaveStdErrContaining("Ignoring FX version [9999.9.9] without .deps.json");
         }
 
         private CommandResult RunTest(Func<RuntimeConfig, RuntimeConfig> runtimeConfig, bool? multiLevelLookup = true)
@@ -254,6 +258,14 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.FrameworkResolution
                     .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("7.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"));
+                Directory.CreateDirectory(Path.Combine(netCoreAppDir, "7.0.0"));
+                Directory.CreateDirectory(Path.Combine(netCoreAppDir, "9999.9.9"));
+
                 DotNetGlobalHive = DotNet("GlobalHive")
                     .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("5.1.2")
                     .AddMicrosoftNETCoreAppFrameworkMockHostPolicy("6.1.4")
index 7ac464d..54872af 100644 (file)
@@ -115,12 +115,15 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation
                 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 AddSdkDirectory(string sdkDir, string version)
@@ -129,6 +132,13 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation
                     Directory.CreateDirectory(versionDir);
                     File.WriteAllText(Path.Combine(versionDir, "dotnet.dll"), string.Empty);
                 }
+
+                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);
+                }
             }
         }
 
index aa6a842..b25fc5e 100644 (file)
@@ -51,8 +51,7 @@ namespace Microsoft.DotNet.CoreSetup.Test
         public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockHostPolicy(string version)
         {
             // ./shared/Microsoft.NETCore.App/<version> - 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/<version>/hostpolicy.dll - this is a mock, will not actually load CoreCLR
             File.Copy(
@@ -120,14 +119,13 @@ namespace Microsoft.DotNet.CoreSetup.Test
         public DotNetBuilder AddMicrosoftNETCoreAppFrameworkMockCoreClr(string version, Action<NetCoreAppBuilder> customizer = null)
         {
             // ./shared/Microsoft.NETCore.App/<version> - 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 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/<version>/coreclr.dll - this is a mock, will not actually run CoreClr
                         .WithAsset((new NetCoreAppBuilder.RuntimeFileBuilder($"runtimes/{currentRid}/native/{Binaries.CoreClr.FileName}"))
@@ -152,7 +150,7 @@ namespace Microsoft.DotNet.CoreSetup.Test
         /// <param name="version">Framework version</param>
         /// <param name="runtimeConfigCustomizer">Customization function for the runtime config</param>
         /// <remarks>
-        /// 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.
         /// </remarks>
         public DotNetBuilder AddFramework(
             string name,
@@ -160,12 +158,11 @@ namespace Microsoft.DotNet.CoreSetup.Test
             Action<RuntimeConfig> runtimeConfigCustomizer,
             Action<string> frameworkCustomizer = null)
         {
-            // ./shared/<name>/<version> - create a mock of effectively empty non-root framework
-            string path = Path.Combine(_path, "shared", name, version);
-            Directory.CreateDirectory(path);
+            // ./shared/<name>/<version> - create a mock of the framework
+            string path = AddFramework(name, version);
 
             // ./shared/<name>/<version>/<name>.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();
 
@@ -191,6 +188,27 @@ namespace Microsoft.DotNet.CoreSetup.Test
             return this;
         }
 
+        /// <summary>
+        /// Add a minimal mock framework with the specified framework name and version
+        /// </summary>
+        /// <param name="name">Framework name</param>
+        /// <param name="version">Framework version</param>
+        /// <returns>Framework directory</returns>
+        /// <remarks>
+        /// The added mock framework will only contain a deps.json.
+        /// </remarks>
+        private string AddFramework(string name, string version)
+        {
+            // ./shared/<name>/<version> - create a mock of effectively the framework
+            string path = Path.Combine(_path, "shared", name, version);
+            Directory.CreateDirectory(path);
+
+            // ./shared/<name>/<version>/<name>.deps.json - empty file
+            File.WriteAllText(Path.Combine(path, $"{name}.deps.json"), string.Empty);
+
+            return path;
+        }
+
         public DotNetCli Build()
         {
             return new DotNetCli(_path);
index b8bb45e..0e2213c 100644 (file)
@@ -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,
     bool disable_multilevel_lookup,
     std::vector<framework_info>* framework_infos)
 {
@@ -43,49 +43,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<pal::string_t> fx_names;
+        if (fx_name != nullptr)
         {
-            std::vector<pal::string_t> 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_local : fx_names)
-            {
-                auto fx_dir = fx_shared_dir;
-                append_path(&fx_dir, fx_name_local.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<pal::string_t> 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<pal::string_t> 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_local, 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);
             }
         }
 
@@ -98,7 +107,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_info> framework_infos;
-    get_all_framework_infos(own_dir, _X(""), /*disable_multilevel_lookup*/ true, &framework_infos);
+    get_all_framework_infos(own_dir, nullptr, /*disable_multilevel_lookup*/ true, &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());
index 8e95bde..926f58c 100644 (file)
@@ -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,
         bool disable_multilevel_lookup,
         std::vector<framework_info>* framework_infos);
 
index faa40ed..7bb65d3 100644 (file)
@@ -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())
@@ -212,7 +209,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());
@@ -236,7 +234,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();
@@ -259,24 +257,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
-                        resolved_ver = resolve_framework_reference_from_version_list({ resolved_ver, selected_ver }, fx_ref);
+                        // 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;
                     }
                 }
             }
index bbbf067..e87cd6d 100644 (file)
@@ -101,14 +101,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, disable_multilevel_lookup, &framework_infos);
+        framework_info::get_all_framework_infos(get_directory(fx_dir), fx_name.c_str(), disable_multilevel_lookup, &framework_infos);
     }
     else
     {
         fx_ver_dirs = dotnet_root;
     }
 
-    framework_info::get_all_framework_infos(dotnet_root, fx_name, disable_multilevel_lookup, &framework_infos);
+    framework_info::get_all_framework_infos(dotnet_root, fx_name.c_str(), disable_multilevel_lookup, &framework_infos);
 
     // Display the error message about missing FX.
     if (fx_version.length())
index a6710dc..9e05206 100644 (file)
@@ -414,7 +414,7 @@ SHARED_API int32_t HOSTFXR_CALLTYPE hostfxr_get_dotnet_environment_info(
     }
 
     std::vector<framework_info> framework_infos;
-    framework_info::get_all_framework_infos(dotnet_dir, _X(""), /*disable_multilevel_lookup*/ true, &framework_infos);
+    framework_info::get_all_framework_infos(dotnet_dir, nullptr, /*disable_multilevel_lookup*/ true, &framework_infos);
 
     std::vector<hostfxr_dotnet_environment_framework_info> environment_framework_infos;
     std::vector<pal::string_t> framework_versions;