From 87692a339f06eb5a1d4fbb60e5f7b95919f91222 Mon Sep 17 00:00:00 2001
From: Ignas Anikevicius <240938+aignas@users.noreply.github.com>
Date: Sat, 4 May 2024 12:14:47 +0900
Subject: [PATCH] fix(whl_library): stop duplicating deps in whl_library

Before this PR we would incorrectly add deps to the platform-specific
list if there were multiple entries in the `METADATA` file. It seems
that some projects (e.g. [opencv-python]) have multiple entries in their
METADATA file to help SAT solvers with selecting the right version when
different interpreter versions are used.

In our case, we will have only one version of a given package because we
are operating with a locked dependency list, so we should ensure that we
do not have duplicates across the lists. With this PR we are solving
this during the construction of the dependency sets so that the internal
model is always consistent.

Fixes #1873

[opencv-python]: https://pypi.org/project/opencv-python/
---
 CHANGELOG.md                                  |  3 ++
 .../tools/wheel_installer/wheel.py            | 18 ++++++++
 .../tools/wheel_installer/wheel_test.py       | 42 +++++++++++++++++++
 3 files changed, 63 insertions(+)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 415b936e8d..fe68ce1ac9 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -34,6 +34,9 @@ A brief description of the categories of changes:
 * (whl_library): Fix the experimental_target_platforms overriding for platform
   specific wheels when the wheels are for any python interpreter version. Fixes
   [#1810](https://github.com/bazelbuild/rules_python/issues/1810).
+* (whl_library): Stop generating duplicate dependencies when encountering
+  duplicates in the METADATA. Fixes
+  [#1873](https://github.com/bazelbuild/rules_python/issues/1873).
 * (gazelle) In `project` or `package` generation modes, do not generate `py_test`
   rules when there are no test files and do not set `main = "__test__.py"` when
   that file doesn't exist.
diff --git a/python/pip_install/tools/wheel_installer/wheel.py b/python/pip_install/tools/wheel_installer/wheel.py
index 750ebfcf7a..f7c686d9ba 100644
--- a/python/pip_install/tools/wheel_installer/wheel.py
+++ b/python/pip_install/tools/wheel_installer/wheel.py
@@ -370,6 +370,24 @@ def _add(self, dep: str, platform: Optional[Platform]):
 
         if not platform:
             self._deps.add(dep)
+
+            # If the dep is in the platform-specific list, remove it from the select.
+            pop_keys = []
+            for p, deps in self._select.items():
+                if dep not in deps:
+                    continue
+
+                deps.remove(dep)
+                if not deps:
+                    pop_keys.append(p)
+
+            for p in pop_keys:
+                self._select.pop(p)
+            return
+
+        if dep in self._deps:
+            # If the dep is already in the main dependency list, no need to add it in the
+            # platform-specific dependency list.
             return
 
         # Add the platform-specific dep
diff --git a/python/pip_install/tools/wheel_installer/wheel_test.py b/python/pip_install/tools/wheel_installer/wheel_test.py
index f7c847fc9e..20141e2867 100644
--- a/python/pip_install/tools/wheel_installer/wheel_test.py
+++ b/python/pip_install/tools/wheel_installer/wheel_test.py
@@ -264,6 +264,48 @@ def test_deps_spanning_all_target_py_versions_are_added_to_common(self):
         self.assertEqual(["bar", "baz"], got.deps)
         self.assertEqual({}, got.deps_select)
 
+    def test_deps_are_not_duplicated(self):
+        # See an example in
+        # https://files.pythonhosted.org/packages/76/9e/db1c2d56c04b97981c06663384f45f28950a73d9acf840c4006d60d0a1ff/opencv_python-4.9.0.80-cp37-abi3-win32.whl.metadata
+        requires_dist = [
+            "bar >=0.1.0 ; python_version < '3.7'",
+            "bar >=0.2.0 ; python_version >= '3.7'",
+            "bar >=0.4.0 ; python_version >= '3.6' and platform_system == 'Linux' and platform_machine == 'aarch64'",
+            "bar >=0.4.0 ; python_version >= '3.9'",
+            "bar >=0.5.0 ; python_version <= '3.9' and platform_system == 'Darwin' and platform_machine == 'arm64'",
+            "bar >=0.5.0 ; python_version >= '3.10' and platform_system == 'Darwin'",
+            "bar >=0.5.0 ; python_version >= '3.10'",
+            "bar >=0.6.0 ; python_version >= '3.11'",
+        ]
+
+        deps = wheel.Deps(
+            "foo",
+            requires_dist=requires_dist,
+            platforms=wheel.Platform.from_string(["cp310_*"]),
+        )
+        got = deps.build()
+
+        self.assertEqual(["bar"], got.deps)
+        self.assertEqual({}, got.deps_select)
+
+    def test_deps_are_not_duplicated_when_encountering_platform_dep_first(self):
+        # Note, that we are sorting the incoming `requires_dist` and we need to ensure that we are not getting any
+        # issues even if the platform-specific line comes first.
+        requires_dist = [
+            "bar >=0.4.0 ; python_version >= '3.6' and platform_system == 'Linux' and platform_machine == 'aarch64'",
+            "bar >=0.5.0 ; python_version >= '3.9'",
+        ]
+
+        deps = wheel.Deps(
+            "foo",
+            requires_dist=requires_dist,
+            platforms=wheel.Platform.from_string(["cp310_*"]),
+        )
+        got = deps.build()
+
+        self.assertEqual(["bar"], got.deps)
+        self.assertEqual({}, got.deps_select)
+
 
 class MinorVersionTest(unittest.TestCase):
     def test_host(self):