Skip to content

[bazel]: de-alias pybind11 headers target #79676

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

Merged
merged 1 commit into from
Jan 27, 2024

Conversation

j2kun
Copy link
Contributor

@j2kun j2kun commented Jan 27, 2024

In trying to set up python headers in an out-of-tree bazel MLIR project, I encountered the pybind11_bazel project, and found that the @python_runtime target used here is not defined by it.

Instead, it seems that @python_runtime is an alias used in some projects like Tensorflow (see
https://github.com/tensorflow/tensorflow/blob/322936ffdd96ee59e27d028467fe458859cf3855/third_party/python_runtime/BUILD#L7-L7), where it is aliased to @local_config_python. In fact, @local_config_python is defined by @pybind11_bazel, and so it seems that this layer of indirection no longer serves a purpose, and instead just prevents anyone who doesn't clone Tensorflow's config from using the python bindings here.

This commit updates the dependent targets to their canonical de-aliased equivalents, and I suspect this will not even break any downstream users since the new target is defined in those projects already.

Without this change, running, for example

bazel build @llvm-project//mlir:MLIRBindingsPythonCore

gives the error

no such package '@python_runtime//': The repository '@python_runtime'
could not be resolved: Repository '@python_runtime' is not defined and
referenced by '@llvm-project//mlir:MLIRBindingsPythonCore'

Minimal reproduction in https://github.com/j2kun/test_mlir_bazel_pybind, which, when pointing to a local LLVM repository that has this change (see bazel/import_llvm.bzl in that repository), results in that build succeeding.

Hat tip to Maksim Levental for going on an hours-long investigation with me to figure this out.

In trying to set up python headers in an out-of-tree bazel MLIR project,
I encountered the `pybind11_bazel` project, and found that the
`@python_runtime` target used here is not defined by it.

Instead, it seems that `@python_runtime` is an alias used in some
projects like Tensorflow (see
https://github.com/tensorflow/tensorflow/blob/322936ffdd96ee59e27d028467fe458859cf3855/third_party/python_runtime/BUILD#L7-L7),
where it is aliased to `@local_config_python`. In fact,
`@local_config_python` is defined by `@pybind11_bazel`, and so it seems
that this layer of indirection no longer serves a purpose, and instead
just prevents anyone who doesn't clone Tensorflow's config from using
the python bindings here.

This commit updates the dependent targets to their canonical de-aliased
equivalents, and I suspect this will not even break any downstream users
since the new target is defined in those projects already.

Without this change, running, for example

```
bazel build @llvm-project//mlir:MLIRBindingsPythonCore
```

gives the error

```
no such package '@python_runtime//': The repository '@python_runtime'
could not be resolved: Repository '@python_runtime' is not defined and
referenced by '@llvm-project//mlir:MLIRBindingsPythonCore'
```

Minimal reproduction in https://github.com/j2kun/test_mlir_bazel_pybind,
which, when pointing to a local LLVM repository that has this change
(see `bazel/import_llvm.bzl` in that repository), results in that build
succeeding.

Hat tip to Maksim Levental for going on an hours-long investigation with
me to figure this out.
@makslevental
Copy link
Contributor

I hereby do solemnly swear that I witnessed all the bazel insanity first-hand and this fix makes sense to me 🤚.

@makslevental makslevental self-requested a review January 27, 2024 05:26
@makslevental makslevental merged commit 5585ddd into llvm:main Jan 27, 2024
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 this pull request may close these issues.

2 participants