fix: correctly merge conflicting paths when files (instead of dirs) are being linked

python/private/venv_runfiles.bzl

@@ -163,8 +163,10 @@ def _group_venv_path_entries(entries):
     current_group = None
     current_group_prefix = None
     for entry in entries:
-        prefix = entry.venv_path
-        anchored_prefix = prefix + "/"
+        # NOTE: When a file is being directly linked, the anchored prefix can look
+        # odd, e.g. "foo/__init__.py/". This is OK; it's just used to prevent
+        # incorrect prefix substring matching.
+        anchored_prefix = entry.venv_path + "/"
         if (current_group_prefix == None or
             not anchored_prefix.startswith(current_group_prefix)):
             current_group_prefix = anchored_prefix
@@ -193,26 +195,41 @@ def _merge_venv_path_group(ctx, group, keep_map):
     # be symlinked directly, not the directory containing them, due to
     # dynamic linker symlink resolution semantics on Linux.
     for entry in group:
-        prefix = entry.venv_path
+        root_venv_path = entry.venv_path
+        anchored_link_to_path = entry.link_to_path + "/"
         for file in entry.files.to_list():
-            # Compute the file-specific venv path. i.e. the relative
-            # path of the file under entry.venv_path, joined with
-            # entry.venv_path
             rf_root_path = runfiles_root_path(ctx, file.short_path)
-            if not rf_root_path.startswith(entry.link_to_path):
-                # This generally shouldn't occur in practice, but just
-                # in case, skip them, for lack of a better option.
-                continue
-            venv_path = "{}/{}".format(
-                prefix,
-                rf_root_path.removeprefix(entry.link_to_path + "/"),
-            )
 
-            # For lack of a better option, first added wins. We happen to
-            # go in top-down prefix order, so the highest level namespace
-            # package typically wins.
-            if venv_path not in keep_map:
-                keep_map[venv_path] = file
+            # It's a file (or directory) being directly linked and
+            # must be directly linked.
+            if rf_root_path == entry.link_to_path:
+                # For lack of a better option, first added wins.
+                if entry.venv_path not in keep_map:
+                    keep_map[entry.venv_path] = file
+
+                # Skip anything remaining: anything left is either
+                # the same path (first set wins), a suffix (violates
+                # preconditions and can't link anyways), or not under
+                # the prefix (violates preconditions).
+                break
+            else:
+                # Compute the file-specific venv path. i.e. the relative
+                # path of the file under entry.venv_path, joined with
+                # entry.venv_path
+                head, match, rel_venv_path = rf_root_path.partition(anchored_link_to_path)
+                if not match or head:
+                    # If link_to_path didn't match, then obviously skip.
+                    # If head is non-empty, it means link_to_path wasn't
+                    # found at the start
+                    # This shouldn't occur in practice, but guard against it
+                    # just in case
+                    continue
+
+                venv_path = paths.join(root_venv_path, rel_venv_path)
+
+                # For lack of a better option, first added wins.
+                if venv_path not in keep_map:
+                    keep_map[venv_path] = file
 
 def get_venv_symlinks(ctx, files, package, version_str, site_packages_root):
     """Compute the VenvSymlinkEntry objects for a library.

tests/venv_site_packages_libs/app_files_building/app_files_building_tests.bzl

@@ -1,6 +1,5 @@
 ""
 
-load("@bazel_skylib//lib:paths.bzl", "paths")
 load("@rules_testing//lib:analysis_test.bzl", "analysis_test")
 load("@rules_testing//lib:test_suite.bzl", "test_suite")
 load("//python/private:py_info.bzl", "VenvSymlinkEntry", "VenvSymlinkKind")  # buildifier: disable=bzl-visibility
@@ -58,34 +57,16 @@ def _venv_symlinks_from_entries(entries):
         ))
     return sorted(result, key = lambda e: (e.link_to_path, e.venv_path))
 
-def _entry(venv_path, link_to_path, files = [], **kwargs):
+def _entry(venv_path, link_to_path, files, **kwargs):
     kwargs.setdefault("kind", VenvSymlinkKind.LIB)
     kwargs.setdefault("package", None)
     kwargs.setdefault("version", None)
-
-    def short_pathify(path):
-        path = paths.join(link_to_path, path)
-
-        # In tests, `../` is used to step out of the link_to_path scope.
-        path = paths.normalize(path)
-
-        # Treat paths starting with "+" as external references. This matches
-        # how bzlmod names things.
-        if link_to_path.startswith("+"):
-            # File.short_path to external repos have `../` prefixed
-            path = paths.join("../", path)
-        else:
-            # File.short_path in main repo is main-repo relative
-            _, _, path = path.partition("/")
-        return path
+    kwargs.setdefault("link_to_file", None)
 
     return VenvSymlinkEntry(
         venv_path = venv_path,
         link_to_path = link_to_path,
-        files = depset([
-            _file(short_pathify(f))
-            for f in files
-        ]),
+        files = depset(files),
         **kwargs
     )
 
@@ -100,15 +81,34 @@ _tests.append(_test_conflict_merging)
 
 def _test_conflict_merging_impl(env, _):
     entries = [
-        _entry("a", "+pypi_a/site-packages/a", ["a.txt"]),
-        _entry("a-1.0.dist-info", "+pypi_a/site-packages/a-1.0.dist-info", ["METADATA"]),
-        _entry("a/b", "+pypi_a_b/site-packages/a/b", ["b.txt"]),
-        _entry("x", "_main/src/x", ["x.txt"]),
-        _entry("x/p", "_main/src-dev/x/p", ["p.txt"]),
-        _entry("duplicate", "+dupe_a/site-packages/duplicate", ["d.py"]),
-        # This entry also provides a/x.py, but since the "a" entry is shorter
-        # and comes first, its version of x.py should win.
-        _entry("duplicate", "+dupe_b/site-packages/duplicate", ["d.py"]),
+        _entry("a", "+pypi_a/site-packages/a", [
+            _file("../+pypi_a/site-packages/a/a.txt"),
+        ]),
+        _entry("a-1.0.dist-info", "+pypi_a/site-packages/a-1.0.dist-info", [
+            _file("../+pypi_a/site-packages/a-1.0.dist-info/METADATA"),
+        ]),
+        _entry("a/b", "+pypi_a_b/site-packages/a/b", [
+            _file("../+pypi_a_b/site-packages/a/b/b.txt"),
+        ]),
+        _entry("x", "_main/src/x", [
+            _file("src/x/x.txt"),
+        ]),
+        _entry("x/p", "_main/src-dev/x/p", [
+            _file("src-dev/x/p/p.txt"),
+        ]),
+        _entry("duplicate", "+dupe_a/site-packages/duplicate", [
+            _file("../+dupe_a/site-packages/duplicate/d.py"),
+        ]),
+        _entry("duplicate", "+dupe_b/site-packages/duplicate", [
+            _file("../+dupe_b/site-packages/duplicate/d.py"),
+        ]),
+        # Case: two distributions provide the same file (instead of directory)
+        _entry("ff/fmod.py", "+ff_a/site-packages/ff/fmod.py", [
+            _file("../+ff_a/site-packages/ff/fmod.py"),
+        ]),
+        _entry("ff/fmod.py", "+ff_b/site-packages/ff/fmod.py", [
+            _file("../+ff_b/site-packages/ff/fmod.py"),
+        ]),
     ]
 
     actual, conflicts = build_link_map(_ctx(), entries, return_conflicts = True)
@@ -117,6 +117,7 @@ def _test_conflict_merging_impl(env, _):
         "a/a.txt": _file("../+pypi_a/site-packages/a/a.txt"),
         "a/b/b.txt": _file("../+pypi_a_b/site-packages/a/b/b.txt"),
         "duplicate/d.py": _file("../+dupe_a/site-packages/duplicate/d.py"),
+        "ff/fmod.py": _file("../+ff_a/site-packages/ff/fmod.py"),
         "x/p/p.txt": _file("src-dev/x/p/p.txt"),
         "x/x.txt": _file("src/x/x.txt"),
     }
@@ -274,8 +275,12 @@ _tests.append(_test_package_version_filtering)
 
 def _test_package_version_filtering_impl(env, _):
     entries = [
-        _entry("foo", "+pypi_v1/site-packages/foo", ["foo.txt"], package = "foo", version = "1.0"),
-        _entry("foo", "+pypi_v2/site-packages/foo", ["bar.txt"], package = "foo", version = "2.0"),
+        _entry("foo", "+pypi_v1/site-packages/foo", [
+            _file("../+pypi_v1/site-packages/foo/foo.txt"),
+        ], package = "foo", version = "1.0"),
+        _entry("foo", "+pypi_v2/site-packages/foo", [
+            _file("../+pypi_v2/site-packages/foo/bar.txt"),
+        ], package = "foo", version = "2.0"),
     ]
 
     actual = build_link_map(_ctx(), entries)
@@ -300,15 +305,15 @@ def _test_malformed_entry_impl(env, _):
             "a",
             "+pypi_a/site-packages/a",
             # This file is outside the link_to_path, so it should be ignored.
-            ["../outside.txt"],
+            [_file("../+pypi_a/site-packages/outside.txt")],
         ),
         # A second, conflicting, entry is added to force merging of the known
         # files. Without this, there's no conflict, so files is never
         # considered.
         _entry(
             "a",
             "+pypi_b/site-packages/a",
-            ["../outside.txt"],
+            [_file("../+pypi_b/site-packages/outside.txt")],
         ),
     ]
 
@@ -328,11 +333,21 @@ _tests.append(_test_complex_namespace_packages)
 
 def _test_complex_namespace_packages_impl(env, _):
     entries = [
-        _entry("a/b", "+pypi_a_b/site-packages/a/b", ["b.txt"]),
-        _entry("a/c", "+pypi_a_c/site-packages/a/c", ["c.txt"]),
-        _entry("x/y/z", "+pypi_x_y_z/site-packages/x/y/z", ["z.txt"]),
-        _entry("foo", "+pypi_foo/site-packages/foo", ["foo.txt"]),
-        _entry("foobar", "+pypi_foobar/site-packages/foobar", ["foobar.txt"]),
+        _entry("a/b", "+pypi_a_b/site-packages/a/b", [
+            _file("../+pypi_a_b/site-packages/a/b/b.txt"),
+        ]),
+        _entry("a/c", "+pypi_a_c/site-packages/a/c", [
+            _file("../+pypi_a_c/site-packages/a/cc.txt"),
+        ]),
+        _entry("x/y/z", "+pypi_x_y_z/site-packages/x/y/z", [
+            _file("../+pypi_x_y_z/site-packages/x/y/z/z.txt"),
+        ]),
+        _entry("foo", "+pypi_foo/site-packages/foo", [
+            _file("../+pypi_foo/site-packages/foo/foo.txt"),
+        ]),
+        _entry("foobar", "+pypi_foobar/site-packages/foobar", [
+            _file("../+pypi_foobar/site-packages/foobar/foobar.txt"),
+        ]),
     ]
 
     actual = build_link_map(_ctx(), entries)
@@ -380,19 +395,19 @@ def _test_multiple_venv_symlink_kinds_impl(env, _):
         _entry(
             "libfile",
             "+pypi_lib/site-packages/libfile",
-            ["lib.txt"],
+            [_file("../+pypi_lib/site-packages/libfile/lib.txt")],
             kind = VenvSymlinkKind.LIB,
         ),
         _entry(
             "binfile",
             "+pypi_bin/bin/binfile",
-            ["bin.txt"],
+            [_file("../+pypi_bin/bin/binfile/bin.txt")],
             kind = VenvSymlinkKind.BIN,
         ),
         _entry(
             "includefile",
             "+pypi_include/include/includefile",
-            ["include.h"],
+            [_file("../+pypi_include/include/includefile/include.h")],
             kind =
                 VenvSymlinkKind.INCLUDE,
         ),