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

Bzlmod integration #269

Closed
jbgcarnes opened this issue Jul 11, 2023 · 16 comments
Closed

Bzlmod integration #269

jbgcarnes opened this issue Jul 11, 2023 · 16 comments
Labels
enhancement New feature or request

Comments

@jbgcarnes
Copy link

Description

I know it's been mentioned a few times tangentially in a few other issues, but as bzlmod's are going to be the standard in Bazel 7.0 (released later this year), it would be useful for the project to support bzlmod and potentially be on the bazel central repo.

Is there a roadmap or path forward in that direction for the project? And if so, what are some of the steps to getting it there (I might have a few cycles I can use to submit mrs)

@aaliddell aaliddell added the enhancement New feature or request label Jul 14, 2023
@aaliddell
Copy link
Member

aaliddell commented Jul 14, 2023

It's something that needs to happen eventually and would solve so many of the dependency tree problems, but the protobuf and grpc modules in the BCR are presently effectively unsupported by Google (IIRC there's an issue somewhere stating they don't have time to support it) and therefore have not kept up to date as new releases occur. So unfortunately at present we are better off not supporting bzlmod, as non-bzlmod protobuf/grpc is more officially supported. Additionally, some of the languages used here do not yet have stable rules modules in the BCR, so a number of languages will be dropped.

When bzlmod support is eventually added, this will be as v6 of rules_proto_grpc that drops support for non-bzlmod usage, with the existing v5 releases available for that use-case. Step 1 on this process will be a minimum viable module that maybe just supports a single language.

So, realistically we are waiting for Google's Bazel/bzlmod team to prod Google's protobuf/grpc teams into officially supporting bzlmod, as nobody external really has the power nor time to make these changes.

@jtcarnes
Copy link

Eloquently put, well thought out, reasonable and what I didn't want to hear lol. Makes total sense. I'll hold tight then. Thank you for the well reasoned answer!

@oh-tarnished
Copy link

Hey Guys, I just checked right now what are all missing so this project can move to bzlmod pretty much everything that we require are in place with proto and protobuf right now we can do partial transition to MODULE system. where we can use both WORKSPACE AND MODULE.

any plans ?

@aaliddell
Copy link
Member

Here's a small sample of the latest upstream issues that affect the viability of implementing bzlmod support here:

It looks like at present C++, Go and perhaps Python + Swift could be made to work under bzlmod, with the outdated protobuf & grpc versions presently in BCR. For other languages the support just isn't there yet

@aaliddell aaliddell pinned this issue Sep 19, 2023
@aaliddell
Copy link
Member

Some experimental bzlmod work is happening in the next branch. Only C++ and Python have been migrated as of today, but it works! I've already hit a number of issues related to the lack of proper upstream bzlmod support in the protobuf and grpc repos, which made Python in particular a bit of a hack. If you are using any of the languages that are available though, any feedback would always be welcome. Once there's a handful of the core languages working, I'll cut an alpha release so that it can be used directly with BCR.

BEWARE: There are sharp edges, things will break & the interface is by no means stable

@linzhp
Copy link
Contributor

linzhp commented Nov 30, 2023

Thanks for the work on the next branch. We (Uber) is using that on our Python monorepo. It works! Looking forward to seeing it in Bazel Central Repository.

@aaliddell
Copy link
Member

Good to hear! Hopefully there’s no more breaking changes before it lands in BCR alpha but can’t promise anything 🤞. I need grpc-gateway and Buf rules over the line as a minimum to issue the first release to cover requirements.

One thing to beware of with the grpc rules and python under bzlmod is that they don’t use the rules_python toolchains, so you may end up building the extension against the wrong python headers. This could be worked around under WORKSPACE but not under bzlmod, so needs grpc/grpc#24665. Until that is resolved you need your system Python to match your toolchain Python version

@linzhp
Copy link
Contributor

linzhp commented Nov 30, 2023

I assume you meant this:

Label(requirement("grpcio")),
# Label("@grpc//src/python/grpcio/grpc:grpcio"), # TODO: restore once grpc in BCR works with python

I actually prefer the pip package grpcio over @grpc//src/python/grpcio/grpc:grpcio, because the pip package is prebuilt, and generally works better due to the complexity of building C libraries from source. Also, the Python projects using importing grpcio will be resolved to the pip package by default, which comes with version resolution and lock in requirements.txt. If rules-proto-grpc uses @grpc//src/python/grpcio/grpc:grpcio, it means there will be two versions of grpcio in the same Python project, which can lead to unpredictable behavior.

@aaliddell
Copy link
Member

Oh I forgot I put that workaround in 🤣

The reason the @grpc version is preferred here is that the plugin used to generate the python sources comes from @grpc repo, as the raw plugin is not distributed in the pip wheel (only a wrapped protoc is bundled, which is useless here 😠). If you have a version skew between the pip dep and the bzlmod dep then I believe there’s no guarantee the generated python sources will work.

So we’re stuck in a position where neither the bzlmod dep nor the wheel provide both a working plugin and a working runtime…

@linzhp
Copy link
Contributor

linzhp commented Nov 30, 2023

Let me clarify: Python gRPC projects using rules-proto-grpc needs to depend on gRPC in two places:

  1. rules-proto-grpc needs the gRPC plugin to generate Python sources
  2. Python sources (both generated and hand-crafted) importing grpcio package need gRPC to build and run

grpc/grpc#24665 doesn't affect 2 when we use the pre-built grpcio pip package, right?

@aaliddell
Copy link
Member

Correct, but you need to match those two versions (in theory anyway; in practice it seems like you can get away with it sometimes since presumably not much changes in these files).

@linzhp
Copy link
Contributor

linzhp commented Nov 30, 2023

We ran into troubles in building grpcio from source but no trouble in build grpc plugin. So in another internal repo without bzlmod, we had to patch rules-proto-grpc so it uses the grpcio pip package.

@aaliddell
Copy link
Member

Ok, if the plugin was bundled directly in the grpcio wheel it’d solve all the problems of version skew and rebuild, but sadly getting that change done upstream would likely be an uphill battle

@linzhp
Copy link
Contributor

linzhp commented Nov 30, 2023

Actually, the plugin is published in a different wheel grpcio-tools. It's recommended by the official tutorial. grpcio-tools requires grpcio. So we can rely on pip to keep both in sync.

@aaliddell
Copy link
Member

The grpcio-tools wheel doesn't contain the plugin in a usable form, which is what I meant earlier about "only a wrapped protoc is bundled, which is useless here".

The problem is that wheel doesn't contain a standard protoc plugin that we can integrate in these rules but instead contains an entire separate implementation of the protoc compiler wrapping the grpc python plugin as a shared library extension. I can understand why they do this for user friendliness (by not needing your own protoc), but it is the only language that does this and does not integrate with any other tooling that expects a normal plugin (such as these rules). The plugin we need is buried within that .so, but last time I tried to convert it into a usable form it I gave up.

@aaliddell
Copy link
Member

I am going to cut an alpha release with languages in the current state they are in the next branch, which will end up closing this issue. I have added an issue tracking further language support here: #299

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

No branches or pull requests

5 participants