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.

Sign up for GitHub

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

Jump to bottom

Add to_proto to descriptor classes #19971

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Add to_proto to descriptor classes #19971

wants to merge 12 commits into from
+1,189 −12

Conversation

y-yagi
Copy link
Contributor

@y-yagi y-yagi commented Jan 12, 2025 •
edited
Loading

Some APIs (e.g. BigQuery Storage) use DescriptorProto for schema descriptions. But, in Ruby, there are no methods for that so far.

This PR implements the method for that(to_proto) to descriptor classes.

Fixes #12044.

@y-yagi y-yagi requested review from a team as code owners January 12, 2025 06:20
@y-yagi y-yagi requested review from JasonLunn and bshaffer and removed request for a team January 12, 2025 06:20
@mkruskal-google mkruskal-google added ruby jruby Issues unique to the JRuby interpreter 🅰️ safe for tests Mark a commit as safe to run presubmits over labels Jan 23, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 23, 2025
@mkruskal-google mkruskal-google removed the request for review from bshaffer January 23, 2025 20:15
@y-yagi y-yagi force-pushed the add_to_proto_to_descriptors branch from 9f2cd99 to 964ec01 Compare January 24, 2025 02:00
@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 24, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 24, 2025
@JasonLunn
Copy link
Contributor

Thanks for contributing this PR, @y-yagi!

The tests are really close to passing, but there seems to be an issue on macOS. Both the Ruby 3.0 and Ruby 3.4 tests are failing on macOS with a failure like this:

Error: test_enum_descriptor_to_proto(BasicTest::MessageContainerTest): Google::Protobuf::ParseError: Error occurred during parsing
/private/var/tmp/_bazel_runner/88067fd1245450c0f2a417255e46b72b/sandbox/darwin-sandbox/1336/execroot/com_google_protobuf/bazel-out/darwin_x86_64-fastbuild/bin/ruby/tests/basic.runfiles/com_google_protobuf/ruby/tests/basic.rb:701:in 'Google::Protobuf::EnumDescriptor#to_proto'
/private/var/tmp/_bazel_runner/88067fd1245450c0f2a417255e46b72b/sandbox/darwin-sandbox/1336/execroot/com_google_protobuf/bazel-out/darwin_x86_64-fastbuild/bin/ruby/tests/basic.runfiles/com_google_protobuf/ruby/tests/basic.rb:701:in 'BasicTest::MessageContainerTest#test_enum_descriptor_to_proto'
     698:     def test_enum_descriptor_to_proto
     699:       enum_descriptor = TestDeprecatedEnum.descriptor
     700: 
  => 701:       assert_instance_of Google::Protobuf::EnumDescriptorProto, enum_descriptor.to_proto
     702:     end
     703:

The same tests are passing on Linux and under FFI on the macOS, and the other descriptor tests are working as expected - it is just enum descriptor test without FFI that is having an issue. I'm going to re-run the tests now to see if the test failures are stable or if the tests are flaky.

Do you have a macOS machine available to troubleshoot this on in the meantime?

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 24, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 24, 2025
JasonLunn
JasonLunn requested changes Jan 24, 2025
View reviewed changes
ruby/ext/google/protobuf_c/defs.c Outdated
const char* serialized = google_protobuf_FileDescriptorProto_serialize(
file_proto, arena, &size);

upb_Arena_Free(arena);
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking across the different implementations for the different descriptor types, some free the newly created arena and some don't.
Is there a reason not to handle them uniformly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was just my miss 🙏. I fixed to free an Area in all functions.
a35588e

ruby/ext/google/protobuf_c/defs.c
self->enumdef, arena);

size_t size;
const char* serialized = google_protobuf_EnumDescriptorProto_serialize(
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the error we're seeing in CI is a parse error, it might be worth being more defensive here and validating that size is non-zero and that serialized isn't null before calling Message_decode_bytes().

Copy link
Contributor Author

@y-yagi y-yagi Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for your review!

I don't have a macOS so I can't reproduce this issue on my local...

I have added the validation for size and serialized for confirmation. 51e54e3
Could you rerun the CI?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm. The enum descriptor test is still getting Google::Protobuf::ParseError. And, it seems that other tests are getting the same error. I will check.

@JasonLunn JasonLunn added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 26, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 26, 2025
@y-yagi y-yagi force-pushed the add_to_proto_to_descriptors branch from 51e54e3 to 1afd9e8 Compare January 27, 2025 07:40
@y-yagi
Copy link
Contributor Author

y-yagi commented Jan 27, 2025

I'm doubting I released an arena too early. So I fixed the timing of it. 1afd9e8

@acozzette acozzette added the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 30, 2025
@github-actions github-actions bot removed the 🅰️ safe for tests Mark a commit as safe to run presubmits over label Jan 30, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkruskal-google mkruskal-google mkruskal-google left review comments

@JasonLunn JasonLunn JasonLunn requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
jruby Issues unique to the JRuby interpreter ruby
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Ruby: Convert Descriptor to DescriptorProto
4 participants
@y-yagi @JasonLunn @mkruskal-google @acozzette