[vcpkg] Fix resolution of default features when using Manifest mode (#…

…12829) * [vcpkg] Fix resolution of default features when using Manifest mode During manifest mode, the dependencies in the manifest should be treated as explicitly specified -- curl[core] should not install curl's default features. * [vcpkg] Improve error message when failed to parse manifest file Co-authored-by: Robert Schumacher <[email protected]>
microsoft · Aug 12, 2020 · c771e7b · c771e7b
1 parent f3b96f3
commit c771e7b
Show file tree

Hide file tree

Showing 9 changed files with 185 additions and 82 deletions.
diff --git a/toolsrc/include/vcpkg-test/util.h b/toolsrc/include/vcpkg-test/util.h
@@ -18,6 +18,22 @@
         }                                                                                                              \
     } while (0)
 
+namespace Catch
+{
+    template<>
+    struct StringMaker<vcpkg::FullPackageSpec>
+    {
+        static std::string convert(vcpkg::FullPackageSpec const& value)
+        {
+            return vcpkg::Strings::concat(value.package_spec.name(),
+                                          '[',
+                                          vcpkg::Strings::join(",", value.features),
+                                          "]:",
+                                          value.package_spec.triplet());
+        }
+    };
+}
+
 namespace vcpkg::Test
 {
     std::unique_ptr<SourceControlFile> make_control_file(

diff --git a/toolsrc/include/vcpkg/dependencies.h b/toolsrc/include/vcpkg/dependencies.h
@@ -161,6 +161,12 @@ namespace vcpkg::Dependencies
                                    const StatusParagraphs& status_db,
                                    const CreateInstallPlanOptions& options = {});
 
+    // `features` should have "default" instead of missing "core". This is only exposed for testing purposes.
+    std::vector<FullPackageSpec> resolve_deps_as_top_level(const SourceControlFile& scf,
+                                                           Triplet triplet,
+                                                           std::vector<std::string> features,
+                                                           CMakeVars::CMakeVarProvider& var_provider);
+
     void print_plan(const ActionPlan& action_plan,
                     const bool is_recursive = true,
                     const fs::path& default_ports_dir = {});

diff --git a/toolsrc/include/vcpkg/packagespec.h b/toolsrc/include/vcpkg/packagespec.h
@@ -22,8 +22,6 @@ namespace vcpkg
     ///
     struct PackageSpec
     {
-        constexpr static StringLiteral MANIFEST_NAME = "default";
-
         PackageSpec() = default;
         PackageSpec(std::string name, Triplet triplet) : m_name(std::move(name)), m_triplet(triplet) { }
 
@@ -50,6 +48,9 @@ namespace vcpkg
         Triplet m_triplet;
     };
 
+    bool operator==(const PackageSpec& left, const PackageSpec& right);
+    inline bool operator!=(const PackageSpec& left, const PackageSpec& right) { return !(left == right); }
+
     ///
     /// <summary>
     /// Full specification of a feature. Contains all information to reference
@@ -111,6 +112,12 @@ namespace vcpkg
                                                   const std::vector<std::string>& all_features) const;
 
         static ExpectedS<FullPackageSpec> from_string(const std::string& spec_as_string, Triplet default_triplet);
+
+        bool operator==(const FullPackageSpec& o) const
+        {
+            return package_spec == o.package_spec && features == o.features;
+        }
+        bool operator!=(const FullPackageSpec& o) const { return !(*this == o); }
     };
 
     ///
@@ -150,9 +157,6 @@ namespace vcpkg
     Optional<std::string> parse_package_name(Parse::ParserBase& parser);
     ExpectedS<ParsedQualifiedSpecifier> parse_qualified_specifier(StringView input);
     Optional<ParsedQualifiedSpecifier> parse_qualified_specifier(Parse::ParserBase& parser);
-
-    bool operator==(const PackageSpec& left, const PackageSpec& right);
-    inline bool operator!=(const PackageSpec& left, const PackageSpec& right) { return !(left == right); }
 }
 
 namespace std

diff --git a/toolsrc/include/vcpkg/portfileprovider.h b/toolsrc/include/vcpkg/portfileprovider.h
@@ -32,10 +32,7 @@ namespace vcpkg::PortFileProvider
         std::vector<const SourceControlFileLocation*> load_all_control_files() const override;
 
     private:
-        const SourceControlFileLocation* load_manifest_file() const;
-
         Files::Filesystem& filesystem;
-        fs::path manifest;
         std::vector<fs::path> ports_dirs;
         mutable std::unordered_map<std::string, SourceControlFileLocation> cache;
     };

diff --git a/toolsrc/include/vcpkg/sourceparagraph.h b/toolsrc/include/vcpkg/sourceparagraph.h
@@ -133,7 +133,6 @@ namespace vcpkg
         fs::path source_location;
     };
 
-    std::string get_error_message(Span<const std::unique_ptr<Parse::ParseControlErrorInfo>> error_info_list);
     void print_error_message(Span<const std::unique_ptr<Parse::ParseControlErrorInfo>> error_info_list);
     inline void print_error_message(const std::unique_ptr<Parse::ParseControlErrorInfo>& error_info_list)
     {

diff --git a/toolsrc/src/vcpkg-test/dependencies.cpp b/toolsrc/src/vcpkg-test/dependencies.cpp
@@ -85,3 +85,67 @@ TEST_CASE ("qualified dependency", "[dependencies]")
     REQUIRE(plan2.install_actions.size() == 2);
     REQUIRE(plan2.install_actions.at(0).feature_list == std::vector<std::string>{"b1", "core"});
 }
+
+TEST_CASE ("resolve_deps_as_top_level", "[dependencies]")
+{
+    using namespace Test;
+    PackageSpecMap spec_map;
+    FullPackageSpec spec_a{spec_map.emplace("a", "b, b[b1] (linux)"), {}};
+    FullPackageSpec spec_b{spec_map.emplace("b", "", {{"b1", ""}}), {}};
+    FullPackageSpec spec_c{spec_map.emplace("c", "b", {{"c1", "b[b1]"}, {"c2", "c[c1], a"}}, {"c1"}), {"core"}};
+    FullPackageSpec spec_d{spec_map.emplace("d", "c[core]"), {}};
+
+    PortFileProvider::MapPortFileProvider map_port{spec_map.map};
+    MockCMakeVarProvider var_provider;
+    Triplet t_linux = Triplet::from_canonical_name("x64-linux");
+    var_provider.dep_info_vars[{"a", t_linux}].emplace("VCPKG_CMAKE_SYSTEM_NAME", "Linux");
+    {
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("a").source_control_file, Triplet::X86_WINDOWS, {}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == spec_b);
+    }
+    {
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("a").source_control_file, t_linux, {}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == FullPackageSpec({"b", t_linux}, {"b1"}));
+    }
+    {
+        // without defaults
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("c").source_control_file, Triplet::X86_WINDOWS, {}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == spec_b);
+    }
+    FullPackageSpec spec_b_with_b1{spec_b.package_spec, {"b1"}};
+    {
+        // with defaults of c (c1)
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("c").source_control_file, Triplet::X86_WINDOWS, {"default"}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == spec_b_with_b1);
+    }
+    {
+        // with c1
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("c").source_control_file, Triplet::X86_WINDOWS, {"c1"}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == spec_b_with_b1);
+    }
+    {
+        // with c2 implying c1
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("c").source_control_file, Triplet::X86_WINDOWS, {"c2"}, var_provider);
+        REQUIRE(deps.size() == 2);
+        REQUIRE(deps.at(0) == spec_a);
+        REQUIRE(deps.at(1) == spec_b_with_b1);
+    }
+    {
+        // d -> c[core]
+        auto deps = vcpkg::Dependencies::resolve_deps_as_top_level(
+            *spec_map.map.at("d").source_control_file, Triplet::X86_WINDOWS, {}, var_provider);
+        REQUIRE(deps.size() == 1);
+        REQUIRE(deps.at(0) == spec_c);
+    }
+}
diff --git a/toolsrc/src/vcpkg/dependencies.cpp b/toolsrc/src/vcpkg/dependencies.cpp
@@ -592,6 +592,70 @@ namespace vcpkg::Dependencies
         m_graph->get(spec).request_type = RequestType::USER_REQUESTED;
     }
 
+    // `features` should have "default" instead of missing "core"
+    std::vector<FullPackageSpec> resolve_deps_as_top_level(const SourceControlFile& scf,
+                                                           Triplet triplet,
+                                                           std::vector<std::string> features,
+                                                           CMakeVars::CMakeVarProvider& var_provider)
+    {
+        PackageSpec spec{scf.core_paragraph->name, triplet};
+        std::map<std::string, std::vector<std::string>> specs_to_features;
+
+        Optional<const PlatformExpression::Context&> ctx_storage = var_provider.get_dep_info_vars(spec);
+        auto ctx = [&]() -> const PlatformExpression::Context& {
+            if (!ctx_storage)
+            {
+                var_provider.load_dep_info_vars({{spec}});
+                ctx_storage = var_provider.get_dep_info_vars(spec);
+            }
+            return ctx_storage.value_or_exit(VCPKG_LINE_INFO);
+        };
+
+        auto handle_deps = [&](View<Dependency> deps) {
+            for (auto&& dep : deps)
+            {
+                if (dep.platform.is_empty() || dep.platform.evaluate(ctx()))
+                {
+                    if (dep.name == spec.name())
+                        Util::Vectors::append(&features, dep.features);
+                    else
+                        Util::Vectors::append(&specs_to_features[dep.name], dep.features);
+                }
+            }
+        };
+
+        handle_deps(scf.core_paragraph->dependencies);
+        enum class State
+        {
+            NotVisited = 0,
+            Visited,
+        };
+        std::map<std::string, State> feature_state;
+        while (!features.empty())
+        {
+            auto feature = std::move(features.back());
+            features.pop_back();
+
+            if (feature_state[feature] == State::Visited) continue;
+            feature_state[feature] = State::Visited;
+            if (feature == "default")
+            {
+                Util::Vectors::append(&features, scf.core_paragraph->default_features);
+            }
+            else
+            {
+                auto it =
+                    Util::find_if(scf.feature_paragraphs, [&feature](const std::unique_ptr<FeatureParagraph>& ptr) {
+                        return ptr->name == feature;
+                    });
+                if (it != scf.feature_paragraphs.end()) handle_deps(it->get()->dependencies);
+            }
+        }
+        return Util::fmap(specs_to_features, [triplet](std::pair<const std::string, std::vector<std::string>>& p) {
+            return FullPackageSpec({p.first, triplet}, Util::sort_unique_erase(std::move(p.second)));
+        });
+    }
+
     ActionPlan create_feature_install_plan(const PortFileProvider::PortFileProvider& port_provider,
                                            const CMakeVars::CMakeVarProvider& var_provider,
                                            const std::vector<FullPackageSpec>& specs,
@@ -627,16 +691,6 @@ namespace vcpkg::Dependencies
 
         auto res = pgraph.serialize(options);
 
-        const auto is_manifest_spec = [](const auto& action) {
-            return action.spec.name() == PackageSpec::MANIFEST_NAME;
-        };
-
-        Util::erase_remove_if(res.install_actions, is_manifest_spec);
-
-        Checks::check_exit(VCPKG_LINE_INFO,
-                           !std::any_of(res.remove_actions.begin(), res.remove_actions.end(), is_manifest_spec),
-                           "\"default\" should never be installed");
-
         return res;
     }
 

diff --git a/toolsrc/src/vcpkg/install.cpp b/toolsrc/src/vcpkg/install.cpp
@@ -767,24 +767,40 @@ namespace vcpkg::Install
                 pkgsconfig = fs::u8path(it_pkgsconfig->second);
             }
 
-            std::vector<std::string> features;
-
-            if (Util::Sets::contains(options.switches, OPTION_MANIFEST_NO_DEFAULT_FEATURES))
+            std::error_code ec;
+            auto manifest_path = paths.manifest_root_dir / fs::u8path("vcpkg.json");
+            auto maybe_manifest_scf = Paragraphs::try_load_manifest(fs, "manifest", manifest_path, ec);
+            if (ec)
             {
-                features.emplace_back("core");
+                Checks::exit_with_message(
+                    VCPKG_LINE_INFO, "Failed to read manifest %s: %s", manifest_path.u8string(), ec.message());
             }
+            else if (!maybe_manifest_scf)
+            {
+                print_error_message(maybe_manifest_scf.error());
+                Checks::exit_with_message(VCPKG_LINE_INFO, "Failed to read manifest %s.", manifest_path.u8string());
+            }
+            auto& manifest_scf = *maybe_manifest_scf.value_or_exit(VCPKG_LINE_INFO);
 
+            std::vector<std::string> features;
             auto manifest_feature_it = options.multisettings.find(OPTION_MANIFEST_FEATURE);
             if (manifest_feature_it != options.multisettings.end())
             {
-                for (const auto& feature : manifest_feature_it->second)
-                {
-                    features.push_back(feature);
-                }
+                features.insert(features.end(), manifest_feature_it->second.begin(), manifest_feature_it->second.end());
+            }
+            auto core_it = Util::find(features, "core");
+            if (core_it == features.end())
+            {
+                if (!Util::Sets::contains(options.switches, OPTION_MANIFEST_NO_DEFAULT_FEATURES))
+                    features.push_back("default");
+            }
+            else
+            {
+                // remove "core" because resolve_deps_as_top_level uses default-inversion
+                features.erase(core_it);
             }
+            auto specs = resolve_deps_as_top_level(manifest_scf, default_triplet, features, var_provider);
 
-            std::vector<FullPackageSpec> specs;
-            specs.emplace_back(PackageSpec{PackageSpec::MANIFEST_NAME, default_triplet}, std::move(features));
             auto install_plan = Dependencies::create_feature_install_plan(provider, var_provider, specs, {});
 
             for (InstallPlanAction& action : install_plan.install_actions)

diff --git a/toolsrc/src/vcpkg/portfileprovider.cpp b/toolsrc/src/vcpkg/portfileprovider.cpp
@@ -27,10 +27,6 @@ namespace vcpkg::PortFileProvider
                                                  const std::vector<std::string>& ports_dirs_paths)
         : filesystem(paths.get_filesystem())
     {
-        if (paths.manifest_mode_enabled())
-        {
-            manifest = paths.manifest_root_dir / fs::u8path("vcpkg.json");
-        }
         auto& fs = paths.get_filesystem();
         for (auto&& overlay_path : ports_dirs_paths)
         {
@@ -62,38 +58,6 @@ namespace vcpkg::PortFileProvider
         ports_dirs.emplace_back(paths.ports);
     }
 
-    const SourceControlFileLocation* PathsPortFileProvider::load_manifest_file() const
-    {
-        if (!manifest.empty())
-        {
-            std::error_code ec;
-            auto maybe_scf = Paragraphs::try_load_manifest(filesystem, "manifest", manifest, ec);
-            if (ec)
-            {
-                Checks::exit_with_message(
-                    VCPKG_LINE_INFO, "Failed to read manifest file %s: %s", manifest.u8string(), ec.message());
-            }
-
-            if (auto scf = maybe_scf.get())
-            {
-                auto it = cache.emplace(std::piecewise_construct,
-                                        std::forward_as_tuple(PackageSpec::MANIFEST_NAME),
-                                        std::forward_as_tuple(std::move(*scf), manifest.parent_path()));
-                return &it.first->second;
-            }
-            else
-            {
-                vcpkg::print_error_message(maybe_scf.error());
-                Checks::exit_with_message(
-                    VCPKG_LINE_INFO, "Error: Failed to load manifest file %s.", manifest.u8string());
-            }
-        }
-        else
-        {
-            return nullptr;
-        }
-    }
-
     ExpectedS<const SourceControlFileLocation&> PathsPortFileProvider::get_control_file(const std::string& spec) const
     {
         auto cache_it = cache.find(spec);
@@ -102,18 +66,6 @@ namespace vcpkg::PortFileProvider
             return cache_it->second;
         }
 
-        if (spec == PackageSpec::MANIFEST_NAME)
-        {
-            if (auto p = load_manifest_file())
-            {
-                return *p;
-            }
-            else
-            {
-                Checks::unreachable(VCPKG_LINE_INFO);
-            }
-        }
-
         for (auto&& ports_dir : ports_dirs)
         {
             // Try loading individual port
@@ -177,11 +129,6 @@ namespace vcpkg::PortFileProvider
         cache.clear();
         std::vector<const SourceControlFileLocation*> ret;
 
-        if (auto p = load_manifest_file())
-        {
-            ret.push_back(p);
-        }
-
         for (auto&& ports_dir : ports_dirs)
         {
             // Try loading individual port