address review comments

Author: rickeylev

python/private/venv_runfiles.bzl

@@ -259,7 +259,6 @@ def get_venv_symlinks(ctx, files, package, version_str, site_packages_root):
         if _is_linker_loaded_library(filename):
             entry = VenvSymlinkEntry(
                 kind = VenvSymlinkKind.LIB,
-                # todo: omit setting link_to_path
                 link_to_path = paths.join(runfiles_dir_name, site_packages_root, filename),
                 link_to_file = src,
                 package = package,
@@ -297,8 +296,6 @@ def get_venv_symlinks(ctx, files, package, version_str, site_packages_root):
                 files = depset([src]),
             )
             venv_symlinks.append(entry)
-        else:
-            fail("hit", src)
 
     # Sort so that we encounter `foo` before `foo/bar`. This ensures we
     # see the top-most explicit package first.
@@ -336,16 +333,18 @@ def get_venv_symlinks(ctx, files, package, version_str, site_packages_root):
 def _is_linker_loaded_library(filename):
     """Tells if a filename is one that `dlopen()` or the runtime linker handles.
 
-    This should return true for `libfoo.so` (regular C library), but not
-    `foo.so` (Python C extension library).
+    This should return true for regular C libraries, but false for Python
+    C extension modules.
+
+    Python extensions: .so (linux, mac), .pyd (windows)
+
+    C libraries: lib*.so (linux), lib*.so.* (linux), lib*.dylib (mac), .dll (windows)
     """
-    if not filename.startswith("lib"):
-        return False
-    if filename.endswith((".so", ".dylib", ".lib")):
+    if filename.endswith(".dll"):
         return True
-
-    # Versioned library, e.g. libfoo.so.1.2.3
-    if ".so." in filename:
+    if filename.startswith("lib") and (
+        filename.endswith((".so", ".dylib")) or ".so." in filename
+    ):
         return True
     return False
 

tests/support/copy_file.bzl

@@ -8,13 +8,21 @@ def _copy_file_to_dir_impl(ctx):
         inputs = [ctx.file.src],
         outputs = [out_file],
         arguments = [ctx.file.src.path, out_file.path],
+        # Perform a copy to better match how a file install from
+        # a repo-phase (e.g. whl extraction) looks.
         command = 'cp -f "$1" "$2"',
         progress_message = "Copying %{input} to %{output}",
     )
     return [DefaultInfo(files = depset([out_file]))]
 
 copy_file_to_dir = rule(
     implementation = _copy_file_to_dir_impl,
+    doc = """
+This allows copying a file whose name is platform-dependent to a directory.
+
+While bazel_skylib has a copy_file rule, you must statically specify the
+output file name.
+""",
     attrs = {
         "out_dir": attr.string(mandatory = True),
         "src": attr.label(

tests/venv_site_packages_libs/ext_with_libs/increment.h

@@ -3,4 +3,4 @@
 
 int increment(int);
 
-#endif  // TESTS_VVENV_SITE_PACKAGES_LIBS_EXT_WITH_LIBS_INCREMENT_H_
+#endif  // TESTS_VENV_SITE_PACKAGES_LIBS_EXT_WITH_LIBS_INCREMENT_H_