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

Roll forward Ruby upb changes now that protobuf Ruby build is fixed #5866

Merged
merged 24 commits into from Aug 14, 2019

Conversation

haberman
Copy link
Member

@haberman haberman commented Mar 11, 2019

Updates the Ruby protobuf library to a new version of upb, the C library used for proto parsing. This removes some APIs that allowed setting fields directly on descriptor objects, but these were internal-only. Any code generated by the protocol compiler, and any code that uses the Ruby/Protobuf DSL should continue to work.

Revert "Revert "Updated upb from defcleanup branch and modified Ruby to use it (protocolbuffers#5539)" (protocolbuffers#5848)"

This reverts commit 1568dea.
@TeBoring
Copy link
Contributor

Any update?

@haberman
Copy link
Member Author

There is a sporadic test failure in the parallelism test. It's very hard to reproduce locally, and I haven't figured out what is causing it yet.

haberman added 11 commits May 5, 2019 16:00
Some of our Kokoro tests seem to run with this level of warnings,
and the source strives to be gnu90 compatible.  Enforcing it for
every build removes the possibility of some errors showing up in
Kokoro/Travis tests only.
I tried to match warning flags with what Ruby appears to do
in our Kokoro tests.
We need to initialize this marked value before creating the instance.
@haberman
Copy link
Member Author

@TeBoring This CL should be ready with the exception of the macOS Timestamp parsing stuff.

@TeBoring
Copy link
Contributor

Could you also merge the fix for Timestamp in this change?

@haberman
Copy link
Member Author

haberman commented May 21, 2019

All tests are passing, but this seems to still be an intermittent, hard-to-reproduce error:

..F
===============================================================================
Failure: test_concurrent_decoding(BasicTest::MessageContainerTest)
/tmp/protobuf/protobuf/ruby/tests/basic.rb:228:in `block (3 levels) in test_concurrent_decoding'
     225:       thds = 2.times.map do
     226:         Thread.new do
     227:           100000.times do
  => 228:             assert_equal o, Outer.decode(raw)
     229:           end
     230:         end
     231:       end
/tmp/protobuf/protobuf/ruby/tests/basic.rb:227:in `times'
/tmp/protobuf/protobuf/ruby/tests/basic.rb:227:in `block (2 levels) in test_concurrent_decoding'
<<BasicTest::Outer: items: {0=><BasicTest::Inner: >}>> expected but was
<<BasicTest::Outer: items: {}>>
diff:
? <BasicTest::Outer: items: {0=><BasicTest::Inner: >}>

I can't figure out if this is a new problem introduced by this change or if it existed previously.

I've tried the following in an attempt to reproduce the bug locally:

  • I compiled a custom Ruby that uses 1ms timeslices instead of 100ms
  • I've increased the number of map entries to 1,000
  • I set GC.stress = 0x01 | 0x04 like gc_test.rb

None of these strategies has reproduced the problem.

@@ -17,7 +17,6 @@ module BasicTest
add_message "BadFieldNames" do
optional :dup, :int32, 1
optional :class, :int32, 2
optional :"a.b", :int32, 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this is not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's an illegal field name. This is checked more robustly now, so the add_message call itself will fail now.

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.

None yet

4 participants