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

cucumber-messages: Use pure-Ruby protobuf implementation (experimental) #813

Merged
merged 12 commits into from
Jan 8, 2020

Conversation

mvz
Copy link
Contributor

@mvz mvz commented Nov 29, 2019

Summary

Replace google-protobuf gem with protobuf, a pure-Ruby implementation that should work on all Ruby platforms.

Details

  • Replace google-protobuf dependency with protobuf.
  • Use protobuf gem's custom generator.
  • Update messages.proto to make the generator emit Cucumber::Messages instead of Io::Cucumber::Messages. This is a bit of a hack. The protobuf gem should use the ruby_package option.

Motivation and Context

JRuby support is lacking in google-protobuf, and the signs are not encouraging that this will get better soon.

How Has This Been Tested?

I ran the specs, and I have backported this change to cucumber-messages 6.0.x and tested it with cucumber 4.0.0.pre.3.

Types of changes

  • Bug fix (non-breaking change which fixes an issue).
  • New feature (non-breaking change which adds functionality).
  • Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

  • The change has been ported to Java.
  • The change has been ported to Ruby.
  • The change has been ported to JavaScript.
  • The change has been ported to Go.
  • The change has been ported to .NET.
  • I've added tests for my code.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Still to do

  • Update the Ndjson specs to actually check the generated json instead of just round-tripping.
  • Update protobuf to honor the ruby_package option.
  • Fix the build on CircleCI.

@mvz mvz changed the title cucumber-messages: Use pure-Ruby protobuf implementation (experimental, draft) cucumber-messages: Use pure-Ruby protobuf implementation (experimental) Nov 29, 2019
Copy link
Contributor

@aslakhellesoy aslakhellesoy left a comment

Choose a reason for hiding this comment

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

This looks promising! The package states it's protobuf 2.x while the rest of the monorepo uses protobuf 3.x. If the build is green I am fine with this - I think the wire format for 2 and 3 are the same.

cucumber-messages/ruby/Rakefile Outdated Show resolved Hide resolved
cucumber-messages/ruby/messages.proto Show resolved Hide resolved
@aslakhellesoy
Copy link
Contributor

This bug must be fixed before we can use the native ruby protobuf library: ruby-protobuf/protobuf#408

@aslakhellesoy
Copy link
Contributor

I've submitted a PR to fix the issue in ruby-protobuf: ruby-protobuf/protobuf#408

@aslakhellesoy
Copy link
Contributor

I've also submitted ruby-protobuf/protobuf#411 - another fix that is required

@mvz
Copy link
Contributor Author

mvz commented Jan 8, 2020

Thanks for working on this, @aslakhellesoy.

@aslakhellesoy aslakhellesoy marked this pull request as ready for review January 8, 2020 10:18
@aslakhellesoy aslakhellesoy merged commit 0e74fb4 into master Jan 8, 2020
@aslakhellesoy aslakhellesoy deleted the cucumber-messages/use-pure-ruby-protobuf branch January 8, 2020 10:32
jackorp added a commit to fedora-distgit/rubygem-protobuf that referenced this pull request Mar 11, 2021
According to cucumber upstream, it should suffice.

link to the discussion: cucumber/common#813
jackorp added a commit to fedora-distgit/rubygem-protobuf that referenced this pull request Mar 12, 2021
…ssages.

According to cucumber upstream, it should suffice.

link to the discussion: cucumber/common#813
jackorp added a commit to fedora-distgit/rubygem-protobuf that referenced this pull request Mar 12, 2021
…ssages.

According to cucumber upstream, it should suffice.

link to the discussion: cucumber/common#813
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants