add a test for topological ordering preference

Author: aignas

python/private/py_executable.bzl

@@ -680,17 +680,17 @@ def _create_venv_symlinks(ctx, venv_dir_map):
     return venv_files
 
 def _build_link_map(entries):
-    # dict[str kind, dict[str rel_path, str link_to_path]]
+    # dict[str kind, dict[str key or rel_path, tuple[str rel_path, str link_to_path]]]
     link_map = {}
     for entry in entries:
         kind = entry.kind
         kind_map = link_map.setdefault(kind, {})
-        if entry.venv_path in kind_map:
-            # We ignore duplicates by design. The dependency closer to the
-            # binary gets precedence due to the topological ordering.
-            continue
-        else:
-            kind_map[entry.venv_path] = entry.link_to_path
+
+        # We overwrite duplicates by design. The dependency closer to the
+        # binary gets precedence due to the topological ordering.
+        #
+        # This allows us to store only one version of the dist-info that is needed
+        kind_map[entry.key or entry.venv_path] = (entry.venv_path, entry.link_to_path)
 
     # An empty link_to value means to not create the site package symlink.
     # Because of the topological ordering, this allows binaries to remove
@@ -710,7 +710,7 @@ def _build_link_map(entries):
         for _ in range(len(kind_map)):
             if not kind_map:
                 break
-            dirname, value = kind_map.popitem()
+            _, (dirname, value) = kind_map.popitem()
             keep_kind_map[dirname] = value
             prefix = dirname + "/"  # Add slash to prevent /X matching /XY
             for maybe_suffix in kind_map.keys():

python/private/py_info.bzl

@@ -45,7 +45,6 @@ def _VenvSymlinkKind_typedef():
 VenvSymlinkKind = struct(
     TYPEDEF = _VenvSymlinkKind_typedef,
     BIN = "BIN",
-    DISTINFO = "DISTINFO",
     LIB = "LIB",
     INCLUDE = "INCLUDE",
 )
@@ -57,6 +56,11 @@ VenvSymlinkEntry = provider(
 An entry in `PyInfo.venv_symlinks`
 """,
     fields = {
+        "key": """
+:type: str | None
+
+Represents the value that gets used to key entries so that each package is represented only once.
+""",
         "kind": """
 :type: str
 

python/private/py_library.bzl

@@ -41,6 +41,7 @@ load(
     "runfiles_root_path",
 )
 load(":flags.bzl", "AddSrcsToRunfilesFlag", "PrecompileFlag", "VenvsSitePackages")
+load(":normalize_name.bzl", "normalize_name")
 load(":precompile.bzl", "maybe_precompile")
 load(":py_cc_link_params_info.bzl", "PyCcLinkParamsInfo")
 load(":py_info.bzl", "PyInfo", "VenvSymlinkEntry", "VenvSymlinkKind")
@@ -270,20 +271,21 @@ def _get_venv_symlinks(ctx, dist_info_metadata):
     repo_runfiles_dirname = None
     dirs_with_init = {}  # dirname -> runfile path
     venv_symlinks = []
+    package = ""
     if dist_info_metadata:
         # in order to be able to have replacements in the venv, we have to add a
         # third value into the venv_symlinks, which would be the normalized
         # package name. This allows us to ensure that we can replace the `dist-info`
         # directories by checking if the package key is there.
         dist_info_dir = paths.basename(dist_info_metadata.dirname)
-        #TODO remove
-        #package, _, _suffix = dist_info_dir.rpartition(".dist-info")
-        #package, _, _version = package.rpartition("-")
-        #symlink_key = "{}.dist-info".format(normalize_name(package))
+        package, _, _suffix = dist_info_dir.rpartition(".dist-info")
+        package, _, _version = package.rpartition("-")
+        package = normalize_name(package)
 
         repo_runfiles_dirname = runfiles_root_path(ctx, dist_info_metadata.short_path).partition("/")[0]
         venv_symlinks.append(VenvSymlinkEntry(
-            kind = VenvSymlinkKind.DISTINFO,
+            key = "{}.dist-info".format(package),
+            kind = VenvSymlinkKind.LIB,
             link_to_path = paths.join(repo_runfiles_dirname, site_packages_root, dist_info_dir),
             venv_path = dist_info_dir,
         ))
@@ -306,6 +308,7 @@ def _get_venv_symlinks(ctx, dist_info_metadata):
             # This would be files that do not have directories and we just need to add
             # direct symlinks to them as is:
             venv_symlinks.append(VenvSymlinkEntry(
+                key = None,
                 kind = VenvSymlinkKind.LIB,
                 link_to_path = paths.join(repo_runfiles_dirname, site_packages_root, filename),
                 venv_path = filename,
@@ -327,6 +330,7 @@ def _get_venv_symlinks(ctx, dist_info_metadata):
 
     for dirname in first_level_explicit_packages:
         venv_symlinks.append(VenvSymlinkEntry(
+            key = None,
             kind = VenvSymlinkKind.LIB,
             link_to_path = paths.join(repo_runfiles_dirname, site_packages_root, dirname),
             venv_path = dirname,

tests/modules/other/simple_v1/site-packages/simple/__init__.py

@@ -1 +1,3 @@
 # Intentionally empty
+
+__version__ = "1.0.0"

tests/modules/other/simple_v2/site-packages/simple/__init__.py

@@ -1 +1,3 @@
 # Intentionally empty
+
+__version__ = "2.0.0"

tests/venv_site_packages_libs/bin.py

@@ -38,6 +38,10 @@ def test_distinfo_is_overriden(self):
 
         dist_info_dirs = [p.name for p in site_packages.glob("*.dist-info")]
 
+        self.assertEqual(
+            "2.0.0",
+            module.__version__,
+        )
         self.assertEqual(
             ["simple-2.0.0.dist-info"],
             dist_info_dirs,