Emit an error for duplicate overrides. (#1573)

Also makes Json::IDeserializer no longer need a virtual dtor. Before (first one wins): PS D:\test> type vcpkg.json { "dependencies": [ "zlib" ], "overrides": [{"name": "zlib", "version": "1.2.13"}, {"name": "zlib", "version": "1.3.1"}] } Installing 2/2 zlib:[email protected]... Building zlib:[email protected]... C:\Users\bion\AppData\Local\vcpkg\registries\git-trees\ad5a49006f73b45b715299515f31164131b51982: info: installing overlay port from here -- Using cached madler-zlib-v1.2.13.tar.gz. -- Extracting source D:/vcpkg-downloads/madler-zlib-v1.2.13.tar.gz -- Applying patch 0001-Prevent-invalid-inclusions-when-HAVE_-is-set-to-0.patch -- Applying patch 0002-skip-building-examples.patch -- Applying patch 0003-build-static-or-shared-not-both.patch -- Applying patch 0004-android-and-mingw-fixes.patch -- Using source at D:/vcpkg/buildtrees/zlib/src/v1.2.13-f30d2a168d.clean -- Found external ninja('1.12.1'). -- Configuring x64-windows -- Building x64-windows-dbg -- Building x64-windows-rel -- Installing: D:/vcpkg/packages/zlib_x64-windows/share/zlib/vcpkg-cmake-wrapper.cmake -- Fixing pkgconfig file: D:/vcpkg/packages/zlib_x64-windows/lib/pkgconfig/zlib.pc -- Using cached msys2-mingw-w64-x86_64-pkgconf-1~2.3.0-1-any.pkg.tar.zst. -- Using cached msys2-msys2-runtime-3.5.4-2-x86_64.pkg.tar.zst. -- Using msys root at D:/vcpkg-downloads/tools/msys2/21caed2f81ec917b -- Fixing pkgconfig file: D:/vcpkg/packages/zlib_x64-windows/debug/lib/pkgconfig/zlib.pc -- Installing: D:/vcpkg/packages/zlib_x64-windows/share/zlib/copyright -- Performing post-build validation Stored binaries in 1 destinations in 93.9 ms. Elapsed time to handle zlib:x64-windows: 3.7 s zlib:x64-windows package ABI: a858cb84557b70b9fa3b3934233ce9667bef3fcf11c99b00070f799cc44bff14 Total install time: 3.7 s The package zlib is compatible with built-in CMake targets: find_package(ZLIB REQUIRED) target_link_libraries(main PRIVATE ZLIB::ZLIB) After: (an error) D:\test\vcpkg.json: error: $.overrides[1] (an override): zlib already has a declared override note: Extended documentation available at 'https://learn.microsoft.com/vcpkg/users/manifests?WT.mc_id=vcpkg_inproduct_cli'. Resolves https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1665249
microsoft · Jan 17, 2025 · a5528f3 · a5528f3
1 parent f8052c7
commit a5528f3
Show file tree

Hide file tree

Showing 15 changed files with 250 additions and 220 deletions.
diff --git a/azure-pipelines/e2e-ports/broken-manifests/broken-duplicate-overrides/portfile.cmake b/azure-pipelines/e2e-ports/broken-manifests/broken-duplicate-overrides/portfile.cmake
@@ -0,0 +1 @@
+set(VCPKG_POLICY_EMPTY_PACKAGE enabled)
diff --git a/azure-pipelines/e2e-ports/broken-manifests/broken-duplicate-overrides/vcpkg.json b/azure-pipelines/e2e-ports/broken-manifests/broken-duplicate-overrides/vcpkg.json
@@ -0,0 +1,17 @@
+{
+  "name": "broken-duplicate-overrides",
+  "version": "1",
+  "dependencies": [
+    "zlib"
+  ],
+  "overrides": [
+    {
+      "name": "zlib",
+      "version": "1.2.13"
+    },
+    {
+      "name": "zlib",
+      "version": "1.3.1"
+    }
+  ]
+}
diff --git a/azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1 b/azure-pipelines/end-to-end-tests-dir/build-test-ports.ps1
@@ -28,6 +28,12 @@ Throw-IfFailed
 Run-Vcpkg @commonArgs --overlay-ports="$PSScriptRoot/../e2e-ports" --overlay-ports="$PSScriptRoot/../e2e-ports/broken-manifests" install control-file
 Throw-IfFailed
 
+$output = Run-VcpkgAndCaptureOutput @commonArgs --overlay-ports="$PSScriptRoot/../e2e-ports/broken-manifests" install broken-duplicate-overrides
+Throw-IfNotFailed
+if ($output -notmatch "vcpkg\.json: error: \$\.overrides\[1\] \(an override\): zlib already has an override") {
+    throw 'Did not detect duplicate override'
+}
+
 $output = Run-VcpkgAndCaptureOutput @commonArgs --overlay-ports="$PSScriptRoot/../e2e-ports/broken-manifests" install broken-no-name
 Throw-IfNotFailed
 if ($output -notmatch "missing required field 'name'") {

diff --git a/include/vcpkg/base/jsonreader.h b/include/vcpkg/base/jsonreader.h
@@ -18,12 +18,8 @@ namespace vcpkg::Json
         using type = Type;
         virtual LocalizedString type_name() const = 0;
 
-    private:
-        friend struct Reader;
         Optional<Type> visit(Reader&, const Value&) const;
         Optional<Type> visit(Reader&, const Object&) const;
-
-    public:
         virtual Optional<Type> visit_null(Reader&) const;
         virtual Optional<Type> visit_boolean(Reader&, bool) const;
         virtual Optional<Type> visit_integer(Reader& r, int64_t i) const;
@@ -33,14 +29,11 @@ namespace vcpkg::Json
         virtual Optional<Type> visit_object(Reader&, const Object&) const;
         virtual View<StringLiteral> valid_fields() const noexcept;
 
-        virtual ~IDeserializer() = default;
-
     protected:
         IDeserializer() = default;
-        IDeserializer(const IDeserializer&) = default;
-        IDeserializer& operator=(const IDeserializer&) = default;
-        IDeserializer(IDeserializer&&) = default;
-        IDeserializer& operator=(IDeserializer&&) = default;
+        IDeserializer(const IDeserializer&) = delete;
+        IDeserializer& operator=(const IDeserializer&) = delete;
+        ~IDeserializer() = default;
     };
 
     struct Reader
@@ -64,9 +57,6 @@ namespace vcpkg::Json
         StringView origin() const noexcept;
 
     private:
-        template<class Type>
-        friend struct IDeserializer;
-
         std::vector<LocalizedString> m_errors;
         std::vector<LocalizedString> m_warnings;
         struct JsonPathElement
@@ -168,19 +158,8 @@ namespace vcpkg::Json
             }
         }
 
-        template<class Type>
-        Optional<Type> visit(const Value& value, const IDeserializer<Type>& visitor)
-        {
-            return visitor.visit(*this, value);
-        }
-        template<class Type>
-        Optional<Type> visit(const Object& value, const IDeserializer<Type>& visitor)
-        {
-            return visitor.visit(*this, value);
-        }
-
-        template<class Type>
-        Optional<std::vector<Type>> array_elements(const Array& arr, const IDeserializer<Type>& visitor)
+        template<class Type, class Fn>
+        Optional<std::vector<Type>> array_elements_fn(const Array& arr, const IDeserializer<Type>& visitor, Fn callback)
         {
             Optional<std::vector<Type>> result{std::vector<Type>()};
             auto& result_vec = *result.get();
@@ -189,7 +168,7 @@ namespace vcpkg::Json
             for (size_t i = 0; i < arr.size(); ++i)
             {
                 m_path.back().index = static_cast<int64_t>(i);
-                auto opt = visitor.visit(*this, arr[i]);
+                auto opt = callback(*this, visitor, arr[i]);
                 if (auto parsed = opt.get())
                 {
                     if (success)
@@ -208,6 +187,15 @@ namespace vcpkg::Json
             return result;
         }
 
+        template<class Type>
+        Optional<std::vector<Type>> array_elements(const Array& arr, const IDeserializer<Type>& visitor)
+        {
+            return array_elements_fn(
+                arr, visitor, [](Reader& this_, const IDeserializer<Type>& visitor, const Json::Value& value) {
+                    return visitor.visit(this_, value);
+                });
+        }
+
         static uint64_t get_reader_stats();
 
     private:

diff --git a/include/vcpkg/base/message-data.inc.h b/include/vcpkg/base/message-data.inc.h
@@ -1124,6 +1124,7 @@ DECLARE_MESSAGE(DownloadWinHttpError,
                 (msg::system_api, msg::exit_code, msg::url),
                 "",
                 "{url}: {system_api} failed with exit code {exit_code}.")
+DECLARE_MESSAGE(DuplicateDependencyOverride, (msg::package_name), "", "{package_name} already has an override")
 DECLARE_MESSAGE(DuplicatedKeyInObj,
                 (msg::value),
                 "{value} is a json property/object",

diff --git a/include/vcpkg/configuration.h b/include/vcpkg/configuration.h
@@ -89,7 +89,7 @@ namespace vcpkg
         Optional<Configuration> config;
     };
 
-    Json::IDeserializer<Configuration>& get_configuration_deserializer();
+    extern const Json::IDeserializer<Configuration>& configuration_deserializer;
     // Parse configuration from a file containing a valid vcpkg-configuration.json file
     Optional<Configuration> parse_configuration(StringView contents,
                                                 StringView origin,

diff --git a/include/vcpkg/registries-parsing.h b/include/vcpkg/registries-parsing.h
@@ -0,0 +1,43 @@
+#pragma once
+#include <vcpkg/base/jsonreader.h>
+
+#include <vcpkg/registries.h>
+
+namespace vcpkg
+{
+    struct FilesystemVersionDbEntryDeserializer final : Json::IDeserializer<FilesystemVersionDbEntry>
+    {
+        LocalizedString type_name() const override;
+        View<StringLiteral> valid_fields() const noexcept override;
+        Optional<FilesystemVersionDbEntry> visit_object(Json::Reader& r, const Json::Object& obj) const override;
+        FilesystemVersionDbEntryDeserializer(const Path& root) : registry_root(root) { }
+
+    private:
+        Path registry_root;
+    };
+
+    struct FilesystemVersionDbEntryArrayDeserializer final : Json::IDeserializer<std::vector<FilesystemVersionDbEntry>>
+    {
+        virtual LocalizedString type_name() const override;
+        virtual Optional<std::vector<FilesystemVersionDbEntry>> visit_array(Json::Reader& r,
+                                                                            const Json::Array& arr) const override;
+        FilesystemVersionDbEntryArrayDeserializer(const Path& root) : underlying{root} { }
+
+    private:
+        FilesystemVersionDbEntryDeserializer underlying;
+    };
+
+    struct GitVersionDbEntryDeserializer final : Json::IDeserializer<GitVersionDbEntry>
+    {
+        LocalizedString type_name() const override;
+        View<StringLiteral> valid_fields() const noexcept override;
+        Optional<GitVersionDbEntry> visit_object(Json::Reader& r, const Json::Object& obj) const override;
+    };
+
+    struct GitVersionDbEntryArrayDeserializer final : Json::IDeserializer<std::vector<GitVersionDbEntry>>
+    {
+        virtual LocalizedString type_name() const override;
+        virtual Optional<std::vector<GitVersionDbEntry>> visit_array(Json::Reader& r,
+                                                                     const Json::Array& arr) const override;
+    };
+}
diff --git a/include/vcpkg/registries.h b/include/vcpkg/registries.h
@@ -217,8 +217,4 @@ namespace vcpkg
     // No match is 0, exact match is SIZE_MAX, wildcard match is the length of the pattern.
     // Note that the * is included in the match size to distinguish from 0 == no match.
     size_t package_pattern_match(StringView name, StringView pattern);
-
-    std::unique_ptr<Json::IDeserializer<std::vector<GitVersionDbEntry>>> make_git_version_db_deserializer();
-    std::unique_ptr<Json::IDeserializer<std::vector<FilesystemVersionDbEntry>>> make_filesystem_version_db_deserializer(
-        const Path& root);
 }
diff --git a/locales/messages.json b/locales/messages.json
@@ -653,6 +653,8 @@
   "DownloadingVcpkgStandaloneBundle": "Downloading standalone bundle {version}.",
   "_DownloadingVcpkgStandaloneBundle.comment": "An example of {version} is 1.3.8.",
   "DownloadingVcpkgStandaloneBundleLatest": "Downloading latest standalone bundle.",
+  "DuplicateDependencyOverride": "{package_name} already has an override",
+  "_DuplicateDependencyOverride.comment": "An example of {package_name} is zlib.",
   "DuplicatePackagePattern": "Package \"{package_name}\" is duplicated.",
   "_DuplicatePackagePattern.comment": "An example of {package_name} is zlib.",
   "DuplicatePackagePatternFirstOcurrence": "First declared in:",

diff --git a/src/vcpkg-test/configmetadata.cpp b/src/vcpkg-test/configmetadata.cpp
@@ -40,7 +40,7 @@ static Configuration parse_test_configuration(StringView text)
     auto object = Json::parse_object(text, origin).value_or_exit(VCPKG_LINE_INFO);
 
     Json::Reader reader(origin);
-    auto parsed_config_opt = reader.visit(object, get_configuration_deserializer());
+    auto parsed_config_opt = configuration_deserializer.visit(reader, object);
     REQUIRE(reader.errors().empty());
 
     return std::move(parsed_config_opt).value_or_exit(VCPKG_LINE_INFO);
@@ -60,7 +60,7 @@ static void check_errors(const std::string& config_text, const std::string& expe
     auto object = Json::parse_object(config_text, origin).value_or_exit(VCPKG_LINE_INFO);
 
     Json::Reader reader(origin);
-    auto parsed_config_opt = reader.visit(object, get_configuration_deserializer());
+    auto parsed_config_opt = configuration_deserializer.visit(reader, object);
     REQUIRE(!reader.errors().empty());
 
     CHECK_LINES(Strings::join("\n", reader.errors()), expected_errors);

diff --git a/src/vcpkg-test/registries.cpp b/src/vcpkg-test/registries.cpp
@@ -5,7 +5,7 @@
 
 #include <vcpkg/configuration.h>
 #include <vcpkg/documentation.h>
-#include <vcpkg/registries.h>
+#include <vcpkg/registries-parsing.h>
 
 using namespace vcpkg;
 
@@ -276,7 +276,7 @@ static vcpkg::Optional<Configuration> visit_default_registry(Json::Reader& r, Js
 {
     Json::Object config;
     config.insert("default-registry", std::move(reg));
-    return r.visit(config, get_configuration_deserializer());
+    return configuration_deserializer.visit(r, config);
 }
 
 TEST_CASE ("registry_parsing", "[registries]")
@@ -428,7 +428,7 @@ TEST_CASE ("registries report pattern errors", "[registries]")
 })json");
 
     Json::Reader r{"test"};
-    auto maybe_conf = r.visit(test_json, get_configuration_deserializer());
+    auto maybe_conf = configuration_deserializer.visit(r, test_json);
     const auto& errors = r.errors();
     CHECK(!errors.empty());
     REQUIRE(errors.size() == 3);
@@ -474,7 +474,7 @@ TEST_CASE ("registries ignored patterns warning", "[registries]")
 })json");
 
     Json::Reader r{"test"};
-    auto maybe_conf = r.visit(test_json, get_configuration_deserializer());
+    auto maybe_conf = configuration_deserializer.visit(r, test_json);
 
     auto conf = maybe_conf.get();
     REQUIRE(conf);
@@ -550,7 +550,6 @@ TEST_CASE ("registries ignored patterns warning", "[registries]")
 
 TEST_CASE ("git_version_db_parsing", "[registries]")
 {
-    auto filesystem_version_db = make_git_version_db_deserializer();
     Json::Reader r{"test"};
     auto test_json = parse_json(R"json(
 [
@@ -572,7 +571,7 @@ TEST_CASE ("git_version_db_parsing", "[registries]")
 ]
 )json");
 
-    auto results_opt = r.visit(test_json, *filesystem_version_db);
+    auto results_opt = GitVersionDbEntryArrayDeserializer().visit(r, test_json);
     auto& results = results_opt.value_or_exit(VCPKG_LINE_INFO);
     CHECK(results[0].version == SchemedVersion{VersionScheme::Date, {"2021-06-26", 0}});
     CHECK(results[0].git_tree == "9b07f8a38bbc4d13f8411921e6734753e15f8d50");
@@ -585,7 +584,7 @@ TEST_CASE ("git_version_db_parsing", "[registries]")
 
 TEST_CASE ("filesystem_version_db_parsing", "[registries]")
 {
-    auto filesystem_version_db = make_filesystem_version_db_deserializer("a/b");
+    FilesystemVersionDbEntryArrayDeserializer filesystem_version_db("a/b");
 
     {
         Json::Reader r{"test"};
@@ -608,7 +607,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        auto results_opt = r.visit(test_json, *filesystem_version_db);
+        auto results_opt = filesystem_version_db.visit(r, test_json);
         auto& results = results_opt.value_or_exit(VCPKG_LINE_INFO);
         CHECK(results[0].version == SchemedVersion{VersionScheme::String, {"puppies", 0}});
         CHECK(results[0].p == "a/b" VCPKG_PREFERRED_SEPARATOR "c/d");
@@ -630,7 +629,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -645,7 +644,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -660,7 +659,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -675,7 +674,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -690,7 +689,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -705,7 +704,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -720,7 +719,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -735,7 +734,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 
@@ -750,7 +749,7 @@ TEST_CASE ("filesystem_version_db_parsing", "[registries]")
     }
 ]
     )json");
-        CHECK(r.visit(test_json, *filesystem_version_db).value_or_exit(VCPKG_LINE_INFO).empty());
+        CHECK(filesystem_version_db.visit(r, test_json).value_or_exit(VCPKG_LINE_INFO).empty());
         CHECK(!r.errors().empty());
     }
 }