Always compare TPA versions (dotnet/core-setup#4423)
authorSteve Harter <steveharter@users.noreply.github.com>
Wed, 8 Aug 2018 18:41:46 +0000 (13:41 -0500)
committerGitHub <noreply@github.com>
Wed, 8 Aug 2018 18:41:46 +0000 (13:41 -0500)
Commit migrated from https://github.com/dotnet/core-setup/commit/210afffd0ac12458146444be6dea783c8b55389f

src/installer/corehost/cli/deps_resolver.cpp
src/installer/corehost/cli/fx_definition.cpp
src/installer/corehost/cli/fx_definition.h
src/installer/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs
src/installer/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs

index 48da225..0658045 100644 (file)
@@ -410,7 +410,7 @@ bool deps_resolver_t::resolve_tpa_list(
     const std::vector<deps_entry_t> empty(0);
     name_to_resolved_asset_map_t items;
 
-    auto process_entry = [&](const pal::string_t& deps_dir, const deps_entry_t& entry, int fx_level, bool compare_versions_on_duplicate) -> bool
+    auto process_entry = [&](const pal::string_t& deps_dir, const deps_entry_t& entry, int fx_level) -> bool
     {
         if (breadcrumb != nullptr && entry.is_serviceable)
         {
@@ -460,36 +460,36 @@ bool deps_resolver_t::resolve_tpa_list(
                 return false;
             }
 
-            if (compare_versions_on_duplicate)
-            {
-                deps_resolved_asset_t* existing_entry = &existing->second;
+            deps_resolved_asset_t* existing_entry = &existing->second;
 
-                // If deps entry is same or newer than existing, then see if it should be replaced
-                if (entry.asset.assembly_version > existing_entry->asset.assembly_version ||
-                    (entry.asset.assembly_version == existing_entry->asset.assembly_version && entry.asset.file_version >= existing_entry->asset.file_version))
+            // If deps entry is same or newer than existing, then see if it should be replaced
+            if (entry.asset.assembly_version > existing_entry->asset.assembly_version ||
+                (entry.asset.assembly_version == existing_entry->asset.assembly_version && entry.asset.file_version >= existing_entry->asset.file_version))
+            {
+                if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path))
                 {
-                    if (probe_deps_entry(entry, deps_dir, fx_level, &resolved_path))
+                    // If the path is the same, then no need to replace
+                    if (resolved_path != existing_entry->resolved_path)
                     {
-                        // If the path is the same, then no need to replace
-                        if (resolved_path != existing_entry->resolved_path)
-                        {
-                            trace::verbose(_X("Replacing deps entry [%s, AssemblyVersion:%s, FileVersion:%s] with [%s, AssemblyVersion:%s, FileVersion:%s]"),
-                                existing_entry->resolved_path.c_str(), existing_entry->asset.assembly_version.as_str().c_str(), existing_entry->asset.file_version.as_str().c_str(),
-                                resolved_path.c_str(), entry.asset.assembly_version.as_str().c_str(), entry.asset.file_version.as_str().c_str());
+                        trace::verbose(_X("Replacing deps entry [%s, AssemblyVersion:%s, FileVersion:%s] with [%s, AssemblyVersion:%s, FileVersion:%s]"),
+                            existing_entry->resolved_path.c_str(), existing_entry->asset.assembly_version.as_str().c_str(), existing_entry->asset.file_version.as_str().c_str(),
+                            resolved_path.c_str(), entry.asset.assembly_version.as_str().c_str(), entry.asset.file_version.as_str().c_str());
 
-                            existing_entry = nullptr;
-                            items.erase(existing);
+                        existing_entry = nullptr;
+                        items.erase(existing);
 
-                            deps_asset_t asset(entry.asset.name, entry.asset.relative_path, entry.asset.assembly_version, entry.asset.file_version);
-                            deps_resolved_asset_t resolved_asset(asset, resolved_path);
-                            add_tpa_asset(resolved_asset, &items);
-                        }
-                    }
-                    else
-                    {
-                        return report_missing_assembly_in_manifest(entry);
+                        deps_asset_t asset(entry.asset.name, entry.asset.relative_path, entry.asset.assembly_version, entry.asset.file_version);
+                        deps_resolved_asset_t resolved_asset(asset, resolved_path);
+                        add_tpa_asset(resolved_asset, &items);
                     }
                 }
+                else if (fx_level != 0)
+                {
+                    // The framework is missing a newer package, so this is an error.
+                    // For compat, it is not an error for the app; this can occur for the main application assembly when using --depsfile
+                    // and the app assembly does not exist with the deps file.
+                    return report_missing_assembly_in_manifest(entry);
+                }
             }
 
             return true;
@@ -507,7 +507,7 @@ bool deps_resolver_t::resolve_tpa_list(
     const auto& deps_entries = get_deps().get_entries(deps_entry_t::asset_types::runtime);
     for (const auto& entry : deps_entries)
     {
-        if (!process_entry(m_app_dir, entry, 0, false))
+        if (!process_entry(m_app_dir, entry, 0))
         {
             return false;
         }
@@ -521,31 +521,27 @@ bool deps_resolver_t::resolve_tpa_list(
         get_dir_assemblies(m_app_dir, _X("local"), &items);
     }
 
-    // Probe FX deps entries after app assemblies are added.
-    for (int i = 1; i < m_fx_definitions.size(); ++i)
+    // If additional deps files were specified that need to be treated as part of the
+    // application, then add them to the mix as well.
+    for (const auto& additional_deps : m_additional_deps)
     {
-        // A minor\major roll-forward affects which layer wins
-        bool is_minor_or_major_roll_forward = m_fx_definitions[i]->did_minor_or_major_roll_forward_occur();
-
-        const auto& deps_entries = m_is_framework_dependent ? m_fx_definitions[i]->get_deps().get_entries(deps_entry_t::asset_types::runtime) : empty;
-        for (const auto& entry : deps_entries)
+        auto additional_deps_entries = additional_deps->get_entries(deps_entry_t::asset_types::runtime);
+        for (auto entry : additional_deps_entries)
         {
-            if (!process_entry(m_fx_definitions[i]->get_dir(), entry, i, is_minor_or_major_roll_forward))
+            if (!process_entry(m_app_dir, entry, 0))
             {
                 return false;
             }
         }
     }
 
-    // If additional deps files were specified that need to be treated as part of the
-    // application, then add them to the mix as well.
-    for (const auto& additional_deps : m_additional_deps)
+    // Probe FX deps entries after app assemblies are added.
+    for (int i = 1; i < m_fx_definitions.size(); ++i)
     {
-        auto additional_deps_entries = additional_deps->get_entries(deps_entry_t::asset_types::runtime);
-        for (auto entry : additional_deps_entries)
+        const auto& deps_entries = m_is_framework_dependent ? m_fx_definitions[i]->get_deps().get_entries(deps_entry_t::asset_types::runtime) : empty;
+        for (const auto& entry : deps_entries)
         {
-            // Always check version numbers to support upgrade scenario where additional deps has newer versions
-            if (!process_entry(m_app_dir, entry, 0, true))
+            if (!process_entry(m_fx_definitions[i]->get_dir(), entry, i))
             {
                 return false;
             }
@@ -792,27 +788,27 @@ bool deps_resolver_t::resolve_probe_dirs(
         (void) library_exists_in_dir(m_app_dir, LIBCLRJIT_NAME, &m_clrjit_path);
     }
 
-    // Add fx package locations to fx_dir
-    for (int i = 1; i < m_fx_definitions.size(); ++i)
+    // Handle any additional deps.json that were specified.
+    for (const auto& additional_deps : m_additional_deps)
     {
-        const auto& fx_entries = m_fx_definitions[i]->get_deps().get_entries(asset_type);
-
-        for (const auto& entry : fx_entries)
+        const auto additional_deps_entries = additional_deps->get_entries(asset_type);
+        for (const auto entry : additional_deps_entries)
         {
-            if (!add_package_cache_entry(entry, m_fx_definitions[i]->get_dir(), i))
+            if (!add_package_cache_entry(entry, m_app_dir, 0))
             {
                 return false;
             }
         }
     }
 
-    // Handle any additional deps.json that were specified.
-    for (const auto& additional_deps : m_additional_deps)
+    // Add fx package locations to fx_dir
+    for (int i = 1; i < m_fx_definitions.size(); ++i)
     {
-        const auto additional_deps_entries = additional_deps->get_entries(asset_type);
-        for (const auto entry : additional_deps_entries)
+        const auto& fx_entries = m_fx_definitions[i]->get_deps().get_entries(asset_type);
+
+        for (const auto& entry : fx_entries)
         {
-            if (!add_package_cache_entry(entry, m_app_dir, 0))
+            if (!add_package_cache_entry(entry, m_fx_definitions[i]->get_dir(), i))
             {
                 return false;
             }
index 481d289..64e1487 100644 (file)
@@ -42,41 +42,3 @@ void fx_definition_t::parse_deps(const deps_json_t::rid_fallback_graph_t& graph)
 {
     m_deps.parse(true, m_deps_file, graph);
 }
-
-bool fx_definition_t::did_minor_or_major_roll_forward_occur() const
-{
-    fx_ver_t requested_ver(-1, -1, -1);
-    if (!fx_ver_t::parse(m_requested_version, &requested_ver, false))
-    {
-        assert(false);
-        return false;
-    }
-
-    fx_ver_t found_ver(-1, -1, -1);
-    if (!fx_ver_t::parse(m_found_version, &found_ver, false))
-    {
-        assert(false);
-        return false;
-    }
-
-    if (requested_ver >= found_ver)
-    {
-        assert(requested_ver == found_ver); // We shouldn't have a > case here
-        return false;
-    }
-
-    if (requested_ver.get_major() != found_ver.get_major())
-    {
-        assert(requested_ver.get_major() < found_ver.get_major());
-        return true;
-    }
-
-    if (requested_ver.get_minor() != found_ver.get_minor())
-    {
-        assert(requested_ver.get_minor() < found_ver.get_minor());
-        return true;
-    }
-
-    // Differs in patch version only
-    return false;
-}
index 3819e0f..c4d38ac 100644 (file)
@@ -32,8 +32,6 @@ public:
     void parse_deps();
     void parse_deps(const deps_json_t::rid_fallback_graph_t& graph);
 
-    bool did_minor_or_major_roll_forward_occur() const;
-
 private:
     pal::string_t m_name;
     pal::string_t m_dir;
index 3b2411f..9b4de2a 100644 (file)
@@ -18,8 +18,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.LightupApp
     {
         private SharedTestState sharedTestState;
 
-        private const string SystemCollectionsImmutableFileVersion = "1.2.3.4";
-        private const string SystemCollectionsImmutableAssemblyVersion = "1.0.1.2";
+        private const string SystemCollectionsImmutableFileVersion = "88.2.3.4";
+        private const string SystemCollectionsImmutableAssemblyVersion = "88.0.1.2";
 
         private string _currentWorkingDir;
         private string _builtDotnet;
@@ -460,9 +460,16 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.LightupApp
                 .And
                 .HaveStdErrContaining($"Adding tpa entry: {uberAssembly}")
                 .And
-                .NotHaveStdErrContaining($"Adding tpa entry: {appAssembly}")
+                .HaveStdErrContaining($"Adding tpa entry: {appAssembly}")
+                .And
+                .HaveStdErrContaining($"Replacing deps entry [{appAssembly}")
+                .And
+                .HaveStdErrContaining($"with [{uberAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}]")
                 .And
-                .NotHaveStdErrContaining($"Replacing deps entry");
+                // Verify final selection in TRUSTED_PLATFORM_ASSEMBLIES
+                .HaveStdErrContaining($"{uberAssembly}{Path.PathSeparator}")
+                .And
+                .NotHaveStdErrContaining($"{appAssembly}{Path.PathSeparator}");
         }
 
         [Fact]
@@ -518,11 +525,12 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.LightupApp
                 .And
                 .HaveStdErrContaining($"Using specified additional deps.json: '{additionalDepsPath}'")
                 .And
-                .HaveStdErrContaining($"Adding tpa entry: {uberAssembly}")
+                .HaveStdErrContaining($"Adding tpa entry: {appAssembly}, AssemblyVersion: 99.9.9.9, FileVersion: 98.9.9.9")
                 .And
-                .HaveStdErrContaining($"Adding tpa entry: {appAssembly}")
+                // Verify final selection in TRUSTED_PLATFORM_ASSEMBLIES
+                .HaveStdErrContaining($"{appAssembly}{Path.PathSeparator}")
                 .And
-                .HaveStdErrContaining($"Replacing deps entry [{uberAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}] with [{appAssembly}, AssemblyVersion:99.9.9.9, FileVersion:98.9.9.9]");
+                .NotHaveStdErrContaining($"{uberAssembly}{Path.PathSeparator}");
         }
 
         private static void CreateLightupFolder(string customLightupPath, string version, string libDepsJson)
index 8205cfb..faa5538 100644 (file)
@@ -9,8 +9,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
 {
     public class GivenThatICareAboutMultilevelSharedFxLookup : IDisposable
     {
-        private const string SystemCollectionsImmutableFileVersion = "1.2.3.4";
-        private const string SystemCollectionsImmutableAssemblyVersion = "1.0.1.2";
+        private const string SystemCollectionsImmutableFileVersion = "88.2.3.4";
+        private const string SystemCollectionsImmutableAssemblyVersion = "88.0.1.2";
 
         private RepoDirectoriesProvider RepoDirectories;
         private TestProjectFixture PreviouslyBuiltAndRestoredPortableTestProjectFixture;
@@ -884,82 +884,6 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
         }
 
         [Fact]
-        public void Multiple_SharedFxLookup_NetCoreApp_MinorRollForward_Wins_Over_UberFx()
-        {
-            var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture
-                .Copy();
-
-            var dotnet = fixture.BuiltDotnet;
-            var appDll = fixture.TestProject.AppDll;
-
-            string runtimeConfig = Path.Combine(fixture.TestProject.OutputDirectory, "SharedFxLookupPortableApp.runtimeconfig.json");
-            SharedFramework.SetRuntimeConfigJson(runtimeConfig, "7777.0.0", null, useUberFramework: true);
-
-            // Add versions in the exe folders
-            SharedFramework.AddAvailableSharedFxVersions(_builtSharedFxDir, _exeSharedFxBaseDir, "9999.1.0");
-            SharedFramework.AddAvailableSharedUberFxVersions(_builtSharedUberFxDir, _exeSharedUberFxBaseDir, "9999.0.0", null, "7777.0.0");
-
-            string uberFile = Path.Combine(_exeSharedUberFxBaseDir, "7777.0.0", "System.Collections.Immutable.dll");
-            string netCoreAppFile = Path.Combine(_exeSharedFxBaseDir, "9999.1.0", "System.Collections.Immutable.dll");
-            // The System.Collections.Immutable.dll is located in the UberFramework and NetCoreApp
-            // Version: NetCoreApp 9999.0.0
-            //          UberFramework 7777.0.0
-            //          'Roll forward on no candidate fx' enabled through config
-            // Exe: NetCoreApp 9999.1.0
-            //      UberFramework 7777.0.0
-            // Expected: 9999.1.0
-            //           7777.0.0
-            dotnet.Exec(appDll)
-                .WorkingDirectory(_currentWorkingDir)
-                .EnvironmentVariable("COREHOST_TRACE", "1")
-                .CaptureStdOut()
-                .CaptureStdErr()
-                .Execute()
-                .Should()
-                .Pass()
-                .And
-                .HaveStdErrContaining($"Replacing deps entry [{uberFile}, AssemblyVersion:1.0.1.2, FileVersion:{SystemCollectionsImmutableFileVersion}] with [{netCoreAppFile}");
-        }
-
-        [Fact]
-        public void Multiple_SharedFxLookup_Uber_Wins_Over_NetCoreApp_On_PatchRollForward()
-        {
-            var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture
-                .Copy();
-
-            var dotnet = fixture.BuiltDotnet;
-            var appDll = fixture.TestProject.AppDll;
-
-            string runtimeConfig = Path.Combine(fixture.TestProject.OutputDirectory, "SharedFxLookupPortableApp.runtimeconfig.json");
-            SharedFramework.SetRuntimeConfigJson(runtimeConfig, "7777.0.0", null, useUberFramework: true);
-
-            // Add versions in the exe folders
-            SharedFramework.AddAvailableSharedFxVersions(_builtSharedFxDir, _exeSharedFxBaseDir, "9999.0.1");
-            SharedFramework.AddAvailableSharedUberFxVersions(_builtSharedUberFxDir, _exeSharedUberFxBaseDir, "9999.0.0", null, "7777.0.0");
-
-            // The System.Collections.Immutable.dll is located in the UberFramework and NetCoreApp
-            // Version: NetCoreApp 9999.0.0
-            //          UberFramework 7777.0.0
-            //          'Roll forward on no candidate fx' enabled through config
-            // Exe: NetCoreApp 9999.0.1
-            //      UberFramework 7777.0.0
-            // Expected: 9999.0.1
-            //           7777.0.0
-            dotnet.Exec(appDll)
-                .WorkingDirectory(_currentWorkingDir)
-                .EnvironmentVariable("COREHOST_TRACE", "1")
-                .CaptureStdOut()
-                .CaptureStdErr()
-                .Execute()
-                .Should()
-                .Pass()
-                .And
-                .HaveStdErrContaining(Path.Combine("7777.0.0", "System.Collections.Immutable.dll"))
-                .And
-                .NotHaveStdErrContaining(Path.Combine("9999.1.0", "System.Collections.Immutable.dll"));
-        }
-
-        [Fact]
         public void SharedFx_Wins_Against_App_On_RollForward_And_Version_Tie()
         {
             var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture
@@ -1005,11 +929,18 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
                 .Should()
                 .Pass()
                 .And
-                .HaveStdErrContaining($"Replacing deps entry [{appAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}] with [{uberAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}]");
+                .HaveStdErrContaining($"Replacing deps entry [{appAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}] with [{uberAssembly}, AssemblyVersion:{SystemCollectionsImmutableAssemblyVersion}, FileVersion:{SystemCollectionsImmutableFileVersion}]")
+                .And
+                // Verify final selection in TRUSTED_PLATFORM_ASSEMBLIES
+                .HaveStdErrContaining($"{uberAssembly}{Path.PathSeparator}")
+                .And
+                .NotHaveStdErrContaining($"{netcoreAssembly}{Path.PathSeparator}")
+                .And
+                .NotHaveStdErrContaining($"{appAssembly}{Path.PathSeparator}");
         }
 
         [Fact]
-        public void SharedFx_Loses_Against_App_On_NoRollForward()
+        public void SharedFx_With_Higher_Version_Wins_Against_App_On_NoRollForward()
         {
             var fixture = PreviouslyBuiltAndRestoredPortableTestProjectFixture
                 .Copy();
@@ -1032,7 +963,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
 
             // Modify the app's deps.json to add System.Collections.Immmutable
             string appDepsJson = Path.Combine(fixture.TestProject.OutputDirectory, "SharedFxLookupPortableApp.deps.json");
-            // Use lower numbers for the app; it should still be selected on non-roll-forward
+            // Use lower numbers for the app
             JObject versionInfo = new JObject();
             versionInfo.Add(new JProperty("assemblyVersion", "0.0.0.1"));
             versionInfo.Add(new JProperty("fileVersion", "0.0.0.2"));
@@ -1045,6 +976,7 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
             // Expected: 9999.0.0
             //           7777.0.0
             // Expected: the framework's version of System.Collections.Immutable is used
+            string uberAssembly = Path.Combine(_exeSharedUberFxBaseDir, "7777.0.0", "System.Collections.Immutable.dll");
             dotnet.Exec(appDll)
                 .WorkingDirectory(_currentWorkingDir)
                 .EnvironmentVariable("COREHOST_TRACE", "1")
@@ -1054,11 +986,14 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.MultilevelSharedFxLooku
                 .Should()
                 .Pass()
                 .And
-                .HaveStdErrContaining($"Adding tpa entry: {appAssembly}, AssemblyVersion: {"0.0.0.1"}, FileVersion: {"0.0.0.2"}")
+                .HaveStdErrContaining($"Adding tpa entry: {uberAssembly}, AssemblyVersion: {SystemCollectionsImmutableAssemblyVersion}, FileVersion: {SystemCollectionsImmutableFileVersion}")
+                .And
+                // Verify final selection in TRUSTED_PLATFORM_ASSEMBLIES
+                .HaveStdErrContaining($"{uberAssembly}{Path.PathSeparator}")
                 .And
-                .NotHaveStdErrContaining($"Adding tpa entry: {netcoreAssembly}, AssemblyVersion: {SystemCollectionsImmutableAssemblyVersion}, FileVersion :{SystemCollectionsImmutableFileVersion}")
+                .NotHaveStdErrContaining($"{netcoreAssembly}{Path.PathSeparator}")
                 .And
-                .NotHaveStdErrContaining($"Replacing deps entry");
+                .NotHaveStdErrContaining($"{appAssembly}{Path.PathSeparator}");
         }
 
         static private JObject GetAdditionalFramework(string fxName, string fxVersion, bool? applyPatches, int? rollForwardOnNoCandidateFx)