Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Commit

Permalink
Always compare TPA versions (#4423) (#4521)
Browse files Browse the repository at this point in the history
  • Loading branch information
@steveharter
steveharter authored Sep 13, 2018
1 parent dcf46ac commit 75426ae
Show file tree
Hide file tree
Showing 6 changed files with 215 additions and 337 deletions.
98 changes: 47 additions & 51 deletions src/corehost/cli/deps_resolver.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down Expand Up @@ -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;
Expand All @@ -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;
}
Expand All @@ -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;
}
Expand Down Expand Up @@ -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;
}
Expand Down
38 changes: 0 additions & 38 deletions src/corehost/cli/fx_definition.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
2 changes: 0 additions & 2 deletions src/corehost/cli/fx_definition.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,6 @@ class fx_definition_t
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;
Expand Down
22 changes: 15 additions & 7 deletions src/test/HostActivationTests/GivenThatICareAboutLightupAppActivation.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ namespace Microsoft.DotNet.CoreSetup.Test.HostActivation.LightupApp
{
public class GivenThatICareAboutLightupAppActivation
{
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 static TestProjectFixture PreviouslyBuiltAndRestoredLightupLibTestProjectFixture { get; set; }
private static TestProjectFixture PreviouslyPublishedAndRestoredLightupLibTestProjectFixture { get; set; }
Expand Down Expand Up @@ -484,9 +484,16 @@ public void SharedFx_With_Higher_Version_Wins_Against_Additional_Deps()
.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}");

SharedFramework.DeleteAvailableSharedFxVersions(_fxBaseDir, "9999.0.0", "additionalDeps");
SharedFramework.DeleteAvailableSharedFxVersions(_uberFxBaseDir, "7777.0.0");
Expand Down Expand Up @@ -545,11 +552,12 @@ public void SharedFx_With_Lower_Version_Loses_Against_Additional_Deps()
.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}");

SharedFramework.DeleteAvailableSharedFxVersions(_fxBaseDir, "9999.0.0", "additionalDeps");
SharedFramework.DeleteAvailableSharedFxVersions(_uberFxBaseDir, "7777.0.0");
Expand Down
Loading

0 comments on commit 75426ae

Please sign in to comment.