Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

-undefined dynamic_lookup is missing when using rules_apple and --incompatible_enable_cc_toolchain_resolution #57

Closed
kersson opened this issue Sep 4, 2023 · 10 comments · Fixed by #58

Comments

@kersson
Copy link
Contributor

kersson commented Sep 4, 2023

We have a repo in which we use rules_apple and have recently adopted --incompatible_enable_cc_toolchain_resolution, which is set to become the default in Bazel 7.0.0 (bazelbuild/bazel#7260).

With this set of constraints, the -undefined dynamic_lookup is no longer included when linking the pybind_extension, causing undefined symbols linker errors.

I'm assuming this is due to the following:

In other words, bazel no longer includes -undefined dynamic_lookup by default when using the Apple toolchain, which was moved to rules_apple.

A bit more context:

Here's how to repro this:
WORKSPACE

load("@bazel_tools//tools/build_defs/repo:http.bzl", "http_archive")

http_archive(
    name = "build_bazel_rules_apple",
    sha256 = "8ac4c7997d863f3c4347ba996e831b5ec8f7af885ee8d4fe36f1c3c8f0092b2c",
    url = "https://github.com/bazelbuild/rules_apple/releases/download/2.5.0/rules_apple.2.5.0.tar.gz",
)

load(
    "@build_bazel_rules_apple//apple:repositories.bzl",
    "apple_rules_dependencies",
)

apple_rules_dependencies()

load(
    "@build_bazel_rules_swift//swift:repositories.bzl",
    "swift_rules_dependencies",
)

swift_rules_dependencies()

load(
    "@build_bazel_rules_swift//swift:extras.bzl",
    "swift_rules_extra_dependencies",
)

swift_rules_extra_dependencies()

load(
    "@build_bazel_apple_support//lib:repositories.bzl",
    "apple_support_dependencies",
)

apple_support_dependencies()

http_archive(
    name = "pybind11_bazel",
    sha256 = "e8355ee56c2ff772334b4bfa22be17c709e5573f6d1d561c7176312156c27bd4",
    strip_prefix = "pybind11_bazel-2.11.1",
    urls = [
        "https://github.com/pybind/pybind11_bazel/releases/download/v2.11.1/pybind11_bazel-2.11.1.tar.gz",
    ],
)

http_archive(
    name = "pybind11",
    build_file = "@pybind11_bazel//:pybind11.BUILD",
    sha256 = "d475978da0cdc2d43b73f30910786759d593a9d8ee05b1b6846d1eb16c6d2e0c",
    strip_prefix = "pybind11-2.11.1",
    urls = [
        "https://github.com/pybind/pybind11/archive/v2.11.1.tar.gz",
    ],
)

load("@pybind11_bazel//:python_configure.bzl", "python_configure")

python_configure(name = "local_config_python")

BUILD

load("@pybind11_bazel//:build_defs.bzl", "pybind_extension")

pybind_extension(
    name = "foo",
    srcs = ["foo.cc"],
)

foo.cc

#include <pybind11/pybind11.h>

PYBIND11_MODULE(foo, m) { m.attr("BAR") = pybind11::str("baz"); }

Run this command:

bazel build --incompatible_enable_cc_toolchain_resolution //:foo.so
@kersson
Copy link
Contributor Author

kersson commented Sep 4, 2023

I put up a PR (#58) that fixes this issue by explicitly adding -undefined dynamic_lookup to the linkopts on macOS.

Let me know if you have any questions or concerns. Thanks so much for providing this great project!

@junyer
Copy link
Contributor

junyer commented Sep 4, 2023

Thanks for the great bug report! This seems reasonable to me. Good to merge, @rwgk?

@rwgk
Copy link
Collaborator

rwgk commented Sep 4, 2023

Thanks for the great bug report! This seems reasonable to me. Good to merge, @rwgk?

Yes, looks good to me, please go ahead.

(Although I have to admit I'm confused about where Apple is going with -undefined dynamic_lookup.)

@junyer junyer closed this as completed in #58 Sep 4, 2023
@shuyangsun
Copy link

@kersson thank you so much for this fix, running into the same issue. When can we expect a release with this fix?

@junyer
Copy link
Contributor

junyer commented Nov 14, 2023

My initial intention was to have a 1:1 mapping from pybind11_bazel releases to pybind11 releases and python_configure.bzl effectively enforces that, but that might be too inflexible in the face of bugs. Is the next pybind11 release very far off, @rwgk? We might need to switch to an N:1 mapping whereby we strip, say, .bzl.<N> from the version number.

P.S. Note that pybind11_bazel releases were inaugurated a couple of months ago entirely for the sake of BCR. Unless you use Bzlmod, there's no strong reason to wait for a release as such.

@rwgk
Copy link
Collaborator

rwgk commented Nov 14, 2023

Is the next pybind11 release very far off, @rwgk?

We don't have a timeline. The release schedule has been and still is rather ad hoc.

but that might be too inflexible in the face of bugs.

TBH I don't feel I have enough context, but a direct coupling via an artifact as arbitrary as a version number does sound troublesome in general.

What comes to mind: maybe a mechanism like requirements.txt is needed?

@junyer
Copy link
Contributor

junyer commented Nov 14, 2023

What comes to mind: maybe a mechanism like requirements.txt is needed?

My experience with requirements.txt is exactly zero, admittedly, but considering that we have just one Python dependency (i.e. pybind11 itself), I'm hesitant to integrate a separate mechanism. And given that we would presumably want to update the version automagically, a modest amount of simple Starlark seems unavoidable? I might very well be missing your point and/or overthinking the problem though. :)

@rwgk
Copy link
Collaborator

rwgk commented Nov 14, 2023

I might very well be missing your point and/or overthinking the problem though. :)

I don't think so: I was just tossing out the first thing that come to mind, as a start.

And given that we would presumably want to update the version automagically, a modest amount of simple Starlark seems unavoidable?

That sounds like a good direction.

@junyer
Copy link
Contributor

junyer commented Nov 15, 2023

As of commit c65db0a, we support .bzl.<N> suffixes for pybind11_bazel versions.

@shuyangsun, please let me know whether you do indeed require a release (2.11.1.bzl.1) to be cut. :)

@junyer
Copy link
Contributor

junyer commented Jan 17, 2024

As foretold by the prophecy, Bazel 7 broke pybind11_bazel on macOS, so I'm going to cut the 2.11.1.bzl.1 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants