feat: Upgrade to protobuf 27.0 and remove py_proto_library #1933

comius · 2024-05-31T12:21:41Z

Protobuf team is taking ownership of py_proto_library and the implementation was moved to protobuf repository.

Remove py_proto_library from rules_python, to prevent divergent implementations.

Make a redirect with a deprecation warning, so that this doesn't break any users.

Expected side effect of this change is also that the protobuf version is sufficiently updated that there is no more use of legacy struct providers.

Closes #1935
Closes #1924
Closes #1925
Closes #1703
Closes #1707
Closes #1597
Closes #1293
Closes #1080
Fixes #1438

rickeylev

Please also add an entry to CHANGELOG.md to note that the rules_python proto rules are deprecated and will be removed in a future release.

CI has a failure when compiling protos. My guess would be the those builds don't have the additional compiler flags that were added. But, this would imply that all users also have to add those flags? That doesn't seem feasible. Can protobuf add those compile flags instead itself?

rickeylev · 2024-05-31T16:08:26Z

examples/bzlmod/py_proto_library/example.com/proto/BUILD.bazel

-load("@rules_proto//proto:defs.bzl", "proto_library")
-load("@rules_python//python:proto.bzl", "py_proto_library")
+load("@com_google_protobuf//bazel:proto_library.bzl", "proto_library")
+load("@com_google_protobuf//bazel:py_proto_library.bzl", "py_proto_library")


The python rules are in the protobuf repo directly, not in a python-specific repo? I thought I remember seeing a BCR PR where they were adding various proto_{java,python,etc} repos so e.g. java wasn't pulled in when only python was used.

The python rules are in the protobuf repo directly, not in a python-specific repo?
Yes.

BCR PR where they were adding various proto_{java,python,etc} repos

Hm, do you mean rules_proto_grpc_*? I imagine those support gRPC, but I wasn't part of that effort.

rickeylev · 2024-05-31T16:09:36Z

internal_setup.bzl

@@ -35,9 +34,6 @@ def rules_python_internal_setup():

    bazel_skylib_workspace()

-    rules_proto_dependencies()


For workspace builds, shouldn't there be some sort of proto-setup call still? Did I miss where it is?

There's a protobuf_deps call just after. We only need to setup protobuf, now that the rules are moving there.

aignas · 2024-06-02T06:17:56Z

I've referenced a lot of issues that have to do with proto library rule, feel free to disagree and alter the list if some of them are out of place.

comius · 2024-06-03T12:08:20Z

Please also add an entry to CHANGELOG.md to note that the rules_python proto rules are deprecated and will be removed in a future release.

Done.

CI has a failure when compiling protos. My guess would be the those builds don't have the additional compiler flags that were added. But, this would imply that all users also have to add those flags? That doesn't seem feasible. Can protobuf add those compile flags instead itself?

I see 3 possible options:

adding the flags
fixing the code so that there are no problems
using precompiled binaries

The last option is something that will be done in protobuf. It was done previously in rules_proto, but in an awkward and unmaintained way.

aignas · 2024-06-04T00:40:24Z

@comius, when are you going to work on using the precompiled binaries? I wonder if we should merge this PR after that part is fixed.

comius · 2024-06-05T10:18:01Z

@comius, when are you going to work on using the precompiled binaries? I wonder if we should merge this PR after that part is fixed.

I hope we don't have to wait, because this PR also removes dependency on extra old protobuf that uses legacy struct providers. I'd like to disable that support in Bazel and that would then break rules_python.

Old py_proto_library is getting precompiled protoc from a really old version of rules_proto, which was never properly maintained.

alexeagle · 2024-06-07T20:48:59Z

Note that due to protocolbuffers/protobuf#17075 this caused a regression - the latest rules_python release has a py_proto_library that can be used with a prebuilt toolchain to avoid compiling protoc, but the one in protocolbuffers/protobuf does not. Thus users with a non-functional cc toolchain will be stuck on the last rules_python release.

@alexeagle

…ild#1933) This reverts commit d0e25cf. We have the following concerns with the associated change: - `sphinxdocs` is excluded entirely. Because protobuf fails to compile? But why? Protos are needed as part of our docgen, but it is unclear how the docgen is still working. Maybe we can get some clarification here? - The `py_proto_library` tests in the `bzlmod` example are excluded from CI. Because protos also fail to compile? But why? It's an example and should Just Work. - Adding the `copts` needs to be done by all downstream users now. But fixing that in protobuf blocks removing legacy struct providers? I don't understand the connection. Also @alexeagle noted extra [regression](bazelbuild#1933 (comment)) being caused by the associated PR. Reverting in order to unblock a new release of `rules_python` and then we can work together with @comius on reverting the revert.

rickeylev · 2024-06-11T05:29:58Z

@comius FYI we're going to revert this PR for now.

The main reason is what @alexeagle is reporting -- that users will be stuck on a release without a way to upgrade. Breaking changes are OK, but have to allow a graceful upgrade path; see https://rules-python.readthedocs.io/en/latest/contributing.html#breaking-changes

From the issue Alex filed, it sounds like part of the problem here is sequencing of releases? That this PR to be released after something else in proto-something is available?

That said, there are several other changes in this PR I have questions about:

It looks like building of doc-related protos was disabled in CI, with a comment saying protobuf compilation fails? Our docgen relies on protos, though.
The py_proto_library bzlmod example was also disabled -- this is partially a test that it works under bzlmod. If its broken, then something seems very wrong.
You said fixing the need to add the copt flags would block removing legacy struct providers? I don't see the connection.

@alexeagle

…1948) This reverts commit d0e25cf. We have the following concerns with the associated change: - `sphinxdocs` is excluded entirely. Because protobuf fails to compile? But why? Protos are needed as part of our docgen, but it is unclear how the docgen is still working. Maybe we can get some clarification here? - The `py_proto_library` tests in the `bzlmod` example are excluded from CI. Because protos also fail to compile? But why? It's an example and should Just Work. - Adding the `copts` needs to be done by all downstream users now. But fixing that in protobuf blocks removing legacy struct providers? I don't understand the connection. Also @alexeagle noted extra [regression](#1933 (comment)) being caused by the associated PR. Reverting in order to unblock a new release of `rules_python` and then we can work together with @comius on reverting the revert. Reverts #1933

comius · 2024-06-11T10:00:31Z

@comius FYI we're going to revert this PR for now.

The main reason is what @alexeagle is reporting -- that users will be stuck on a release without a way to upgrade. Breaking changes are OK, but have to allow a graceful upgrade path; see https://rules-python.readthedocs.io/en/latest/contributing.html#breaking-changes

From the issue Alex filed, it sounds like part of the problem here is sequencing of releases? That this PR to be released after something else in proto-something is available?

That said, there are several other changes in this PR I have questions about:

It looks like building of doc-related protos was disabled in CI, with a comment saying protobuf compilation fails? Our docgen relies on protos, though.

It was disabled only on some platforms with GCC. Not on all of them.

The py_proto_library bzlmod example was also disabled -- this is partially a test that it works under bzlmod. If its broken, then something seems very wrong.

Same here.

You said fixing the need to add the copt flags would block removing legacy struct providers? I don't see the connection.

Try running rules_python build with --incompatible_disallow_struct_provider_syntax and the connection will become clear. Struct providers are used in an extra old protoc, that's coming from rules_proto.

alexeagle · 2024-06-11T13:53:05Z

I'll try to clarify:
I've been working on the rollout of Ivo's "toolchainization" of proto tools (the protoc and any language-specific plugins. We have to work through a large number of hard-coded @com_google_protobuf//:protoc references (to a cc_binary) which are listed here bazelbuild/rules_proto#213

We announced that Python is working, but the fork of py_proto_library to the protobuf repo occurred at an earlier commit and lost the fix. My fix for that has just landed: protocolbuffers/protobuf#17078

With my devrel hat on, I'd like to see that fix released by protobuf before rules_python drops the rule so this feature doesn't temporarily regress - it's hard to explain to users "yeah it was fixed, but then it was unfixed, and you're on this one release of rules_python in that range, sorry footgun..."

rickeylev · 2024-06-11T18:58:58Z

Thanks @alexeagle, @comius

So if I understand correctly, we just have to wait for the next protobuf release, and then can re-merge this change?

Will the protobuf fix also address having to add the copt flag and the various CI config changes that were necessary?

Is that it, or should something also be done in the meantime? (opt-in env var? bump rules_proto or protobuf version past 21 but before 27? idk).

alexeagle · 2024-06-11T19:22:12Z

aspect-build/toolchains_protoc#13 demonstrates the breakage caused by the PR, for users who don't have cc toolchain.

just have to wait for the next protobuf release, and then can re-merge this change

Yes, next protobuf release fixes the py_proto_library rule so I that's what I would prefer. I know @comius is now blocked on some other changes downstream of this one, so I wont object to a roll-forward as soon as you like.

having to add the copt flag and the various CI config changes

I don't have context on that, and don't care about their compiler options for building protoc from source since I don't do that. It seems like they break it as often as they fix it.

Reintroduce the changes in #1933, which got reverted in commit 8b0eaed (#1948).

aignas · 2024-06-12T02:23:41Z

Created a new PR and added all of you as reviewers: #1951.

comius added 6 commits May 31, 2024 14:18

Upgrade to protobuf 27 and remove py_proto_library

ab38839

Add missing sha

bd739d0

Fix for bazel_tools, using com_google_protobuf

3ab7065

One more com_google_protobuf

fa778f3

Remove reference to py_proto_library

1617b19

Disable warnings as errors for Debian/gcc distributions

0e179b2

comius marked this pull request as ready for review May 31, 2024 14:20

comius requested a review from rickeylev as a code owner May 31, 2024 14:20

comius added 2 commits May 31, 2024 16:32

Disable warnings as errors for Debian/gcc distributions for tests

eae747a

Fix identation

715018c

rickeylev reviewed May 31, 2024

View reviewed changes

comius added 11 commits May 31, 2024 18:26

Try disabling warnings as errors

089e52a

Try disabling targets

9936111

Disabling targets, try 2

01e7076

try 3

9d874fb

Try 4

fc3b171

try 6

2ad5e14

try 5

a7235f1

try 7

876c0d4

try 8

ad24d39

try 9

1e1d9d6

try 10

8adef63

comius requested a review from rickeylev May 31, 2024 21:21

aignas mentioned this pull request Jun 2, 2024

Unable to get bazel 7 py_proto_library example working outside of rules_python dir #1935

Closed

aignas approved these changes Jun 2, 2024

View reviewed changes

Add change to changelog

a055900

aignas added this pull request to the merge queue Jun 5, 2024

Merged via the queue into bazelbuild:main with commit d0e25cf Jun 5, 2024
4 checks passed

michael-christen mentioned this pull request Jun 5, 2024

[Gazelle][proto] How to automatically resolve protobuf target #1703

Open

aignas mentioned this pull request Jun 11, 2024

revert: upgrade to protobuf 27.0 and remove py_proto_library (#1933) #1948

Merged

aignas added a commit that referenced this pull request Jun 12, 2024

feat: upgrade to protobuf 27.0 and remove py_proto_library (#1933)

dd08165

Reintroduce the changes in #1933, which got reverted in commit 8b0eaed (#1948).

aignas mentioned this pull request Jun 12, 2024

feat: upgrade to protobuf 27.0 and remove py_proto_library (#1933) #1951

Closed

XuanWang-Amos mentioned this pull request Sep 5, 2024

Align py_proto_library() rule with the ones from protobuf and rules_python grpc/grpc#37515

Open

aignas mentioned this pull request Sep 7, 2024

Cleanup proto rules and point to the source of truth #2173

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Upgrade to protobuf 27.0 and remove py_proto_library #1933

feat: Upgrade to protobuf 27.0 and remove py_proto_library #1933

comius commented May 31, 2024 •

edited by aignas

Loading

rickeylev left a comment

rickeylev May 31, 2024

comius May 31, 2024

rickeylev May 31, 2024

comius May 31, 2024

aignas commented Jun 2, 2024

comius commented Jun 3, 2024

aignas commented Jun 4, 2024

comius commented Jun 5, 2024

alexeagle commented Jun 7, 2024

rickeylev commented Jun 11, 2024

comius commented Jun 11, 2024 •

edited

Loading

alexeagle commented Jun 11, 2024

rickeylev commented Jun 11, 2024

alexeagle commented Jun 11, 2024

aignas commented Jun 12, 2024

		@@ -35,9 +34,6 @@ def rules_python_internal_setup():

		bazel_skylib_workspace()

		rules_proto_dependencies()

feat: Upgrade to protobuf 27.0 and remove py_proto_library #1933

feat: Upgrade to protobuf 27.0 and remove py_proto_library #1933

Conversation

comius commented May 31, 2024 • edited by aignas Loading

rickeylev left a comment

Choose a reason for hiding this comment

rickeylev May 31, 2024

Choose a reason for hiding this comment

comius May 31, 2024

Choose a reason for hiding this comment

rickeylev May 31, 2024

Choose a reason for hiding this comment

comius May 31, 2024

Choose a reason for hiding this comment

aignas commented Jun 2, 2024

comius commented Jun 3, 2024

aignas commented Jun 4, 2024

comius commented Jun 5, 2024

alexeagle commented Jun 7, 2024

rickeylev commented Jun 11, 2024

comius commented Jun 11, 2024 • edited Loading

alexeagle commented Jun 11, 2024

rickeylev commented Jun 11, 2024

alexeagle commented Jun 11, 2024

aignas commented Jun 12, 2024

comius commented May 31, 2024 •

edited by aignas

Loading

comius commented Jun 11, 2024 •

edited

Loading