rules_python icon indicating copy to clipboard operation
rules_python copied to clipboard

Support pyd creation

Open abergmeier-dsfishlabs opened this issue 8 years ago • 13 comments

Description of the problem / feature request / question:

Native Python modules on Windows need to have a .pyd extension (or they will not be recognized). cc_binary does not handle correctly if name is e.g. foo.pyd. You get error message like:

ERROR: ... in linkshared attribute of cc_binary rule //foo.pyd: 'linkshared' used in non-shared library.

If possible, provide a minimal example to reproduce the problem:

cc_binary(
    name = "foo.pyd",
    linkstatic = 1,
    linkshared = 1,
)

Environment info

  • Operating System: Windows 10
  • Bazel version (output of bazel info release): 0.4.4

Have you found anything relevant by searching the web? (e.g. GitHub issues, email threads in the [email protected] archive)

Nope

Anything else, information or logs or outputs that would be helpful?

So IMO either allow any name to be a shared library or perhaps provide a py_cc_library, which handles this specially.

abergmeier-dsfishlabs avatar Feb 06 '17 15:02 abergmeier-dsfishlabs

And py_cc_library can be implemented in Skylark easily.

meteorcloudy avatar Feb 08 '17 09:02 meteorcloudy

Python team, could you please help triaging this issue? Are you aware of this issue? Is it Windows-specific? Is it relevant or obsolete?

laszlocsomor avatar Jul 08 '19 07:07 laszlocsomor

I ran into this issue too.

This is definitely a problem for native python extensions under windows. I spent some time trying to work around it's absence without success.

So a .pyd file is just a renamed .dll file and the python interpreter won't attempt to load a .dll file.

My first attempt was just to build a .dll with a cc_binary rule and then use a genrule to rename the .dll to a .pyd file. But what I ran into with real world examples is other .dll files that the python extension you are building transitively depends on seem pretty hard to support. Trying to add them manually to the genrule curiously stated that they were conflicting with existing output files that I did not seem to be able to reference in my py_library.

So then I built bazel and tested out adding .pyd as a SHARED_LIBRARY in CppFileTypes.java and that worked as expected and solved the issues with transitive dll dependencies.

adam-porich-sm avatar Jul 21 '19 03:07 adam-porich-sm

I tried reproing this issue, and it was a very enlightening experience.

In the process of it I created a complete example that:

  • builds a cc_binary with .dll deps that themselves depend on .dlls (heavily based on https://github.com/bazelbuild/bazel/tree/master/examples/windows/dll)

  • builds a .pyd and collects all .dll deps of it in one directory, plus builds a py_binary that uses them; unfortunately I can't get bazel run working (see link below)

See https://github.com/laszlocsomor/projects/tree/master/bazel/pyd

laszlocsomor avatar Aug 09 '19 15:08 laszlocsomor

/cc @brandjon -- Jon, do you have any plans to support .pyd files? I couldn't tell py_binary.modules to use the directory of my .pyd file (see https://github.com/laszlocsomor/projects/tree/master/bazel/pyd).

laszlocsomor avatar Aug 09 '19 15:08 laszlocsomor

I think there are two issues here:

  1. py_binary is enforcing a hard-coded list of file extensions when linkshared = True. "pyd" isn't on the list. I would advocate for this restriction to simply be eliminated (or at least have some other common ones added). There are many "creative" alias extensions on Windows for "dll". I don't think Bazel should be in the business of policing that.

  2. We really need a story for building python extensions in Bazel. This is probably hard to do in a super principled way, especially considering all of the ways that shared linkage can happen, but some building blocks would be great.

For both, I would refer you to here: https://github.com/google/iree/blob/master/build_tools/python/build_defs.bzl

In that file, you'll see the workaround I have to do for the pyd file, which is a pure waste of work.

Also, this package contains a full implementation of a python native build rule that is compatible with Windows. Feel free to rip it off if any of it is helpful. I'm not a bazel expert, so I'm sure there are some better ways to go about it...

stellaraccident avatar Oct 18 '19 00:10 stellaraccident

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

github-actions[bot] avatar Apr 04 '22 22:04 github-actions[bot]

I'm still seeing patches in OSS projects that work around this in gross ways so i think is still a problem. Please don't close unless if fixed.

stellaraccident avatar Apr 04 '22 23:04 stellaraccident

/cc @oquenchil Is it OK we just add "pyd" as an extra extension of shared library here: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/cpp/CppFileTypes.java;l=207

meteorcloudy avatar Apr 05 '22 08:04 meteorcloudy

If it works then by all means.

oquenchil avatar Apr 05 '22 10:04 oquenchil

So anyone whats to add pyd support, feel free to send a PR.

meteorcloudy avatar Apr 05 '22 10:04 meteorcloudy

Such a patch would be a a stopgap for this very specific case, but I can't think of any reason why the build tool should be enforcing anything here. If you really wanted this to be first class, you would allow the output name to be set (and be configurable) independently (which is what cmake allows). Even with an additional exception, people wanting to have one rule that generates the .so/.dylib/.pyd can't do that. It's really janky and compares badly to cmake, which supports the flexibility for all of the uses in the wild.

stellaraccident avatar Apr 05 '22 12:04 stellaraccident

@stellaraccident technically it's .so/.so/.pyd (yes on macos you must also use .so for "native" package :face_exhaling: ) ref:

  • https://github.com/python/cpython/blob/8781a041a00b7a202d73bcb47606ea10e56fb1d1/Python/dynload_shlib.c#L46
  • https://github.com/python/cpython/blob/8781a041a00b7a202d73bcb47606ea10e56fb1d1/Python/dynload_win.c#L29

@meteorcloudy IMHO I would do like rust and its RUST_RLIB:

 // dynamic library artifact created to be used as python module.
// see https://docs.python.org/3/extending/building.html#building-c-and-c-extensions
  public static final FileType PYTHON_C_EXTENSIONS = FileType.of(".so", ".pyd");

then add it here and there.. e.g. https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/rules/python/PyCommon.java;l=327

Mizux avatar May 25 '22 13:05 Mizux

This issue has been automatically marked as stale because it has not had any activity for 180 days. It will be closed if no further activity occurs in 30 days. Collaborators can add an assignee to keep this open indefinitely. Thanks for your contributions to rules_python!

github-actions[bot] avatar Nov 21 '22 22:11 github-actions[bot]

This issue was automatically closed because it went 30 days without a reply since it was labeled "Can Close?"

github-actions[bot] avatar Dec 21 '22 22:12 github-actions[bot]