pyo3 python_calling_rust example

[mfarrugi]

1. Do we want this example?
2. Where should py_rust_library.bzl go? Should it use @rules_python instead of native rules?

TODO:

• Move under examples/ffi/python_calling_rust

---

Separately, I noticed that this is unnecessarily linking libm and libstdc++, why do we do that? This happened to blow up on me with a nixpkgs provided python3 (apparently a known issue).

>> ldd -u -r bazel-bin/pyo3/hello_py.so 
Unused direct dependencies:
        /lib/x86_64-linux-gnu/libm.so.6
        /usr/lib/x86_64-linux-gnu/libstdc++.so.6

[UebelAndre]

To make this change more reviewable, could you move all the cargo-raze output to the first commit or something? 😅

I'd raised the question before of what should and shouldn't live in the rules. It seems like the precedent has already been set (rust_wasm_*, rust_bindgen_*, rust_proto_*) that a python binding rule could also be added. And I think pyo3 is becoming popular enough that it'd be worth formalizing a Bazel rule around it. I don't think that would be done via a macro though. I'd expect something that used both rust and python providers but maybe that's overkill? Happy to discuss that more but would like to first know whether or not this is something that belongs in this repo.

cc @hlopko @illicitonion @dfreese

[illicitonion]

1. Do we want this example?
2. Where should py_rust_library.bzl go? Should it use @rules_python instead of native rules?

I think it's handy and reasonable to add, but I don't have particularly strong feelings either way :)

Separately, I noticed that this is unnecessarily linking libm and libstdc++, why do we do that?

I'm pretty sure @hlopko introduced this in #562 - I think before we were never linking them which caused problems for things which needed them, and I'm not sure we have good ways of detecting when we should be...

[hlopko]

I see this PR is still a draft, are we sure @mfarrugi is asking for feedback at this point?

[UebelAndre]

I see this PR is still a draft, are we sure @mfarrugi is asking for feedback at this point?

Maybe I jumped the gun. Sorry, was probably too excited to see a rule for pyo3 😅

[mfarrugi]

Feedback is welcome! I just haven't had time to address the automated lints yet is all.

[mfarrugi]

I looked into doing something fancier than the macro, and it looks like currently the fanciest thing possible is to extract the python version from the current python toolchain and use that to configure pyo3. I don't think it's worth the hassle of setting up a pyo3 toolchain and rule when it's straight forward to configure pyo3 directly.

[mfarrugi]

No clue how to fix the windows build, but should be okay to skip ubuntu 16.04 for having a py3 that's too old.

[dae]

You can support Windows with another select, eg https://github.com/ankitects/anki/blob/af50c445dd89f2d9043810ac94def68887d427c9/pylib/anki/_backend/BUILD.bazel#L56

[mfarrugi]

@dae This is running into the same issue as #423 , how did you resolve it?

[dae]

Not sure what's going on there, but it looks similar to #710. I have not tested the fix listed there; I still use proc-macro-nested=0.1.6 at the moment.

[hlopko]

Separately, I noticed that this is unnecessarily linking libm and libstdc++, why do we do that?

I'm pretty sure @hlopko introduced this in #562 - I think before we were never linking them which caused problems for things which needed them, and I'm not sure we have good ways of detecting when we should be...

Rebuttal!! :) What #562 does is adding stdlib artifact as action input and corresponding flags when C++ toolchain uses static runtime linking (in other words, sets cc_toolchain.static_runtime_lib attribute to something and specifies some features in the CcToolchainConfigInfo). The default Bazel C++ toolchain doesn't specify explicit static runtime library artifacts, it uses system installed libraries and it only sets flags (and that was the behavior before #562 as well).

This is where these flags come from: https://osscs.corp.google.com/bazel/bazel/+/master:tools/cpp/unix_cc_configure.bzl;l=395?q=unix_cc_configure. The "fix" is to set envvar BAZEL_LINKOPTS (if on Bazel <4.0) or BAZEL_LINKLIBS (if on Bazel >= 4.0; bazelbuild/bazel#10905). C++ stuff will likely stop building though (I worry about the process wrapper).

[UebelAndre]

The "fix" is to set envvar BAZEL_LINKOPTS (if on Bazel <4.0) or BAZEL_LINKLIBS (if on Bazel >= 4.0; bazelbuild/bazel#10905). C++ stuff will likely stop building though (I worry about the process wrapper).

Is there no fix that can be applied in just the rules..? I wouldn't really consider anything a "fix" that requires the machine to have a specific environment variable set outside of Bazel. That feels like something that goes against the reproducible spirit of Bazel.

[hlopko]

Is there no fix that can be applied in just the rules..? I wouldn't really consider anything a "fix" that requires the machine to have a specific environment variable set outside of Bazel. That feels like something that goes against the reproducible spirit of Bazel.

(you typically pass --repo_env Bazel option when you want to configure Starlark repositories, and those options can go to the .bazelrc. You typically don't set system wide env vars just for bazel)

I wrote "fix" in quotes because it's not really a fix (and there's not really a bug), but a way to configure the default Bazel C++ toolchain to our liking. Rules rust use C++ toolchain, for example we inspect what linker is configured by the C++ toolchain and we use that for linking Rust as well. If the C++ toolchain the user chose used system installed stuff, we'll take it. If the user chose a C++ toolchain that is nicely hermetic but still hardcoded -lm we'll take it. The problem at hand if I understand correctly is that pyo3 doesn't work with all the possible C++ toolchains, and unfortunately the one used by Marco is among them.

[alex]

👋 I'm one of the authors of pyca/cryptography, a popular python library for cryptography. We've started shipping an extension module written in Rust. I don't know how common it us to use bazel among our users, but I imagine first class support for pyo3 would be a big win for those users.

[UebelAndre]

If windows is the only thing not working, would it be acceptable to merge with the examples disabled in that environment until some windows expert shows up to help fix that? (Either shows up here or in BazelCI 😅)

[mfarrugi]

I'd be fine with that. Did anyone have any other thoughts?

…

On Wed, Sep 1, 2021, 16:48 UebelAndre ***@***.***> wrote: If windows is the only thing not working, would it be acceptable to merge with the examples disabled in that environment until some windows expert shows up to help fix that? (Either shows up here or in BazelCI 😅) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#753 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAROG6GAHL235CJ53KZ5V73T72GQVANCNFSM45TPX7AA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.

[UebelAndre]

@mfarrugi Would you be willing to rebase and resolve the conflicts?

[mfarrugi]

One problem with this macro is that rules_rust doesn't have the equivalent of linkstatic = False as in the (naieve?) cc macro bazelbuild/bazel#701 (comment)

I suspect this is not as much of an issue for Rust since versions are mangled into symbols, but I'm not sure what happens if the same exact crate is statically linked into two separate python extensions that are then loaded into python.

[mfarrugi]

mfarrugi@a483a7f illustrates the issue, and fails like so:

Traceback (most recent call last):
  File "/home/marco/.cache/bazel/_bazel_marco/9ec96d05a32431fe9b2731f854ff9adb/execroot/examples/bazel-out/k8-fastbuild/bin/ffi/python_calling_rust/test.runfiles/examples/ffi/python_calling_rust/test.py", line 12, in <module>
    assert s1 == s2, (s1, s2)
AssertionError: (-877036644, -885769300)

The same thing can happen with the cc macro linked above if there is a dependency that can only be linked statically. Fixing this pedantically would require each py_binary rule to link all of its native extensions at the same time (or require them to be fully dynamically linked), which is far beyond the scope of this PR.

[UebelAndre]

@hlopko is this something you think would be valuable to have as an example? I think it'd be very helpful to users to demonstrate more inter-op between languages so am in favor but also understand potential risk of regressing something we only support as an example.

[UebelAndre]

I don't think this is needed. Perhaps it should be removed unless we're going to be documenting some python rules in the root package.

[wt]

I personally would like to see the example landed somewhere. Is it possible that there should be a repo somewhere that just holds multilanguage examples instead of landing this in a rust language focused repo?