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

Add test cases for Protobuf Editions #225

Merged
merged 5 commits into from
Jul 16, 2024
Merged

Conversation

jchadwick-buf
Copy link
Member

Whew! This one is a doozy.

The vast majority of the work is fairly mechanical. In order to test the gauntlet of new semantics that might occur as a result of editions, a new set of test cases are created that mostly mirror the existing tests that already differ between proto2 and proto3. These test cases cover the existing tests that cover edge cases, with the possible set of field presence variants (implicit, explicit, and legacy-required; note that not all of these are valid in all cases) and with both default and expanded repeated field representation.

Most of these changes are transparent or invisible even at the protoreflect level and mostly just ensure that protovalidate implementations are not tripping up over the wire format or resulting descriptors.

One wrinkle is that getting a new enough version of the protobuf toolchain in Bazel for protobuf editions support is tricky; with upstream rules_proto, the latest toolchain you get does not support editions at all. I ran into a couple of very tricky issues getting a newer toolchain to work, but thankfully the resulting changes to work around those issues are relatively simple:

  • It seems like the rules_python repo needs to be defined for protobuf v27+, but the protobuf_deps function does not define it. We must define it ourselves.

  • For unknown reasons, it is necessary to specify both --proto_compiler and --proto_toolchain_for_cc (and presumably other --proto_toolchain_for_* options). The values specified were actually supposedly the default values for these options, so it is unclear why they need to be specified. I suspect this has something to do with refactoring of Bazel's protobuf rules but didn't want to spend more time investigating, as it took a fair bit of time to figure out that it worked in the first place.

Copy link
Member

@rodaine rodaine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Herculean effort! Thanks @jchadwick-buf

@rodaine rodaine merged commit bb1d618 into bufbuild:main Jul 16, 2024
3 checks passed
@jchadwick-buf jchadwick-buf deleted the editions branch September 24, 2024 13:57
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