pybind_extension do not behave the same on windows (not working as a deps) #124
Description
ndrl: This is a follow up of the #113 (comment)
TLDR: Explore why, contrary to google3, pybind_extension() must be a data dependencies instead of a deps in py_test.
Table of Content
1 Current Status (v3.0.0)
Here is a list of behaviour you can see when using the pybind_bazel v3.0.0 module...
1.1 py_test() support
In google3 pybind_extension() can be consumed as a deps of py_test (or equivalent: pytype_strict_test, py_trict_test...) while externally our current implementation seems to only work when adding it to data for windows build.
note: using it as deps seems to work, as expected, on linux and macos
see: https://github.com/Mizux/pybind11_bazel/actions/runs/18743765460/job/53466080751#step:9:279
1.1.1 Using data field (Working Everywhere)
Here the idiomatic way to use pybind_extension() and its consumption in a py_test() using data...
pybind_extension(
name = "basic",
srcs = ["basic.cpp"],
)
py_test(
name = "basic_pybind_data_test",
size="small",
srcs = ["basic_test.py"],
main = "basic_test.py",
data = [":basic"],
)1.1.2 Using deps field (Not Working on Windows)
What's work on Blaze and on Unix platform with bazel BUT NOT on Windows
py_test(
name = "basic_pybind_deps_test",
size="small",
srcs = ["basic_test.py"],
main = "basic_test.py",
deps = [":basic"],
)Observed Output on Windows
At compile time we got an issue...
> cd examples/basic && bazel build //...
...
deps attribute of py_test rule //:basic_pybind_deps_test:
# '//:basic_copy_dll_to_pyd' does not have mandatory providers:
# 'PyInfo' or 'CcInfo'.
# Since this rule was created by the macro 'py_test', the error might have been
# caused by the macro implementationhypothesis: since the target is just a copy file rules, all metadata are gone thus the error of PyInfo and CcInfo not available
ref:
Lines 82 to 94 in 9017093
1.2 py_wheel() support
1.2.1 Using extension directly as deps
py_wheel(
name = "basic_pybind_wheel",
testonly = True,
distribution = "basic_pybind",
version = "1.0.0",
deps = [":basic"],
)Observed
Seems OK, basic.so (basic.pyd) is in the whl package
bazel build //...
unzip -l bazel-bin/basic-1.0.0-py3-none-any.whl
Archive: bazel-bin/basic-1.0.0-py3-none-any.whl
Length Date Time Name
--------- ---------- ----- ----
687392 1980-01-01 00:00 basic.so
91 1980-01-01 00:00 basic-1.0.0.dist-info/WHEEL
58 1980-01-01 00:00 basic-1.0.0.dist-info/METADATA
265 1980-01-01 00:00 basic-1.0.0.dist-info/RECORD
--------- -------
687806 4 files1.2.2 Using a py_library with data field
py_library(
name = "basic_lib",
data = [":basic"],
imports = ["."],
)
py_wheel(
name = "basic_lib_wheel",
testonly = True,
distribution = "basic_lib",
version = "2.0.0",
deps = [":basic_lib"],
)Observed
bazel build //...
unzip -l bazel-bin/basic_lib-2.0.0-py3-none-any.whl
Archive: bazel-bin/basic_lib-2.0.0-py3-none-any.whl
Length Date Time Name
--------- ---------- ----- ----
91 1980-01-01 00:00 basic_lib-2.0.0.dist-info/WHEEL
62 1980-01-01 00:00 basic_lib-2.0.0.dist-info/METADATA
210 1980-01-01 00:00 basic_lib-2.0.0.dist-info/RECORD
--------- -------
363 3 filesthe native dynamic library is NOT packaged
1.2.3 Using a py_library with a deps field
TODO(@Mizux)
2 Dev Notes
Here the list of tests I'm doing to better understand the behaviour and try to fix this discrepancy between Blaze and Bazel.
this would ultimately reduce the copybara cost for Googler to maintain public oss project.
2.1 Build a cc_binary's .pyd on Windows
First idea, try to build a .pyd shared library directly instead
Patch:
$ git diff -u
diff --git a/build_defs.bzl b/build_defs.bzl
index af449cb..6a7bd64 100644
--- a/build_defs.bzl
+++ b/build_defs.bzl
@@ -105,18 +105,41 @@ def pybind_extension(
**kwargs
)
- copy_file(
- name = name + "_copy_dll_to_pyd",
- src = name + ".dll",
- out = name + ".pyd",
- testonly = kwargs.get("testonly"),
- visibility = kwargs.get("visibility"),
+ cc_binary(
+ name = name + ".pyd",
+ copts = copts + PYBIND_COPTS + select({
+ Label("@pybind11//:msvc_compiler"): [],
+ "//conditions:default": ["-fvisibility=hidden"],
+ }),
+ features = features + PYBIND_FEATURES,
+ linkopts = linkopts + select({
+ "@platforms//os:osx": ["-undefined", "dynamic_lookup"],
+ Label("@pybind11//:msvc_compiler"): [],
+ "//conditions:default": ["-Wl,-Bsymbolic"],
+ }),
+ linkshared = 1,
+ tags = tags,
+ target_compatible_with = select({
+ "@platforms//os:windows": [],
+ "//conditions:default": ["@platforms//:incompatible"],
+ }),
+ deps = deps + PYBIND_DEPS,
+ **kwargs
)
+ #copy_file(
+ # name = name + "_copy_dll_to_pyd",
+ # src = name + ".dll",
+ # out = name + ".pyd",
+ # testonly = kwargs.get("testonly"),
+ # visibility = kwargs.get("visibility"),
+ #)
+
native.alias(
name = name,
actual = select({
- "@platforms//os:windows": name + "_copy_dll_to_pyd",
+ "@platforms//os:windows": name + ".pyd",
"//conditions:default": name + ".so",
}),
testonly = kwargs.get("testonly"),Observed output
> cd examples/basic && bazel build /...
...
> bazel test --test_output=errors //...
...
FAIL: //:basic_lib_test (Exit 1) (see C:/users/corentinl/_bazel_corentinl/5s5kh63p/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/basic_lib_test/test.log)
INFO: From Testing //:basic_lib_test:
==================== Test output for //:basic_lib_test:
Traceback (most recent call last):
File "C:\Users\CORENT~1\AppData\Local\Temp\Bazel.runfiles_zsqjt5zd\runfiles\_main\basic_test.py", line 4, in <module>
import basic
ModuleNotFoundError: No module named 'basic'
================================================================================
FAIL: //:basic_pybind_data_test (Exit 1) (see C:/users/corentinl/_bazel_corentinl/5s5kh63p/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/basic_pybind_data_test/test.log)
INFO: From Testing //:basic_pybind_data_test:
==================== Test output for //:basic_pybind_data_test:
Traceback (most recent call last):
File "C:\Users\CORENT~1\AppData\Local\Temp\Bazel.runfiles_0ensyhl3\runfiles\_main\basic_test.py", line 4, in <module>
import basic
ModuleNotFoundError: No module named 'basic'
================================================================================after looking at the bazel-bin/ we can see the module is named basic.pyd.dll aka cc_binary() has suffixed the target name since .pyd is not recognized as a extension... -> THIS IS A RULES_CC ISSUE !
2.2 Build a cc_binary's .pyd on Windows while hacking rules_cc implem
Next test is to patch rules_cc to confirm the previous hypothesis then send a patch upstream then wait a bcr release then bump the bazel_deps here...
A tentative patch for rules_cc:
[0]—[~/work/rules_cc]—[main]
[^v^]—[corentinl@corentinl] »git diff -u
diff --git a/cc/common/cc_helper.bzl b/cc/common/cc_helper.bzl
index 8444d4f..91bbed1 100644
--- a/cc/common/cc_helper.bzl
+++ b/cc/common/cc_helper.bzl
@@ -68,6 +68,7 @@ def _libraries_from_linking_context(linking_context):
def _is_valid_shared_library_name(shared_library_name):
if (shared_library_name.endswith(".so") or
shared_library_name.endswith(".dll") or
+ shared_library_name.endswith(".pyd") or
shared_library_name.endswith(".dylib") or
shared_library_name.endswith(".wasm")):
return True
@@ -512,7 +513,7 @@ def _get_toolchain_global_make_variables(cc_toolchain):
result["CROSSTOOLTOP"] = cc_toolchain._crosstool_top_path
return result
-_SHARED_LIBRARY_EXTENSIONS = ["so", "dll", "dylib", "wasm"]
+_SHARED_LIBRARY_EXTENSIONS = ["so", "dll", "pyd", "dylib", "wasm"]
def _is_valid_shared_library_artifact(shared_library):
if (shared_library.extension in _SHARED_LIBRARY_EXTENSIONS):
diff --git a/cc/common/cc_helper_internal.bzl b/cc/common/cc_helper_internal.bzl
index 5309491..ef41e11 100644
--- a/cc/common/cc_helper_internal.bzl
+++ b/cc/common/cc_helper_internal.bzl
@@ -96,7 +96,7 @@ _ARCHIVE = [".a", ".lib"]
_PIC_ARCHIVE = [".pic.a"]
_ALWAYSLINK_LIBRARY = [".lo"]
_ALWAYSLINK_PIC_LIBRARY = [".pic.lo"]
-_SHARED_LIBRARY = [".so", ".dylib", ".dll", ".wasm"]
+_SHARED_LIBRARY = [".so", ".dylib", ".dll", ".pyd", ".wasm"]
_INTERFACE_SHARED_LIBRARY = [".ifso", ".tbd", ".lib", ".dll.a"]
_OBJECT_FILE = [".o", ".obj"]
_PIC_OBJECT_FILE = [".pic.o"]
@@ -170,7 +170,7 @@ _ArtifactCategoryInfo, _unused_new_aci = provider(
_artifact_categories = [
_ArtifactCategoryInfo("STATIC_LIBRARY", "lib", ".a", ".lib"),
_ArtifactCategoryInfo("ALWAYSLINK_STATIC_LIBRARY", "lib", ".lo", ".lo.lib"),
- _ArtifactCategoryInfo("DYNAMIC_LIBRARY", "lib", ".so", ".dylib", ".dll", ".wasm"),
+ _ArtifactCategoryInfo("DYNAMIC_LIBRARY", "lib", ".so", ".dylib", ".dll", ".pyd", ".wasm"),
_ArtifactCategoryInfo("EXECUTABLE", "", "", ".exe", ".wasm"),
_ArtifactCategoryInfo("INTERFACE_LIBRARY", "lib", ".ifso", ".tbd", ".if.lib", ".lib"),
_ArtifactCategoryInfo("PIC_FILE", "", ".pic"),
diff --git a/cc/private/link/create_libraries_to_link_values.bzl b/cc/private/link/create_libraries_to_link_values.bzl
index 3adf203..7a833e8 100644
--- a/cc/private/link/create_libraries_to_link_values.bzl
+++ b/cc/private/link/create_libraries_to_link_values.bzl
@@ -286,11 +286,12 @@ def _add_dynamic_library_to_link(
# -l:libfoo.so.1 -> libfoo.so.1
has_compatible_name = (
name.startswith("lib") or
- (not name.endswith(".so") and not name.endswith(".dylib") and not name.endswith(".dll"))
+ (not name.endswith(".so") and not name.endswith(".dylib") and \
+ not name.endswith(".dll") and not name.endswith(".pyd"))
)
if shared_library and has_compatible_name:
lib_name = name.removeprefix("lib").removesuffix(".so").removesuffix(".dylib") \
- .removesuffix(".dll")
+ .removesuffix(".dll").removesuffix(".pyd")
libraries_to_link_values.append(
_NamedLibraryInfo(
type = _TYPE.DYNAMIC_LIBRARY,
diff --git a/cc/private/rules_impl/cc_binary.bzl b/cc/private/rules_impl/cc_binary.bzl
index 8b3f430..f1d0943 100644
--- a/cc/private/rules_impl/cc_binary.bzl
+++ b/cc/private/rules_impl/cc_binary.bzl
@@ -331,7 +331,7 @@ def _create_transitive_linking_actions(
# entries during linking process.
for libs in precompiled_files[:]:
for artifact in libs:
- if _matches([".so", ".dylib", ".dll", ".ifso", ".tbd", ".lib", ".dll.a"], artifact.basename) or cc_helper.is_valid_shared_library_artifact(artifact):
+ if _matches([".so", ".dylib", ".dll", ".pyd", ".ifso", ".tbd", ".lib", ".dll.a"], artifact.basename) or cc_helper.is_valid_shared_library_artifact(artifact):
library_to_link = cc_common.create_library_to_link(
actions = ctx.actions,
feature_configuration = feature_configuration,
@@ -477,7 +477,7 @@ def cc_binary_impl(ctx, additional_linkopts, force_linkstatic = False):
# the target name.
# This is no longer necessary, the toolchain can figure out the correct file extensions.
target_name = ctx.label.name
- has_legacy_link_shared_name = _is_link_shared(ctx) and (_matches([".so", ".dylib", ".dll"], target_name) or cc_helper.is_valid_shared_library_name(target_name))
+ has_legacy_link_shared_name = _is_link_shared(ctx) and (_matches([".so", ".dylib", ".dll", ".pyd"], target_name) or cc_helper.is_valid_shared_library_name(target_name))
binary = None
is_dbg_build = (cc_toolchain._cpp_configuration.compilation_mode() == "dbg")
if has_legacy_link_shared_name:
diff --git a/cc/private/toolchain/windows_cc_toolchain_config.bzl b/cc/private/toolchain/windows_cc_toolchain_config.bzl
index 5d0d40f..38a67aa 100644
--- a/cc/private/toolchain/windows_cc_toolchain_config.bzl
+++ b/cc/private/toolchain/windows_cc_toolchain_config.bzl
@@ -119,6 +119,11 @@ def _impl(ctx):
prefix = "",
extension = ".dll",
),
+ artifact_name_pattern(
+ category_name = "dynamic_library",
+ prefix = "",
+ extension = ".pyd",
+ ),
artifact_name_pattern(
category_name = "interface_library",
prefix = "",
diff --git a/tests/simple_binary/BUILD b/tests/simple_binary/BUILD
index 7a0fb01..74abb02 100644
--- a/tests/simple_binary/BUILD
+++ b/tests/simple_binary/BUILD
@@ -27,6 +27,17 @@ cc_binary(
linkshared = 1,
)
+# On windows, python use .pyd extension for native modules.
+cc_binary(
+ name = "foo.pyd",
+ srcs = ["foo.cc"],
+ linkshared = 1,
+ target_compatible_with = select({
+ "@platforms//os:windows": [],
+ "//conditions:default": ["@platforms//:incompatible"],
+ }),
+)
+
# Regression test for building C code
# https://github.com/bazelbuild/rules_cc/pull/440#issuecomment-3075519628
cc_binary(-
Here we add a new (windows only)
//tests/simple_binary:foo.pydtarget to test the behaviour ofcc_binary()
note: we can notice a bazel-bin/foo.pyd.dll is still generated (using a fresh cloudtop with bazelisk 8.4.2) aka.pydis not a "valid" extension... -
Seems there is few part with a
.dllbut not.pydstill the code contains few pyd here and there...
BUT not trace a any.pydin tests/... (iegit grep "pyd" testsreturn nothing) -
Actually most of the above code are in "dead" code since rules_cc only enable it with bazel 9 (not released yet
ref: