Skip to content

Fix a bunch of cases where default features were incorrectly included #2012

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
cqundefine wants to merge 1 commit into
base: main
Choose a base branch
from
+183 −23
Open

Fix a bunch of cases where default features were incorrectly included #2012

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions src/vcpkg-test/dependencies.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1870,6 +1870,44 @@ TEST_CASE ("version install qualified default suppression", "[versionplan]")
check_name_and_version(install_plan.install_actions[1], "b", {"1", 0});
}

TEST_CASE ("version host dependency respects explicit no-defaults", "[versionplan]")
{
MockVersionedPortfileProvider vp;

auto& a_scf = vp.emplace("a", {"1", 0}, VersionScheme::Relaxed).source_control_file;
a_scf->core_paragraph->default_features.push_back({"x"});
a_scf->feature_paragraphs.push_back(make_fpgh("x"));

Dependency host_self{"a"};
host_self.host = true;
host_self.default_features = false;
a_scf->core_paragraph->dependencies.push_back(std::move(host_self));

MockCMakeVarProvider var_provider;

MockBaselineProvider bp;
bp.v["a"] = {"1", 0};

auto install_plan = create_versioned_install_plan(vp, bp, var_provider, {CoreDependency{"a"}}, {}, toplevel_spec())
.value_or_exit(VCPKG_LINE_INFO);

REQUIRE(install_plan.size() == 2);

auto host_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.triplet() == Test::ARM_UWP;
});
REQUIRE(host_it != install_plan.install_actions.end());
check_name_and_version(*host_it, "a", {"1", 0});
REQUIRE(host_it->default_features.empty());

auto target_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.triplet() == toplevel_spec().triplet();
});
REQUIRE(target_it != install_plan.install_actions.end());
check_name_and_version(*target_it, "a", {"1", 0});
REQUIRE(target_it->default_features.empty());
}

TEST_CASE ("version install qualified transitive", "[versionplan]")
{
MockVersionedPortfileProvider vp;
Expand Down
88 changes: 88 additions & 0 deletions src/vcpkg-test/plan.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1226,6 +1226,94 @@ TEST_CASE ("basic existing tool port scheme", "[plan]")
}
}

TEST_CASE ("host dependencies suppress default feature metadata", "[plan]")
{
PackageSpecMap spec_map;
spec_map.emplace("a", "a", {{"x", ""}}, {"x"});
auto& dep = spec_map.map.at("a").source_control_file->core_paragraph->dependencies[0];
dep.host = true;
dep.default_features = false;

MapPortFileProvider map_port(spec_map.map);
MockCMakeVarProvider var_provider;

auto install_plan =
create_feature_install_plan(map_port, var_provider, Test::parse_test_fspecs("a"), {}, Test::X64_WINDOWS);

REQUIRE(install_plan.size() == 2);
auto host_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.triplet() == Test::X64_WINDOWS;
});
REQUIRE(host_it != install_plan.install_actions.end());
features_check(*host_it, "a", {"core"}, Test::X64_WINDOWS);
REQUIRE(host_it->default_features.empty());

auto target_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.triplet() != Test::X64_WINDOWS;
});
REQUIRE(target_it != install_plan.install_actions.end());
features_check(*target_it, "a", {"x", "core"});
REQUIRE(target_it->default_features == std::vector<std::string>{"x"});
}

TEST_CASE ("host dependencies respect explicit no-defaults on core install", "[plan]")
{
PackageSpecMap spec_map;
spec_map.emplace("a", "a", {{"x", ""}}, {"x"});
auto& dep = spec_map.map.at("a").source_control_file->core_paragraph->dependencies[0];
dep.host = true;
dep.default_features = false;

MapPortFileProvider map_port(spec_map.map);
MockCMakeVarProvider var_provider;

auto install_plan =
create_feature_install_plan(map_port, var_provider, Test::parse_test_fspecs("a[core]"), {}, Test::X64_WINDOWS);

REQUIRE(install_plan.size() == 2);
features_check(install_plan.install_actions.at(0), "a", {"core"}, Test::X64_WINDOWS);
REQUIRE(install_plan.install_actions.at(0).default_features.empty());
features_check(install_plan.install_actions.at(1), "a", {"core"});
REQUIRE(install_plan.install_actions.at(1).default_features.empty());
}

TEST_CASE ("host dependency reinstall respects explicit no-defaults", "[plan]")
{
std::vector<std::unique_ptr<StatusParagraph>> status_paragraphs;
status_paragraphs.push_back(make_status_pgh("a", "", "", "x64-windows"));
StatusParagraphs status_db(std::move(status_paragraphs));

PackageSpecMap spec_map(Test::ARM_UWP);
spec_map.emplace("a", "", {{"x", ""}, {"y", ""}}, {"x"});
spec_map.emplace("b", "a[y]");
auto& dep = spec_map.map.at("b").source_control_file->core_paragraph->dependencies[0];
dep.host = true;
dep.default_features = false;

MapPortFileProvider map_port(spec_map.map);
MockCMakeVarProvider var_provider;

auto install_plan = create_feature_install_plan(
map_port, var_provider, Test::parse_test_fspecs("b:arm-uwp"), status_db, Test::X64_WINDOWS);

REQUIRE(install_plan.remove_actions.size() == 1);
remove_plan_check(install_plan.remove_actions.at(0), "a", Test::X64_WINDOWS);
REQUIRE(install_plan.install_actions.size() == 2);

auto host_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.name() == "a" && action.spec.triplet() == Test::X64_WINDOWS;
});
REQUIRE(host_it != install_plan.install_actions.end());
features_check(*host_it, "a", {"y", "core"}, Test::X64_WINDOWS);
REQUIRE(host_it->default_features.empty());

auto target_it = Util::find_if(install_plan.install_actions, [](const InstallPlanAction& action) {
return action.spec.name() == "b" && action.spec.triplet() == Test::ARM_UWP;
});
REQUIRE(target_it != install_plan.install_actions.end());
features_check(*target_it, "b", {"core"}, Test::ARM_UWP);
}

TEST_CASE ("remove tool port scheme", "[plan]")
{
std::vector<std::unique_ptr<StatusParagraph>> pghs;
Expand Down
80 changes: 57 additions & 23 deletions src/vcpkg/dependencies.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,8 @@ namespace vcpkg
void add_feature(const std::string& feature,
const CMakeVars::CMakeVarProvider& var_provider,
std::vector<FeatureSpec>& out_new_dependencies,
Triplet host_triplet)
Triplet host_triplet,
std::unordered_set<PackageSpec>& explicit_no_default_host_deps)
{
const auto& scfl = get_scfl_or_exit();

Expand Down Expand Up @@ -193,6 +194,10 @@ namespace vcpkg
}
}
auto fullspec = dep.to_full_spec(features, m_spec.triplet(), host_triplet);
if (dep.host && !dep.default_features)
{
explicit_no_default_host_deps.insert(fullspec.package_spec);
}
fullspec.expand_fspecs_to(dep_list);
if (auto opt = dep.constraint.try_get_minimum_version())
{
Expand All @@ -216,6 +221,10 @@ namespace vcpkg
dep.to_full_spec(Util::fmap(dep.features, [](const auto& f) { return f.name; }),
m_spec.triplet(),
host_triplet);
if (dep.host && !dep.default_features)
{
explicit_no_default_host_deps.insert(fullspec.package_spec);
}
fullspec.expand_fspecs_to(dep_list);
if (auto opt = dep.constraint.try_get_minimum_version())
{
Expand Down Expand Up @@ -243,7 +252,8 @@ namespace vcpkg
Util::Vectors::append(out_new_dependencies, std::move(dep_list));
}

void create_install_info(std::vector<FeatureSpec>& out_reinstall_requirements)
void create_install_info(std::vector<FeatureSpec>& out_reinstall_requirements,
bool suppress_implicit_default_features = false)
{
Comment thread
cqundefine marked this conversation as resolved.
bool defaults_requested = false;
if (const ClusterInstalled* inst = m_installed.get())
Expand All @@ -265,7 +275,7 @@ namespace vcpkg
{
out_reinstall_requirements.emplace_back(m_spec, FeatureNameDefault);
}
else if (request_type != RequestType::USER_REQUESTED)
else if (!suppress_implicit_default_features && request_type != RequestType::USER_REQUESTED)
{
out_reinstall_requirements.emplace_back(m_spec, FeatureNameDefault);
m_install_info.get()->reduced_defaults = true;
Expand Down Expand Up @@ -348,7 +358,8 @@ namespace vcpkg
Editable editable_if_user_requested) const;

void mark_for_reinstall(const PackageSpec& spec,
std::vector<FeatureSpec>& out_reinstall_requirements) const;
std::vector<FeatureSpec>& out_reinstall_requirements,
const std::unordered_set<PackageSpec>& explicit_no_default_host_deps) const;
const CMakeVars::CMakeVarProvider& m_var_provider;

std::unique_ptr<ClusterGraph> m_graph;
Expand Down Expand Up @@ -750,7 +761,8 @@ namespace vcpkg
}

void PackageGraph::mark_for_reinstall(const PackageSpec& first_remove_spec,
std::vector<FeatureSpec>& out_reinstall_requirements) const
std::vector<FeatureSpec>& out_reinstall_requirements,
const std::unordered_set<PackageSpec>& explicit_no_default_host_deps) const
{
std::set<PackageSpec> removed;
std::vector<PackageSpec> to_remove{first_remove_spec};
Expand All @@ -767,7 +779,9 @@ namespace vcpkg

if (!clust.m_install_info)
{
clust.create_install_info(out_reinstall_requirements);
const bool suppress_implicit_defaults =
Util::Sets::contains(explicit_no_default_host_deps, remove_spec);
clust.create_install_info(out_reinstall_requirements, suppress_implicit_defaults);
}

to_remove.insert(to_remove.end(), info.remove_edges.begin(), info.remove_edges.end());
Expand All @@ -783,6 +797,7 @@ namespace vcpkg
std::vector<FeatureSpec> next_dependencies{specs.begin(), specs.end()};

// Keep running while there is any chance of finding more dependencies
std::unordered_set<PackageSpec> explicit_no_default_host_deps;
while (!next_dependencies.empty())
{
// Keep running until the only dependencies left are qualified
Expand Down Expand Up @@ -875,14 +890,24 @@ namespace vcpkg

if (clust.m_install_info.has_value())
{
clust.add_feature(spec.feature(), m_var_provider, next_dependencies, m_graph->m_host_triplet);
clust.add_feature(spec.feature(),
m_var_provider,
next_dependencies,
m_graph->m_host_triplet,
explicit_no_default_host_deps);
}
else
{
if (!clust.m_installed.has_value())
{
clust.create_install_info(next_dependencies);
clust.add_feature(spec.feature(), m_var_provider, next_dependencies, m_graph->m_host_triplet);
const bool suppress_implicit_defaults =
Util::Sets::contains(explicit_no_default_host_deps, spec.spec());
clust.create_install_info(next_dependencies, suppress_implicit_defaults);
clust.add_feature(spec.feature(),
m_var_provider,
next_dependencies,
m_graph->m_host_triplet,
explicit_no_default_host_deps);
}
else
{
Expand All @@ -893,7 +918,7 @@ namespace vcpkg
clust.m_installed.get()->defaults_requested = true;
if (!clust.has_defaults_installed())
{
mark_for_reinstall(spec.spec(), next_dependencies);
mark_for_reinstall(spec.spec(), next_dependencies, explicit_no_default_host_deps);
}
}
}
Expand All @@ -902,9 +927,12 @@ namespace vcpkg
// If install_info is not present and it is already installed, we have never added a feature
// which hasn't already been installed to this cluster. In this case, we need to reinstall
// the port if the feature isn't already present.
mark_for_reinstall(spec.spec(), next_dependencies);
clust.add_feature(
spec.feature(), m_var_provider, next_dependencies, m_graph->m_host_triplet);
mark_for_reinstall(spec.spec(), next_dependencies, explicit_no_default_host_deps);
clust.add_feature(spec.feature(),
m_var_provider,
next_dependencies,
m_graph->m_host_triplet,
explicit_no_default_host_deps);
}
}
}
Expand Down Expand Up @@ -936,7 +964,7 @@ namespace vcpkg
std::vector<FeatureSpec> reinstall_reqs;

for (const PackageSpec& spec : specs)
mark_for_reinstall(spec, reinstall_reqs);
mark_for_reinstall(spec, reinstall_reqs, {});

Util::sort_unique_erase(reinstall_reqs);

Expand Down Expand Up @@ -1428,7 +1456,7 @@ namespace vcpkg

// Add an initial requirement for a package.
// Returns a reference to the node to place additional constraints
PackageNode* require_package(const PackageSpec& spec, const std::string& origin);
PackageNode* require_package(const PackageSpec& spec, const std::string& origin, bool implicit_defaults);

void require_scfl(PackageNode& ref, const SourceControlFileAndLocation* scfl, const std::string& origin);

Expand Down Expand Up @@ -1495,7 +1523,8 @@ namespace vcpkg
if (!dep.platform.is_empty() && !dep.platform.evaluate(batch_load_vars(frame))) continue;

PackageSpec dep_spec(dep.name, dep.host ? m_host_triplet : frame.spec.triplet());
auto node = require_package(dep_spec, frame.spec.name());
auto node =
require_package(dep_spec, frame.spec.name(), !dep.host && frame.spec.name() != m_toplevel.name());
if (node)
{
// If the node is overlayed or overridden, don't apply version constraints
Expand Down Expand Up @@ -1626,7 +1655,8 @@ namespace vcpkg
}

VersionedPackageGraph::PackageNode* VersionedPackageGraph::require_package(const PackageSpec& spec,
const std::string& origin)
const std::string& origin,
bool implicit_defaults)
{
auto it = m_graph.find(spec);
if (it != m_graph.end())
Expand Down Expand Up @@ -1682,9 +1712,9 @@ namespace vcpkg
}
}

// Implicit defaults are disabled if spec is requested from top-level spec.
// Note that if top-level doesn't also mark that reference as `[core]`, defaults will be re-engaged.
it->second.default_features = origin != m_toplevel.name();
// For versioned installs, transitive target dependencies start with implicit defaults enabled.
// Host dependencies are handled strictly via each incoming edge's `default-features` flag.
it->second.default_features = implicit_defaults;
it->second.requested_features.insert(FeatureNameCore.to_string());

require_scfl(*it, it->second.scfl, origin);
Expand Down Expand Up @@ -1904,11 +1934,15 @@ namespace vcpkg
auto maybe_vars = m_var_provider.get_or_load_dep_info_vars(p.first->first, m_host_triplet);

std::vector<std::string> default_features;
for (const auto& feature : node->second.scfl->source_control_file->core_paragraph->default_features)
if (node->second.default_features)
{
if (feature.platform.evaluate(maybe_vars))
for (const auto& feature :
node->second.scfl->source_control_file->core_paragraph->default_features)
{
default_features.push_back(feature.name);
if (feature.platform.evaluate(maybe_vars))
{
default_features.push_back(feature.name);
}
}
}
std::vector<DepSpec> deps;
Expand Down
Loading