From 50424849dacf3c1a9c70993f0b06ee0d45a63cac Mon Sep 17 00:00:00 2001 From: Steve Harter Date: Wed, 8 Aug 2018 13:41:46 -0500 Subject: [PATCH] Always compare TPA versions (dotnet/core-setup#4423) Commit migrated from https://github.com/dotnet/core-setup/commit/210afffd0ac12458146444be6dea783c8b55389f --- src/installer/corehost/cli/deps_resolver.cpp | 98 ++++++++++---------- src/installer/corehost/cli/fx_definition.cpp | 38 -------- src/installer/corehost/cli/fx_definition.h | 2 - .../GivenThatICareAboutLightupAppActivation.cs | 22 +++-- .../GivenThatICareAboutMultilevelSharedFxLookup.cs | 103 ++++----------------- 5 files changed, 81 insertions(+), 182 deletions(-) diff --git a/src/installer/corehost/cli/deps_resolver.cpp b/src/installer/corehost/cli/deps_resolver.cpp index 48da225..0658045 100644 --- a/src/installer/corehost/cli/deps_resolver.cpp +++ b/src/installer/corehost/cli/deps_resolver.cpp @@ -410,7 +410,7 @@ bool deps_resolver_t::resolve_tpa_list( const std::vector 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; } diff --git a/src/installer/corehost/cli/fx_definition.cpp b/src/installer/corehost/cli/fx_definition.cpp index 481d289..64e1487 100644 --- a/src/installer/corehost/cli/fx_definition.cpp +++ b/src/installer/corehost/cli/fx_definition.cpp @@ -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; -} diff --git a/src/installer/corehost/cli/fx_definition.h b/src/installer/corehost/cli/fx_definition.h index 3819e0f..c4d38ac 100644 --- a/src/installer/corehost/cli/fx_definition.h +++ b/src/installer/corehost/cli/fx_definition.h @@ -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; diff --git a/src/installer/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs b/src/installer/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs index 3b2411f..9b4de2a 100644 --- a/src/installer/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs +++ b/src/installer/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs @@ -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) diff --git a/src/installer/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs b/src/installer/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs index 8205cfb..faa5538 100644 --- a/src/installer/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs +++ b/src/installer/test/HostActivationTests/GivenThatICareAboutMultilevelSharedFxLookup.cs @@ -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) -- 2.7.4