protobuf icon indicating copy to clipboard operation
protobuf copied to clipboard

In Ruby, support each without block on repeated fields

Open shaldengeki opened this issue 2 years ago • 17 comments

Does what it says on the tin. ~Much to my pleasant surprise, this test currently passes (at least, it does on my machine)!~

Fixes https://github.com/protocolbuffers/protobuf/issues/5999

shaldengeki avatar Dec 12 '23 03:12 shaldengeki

Oh, interesting. The test isn't running because it's out of testing scope -- looks like an adjacent test is also out of scope, which is where I went astray. I think this test currently fails if you make it actually run:

https://github.com/protocolbuffers/protobuf/pull/9677/files#diff-e247eaaa827af2292a6e18f804080f5d57c734e965436bbaccf8a3a76bb4a940R717

cc @JasonLunn let me know if I'm wildly off-base here, totally possible

shaldengeki avatar Dec 12 '23 04:12 shaldengeki

Hi shaldengeki, do you plan to make these tests run (and fix them if they need fixing) in this PR, or in a separte one? Should we convert this one to draft in the meantime? Thanks for clarification :)

hlopko avatar Dec 12 '23 13:12 hlopko

Hi @hlopko! I want to make sure I'm not missing anything first - it's been awhile since I contributed here. If someone can confirm that what I'm saying makes sense, I think I'd prefer to open a separate PR to try and fix the regression test for #9202, and keep this PR centered on #5999.

shaldengeki avatar Dec 12 '23 14:12 shaldengeki

Great catch, @shaldengeki - that test does look like it is out of place and isn't getting run as it should be.

JasonLunn avatar Dec 12 '23 15:12 JasonLunn

Thanks for confirming @JasonLunn!

I'll convert this to a draft -- looks like the FFI tests are failing and I have to figure out how to run those locally.

shaldengeki avatar Dec 12 '23 17:12 shaldengeki

@hlopko Mind running the tests on this? When I run the FFI tests locally I see that the basic tests pass, but I get a segfault in the GC tests when importing generated code, and I want to rule out a local issue.

shaldengeki avatar Dec 13 '23 01:12 shaldengeki

Hmmm, interesting. So, this is failing in Github:

FAIL: //ruby/compatibility_tests/v3.0.0/tests:basic (see /workspace/_build/out/execroot/com_google_protobuf/bazel-out/k8-fastbuild/testlogs/ruby/compatibility_tests/v3.0.0/tests/basic/test.log)
INFO: From Testing //ruby/compatibility_tests/v3.0.0/tests:basic:
==================== Test output for //ruby/compatibility_tests/v3.0.0/tests:basic:
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.3/lib/power_assert.rb:8: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
/workspace/_build/out/external/protobuf_bundle/lib/jruby/3.1.0/gems/power_assert-2.0.3/lib/power_assert.rb:8: warning: tracing (e.g. set_trace_func) will not capture all events without --debug flag
Error: Your application used more stack memory than the safety cap of 2048K.
Specify -J-Xss####k to increase it (#### = cap size in KB).
Specify -w for full java.lang.StackOverflowError stack trace
================================================================================

But this passes for me locally, even when I copy the Bazel invocation used in that action:

~/protobuf$ bazel test //ruby/compatibility_tests/v3.0.0/tests:basic //ruby/tests:ruby_version --test_env=KOKORO_RUBY_VERSION --test_env=BAZEL=true --//ruby:ffi=enabled --test_env=PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=FFI --keep_going --test_output=errors --test_timeout=600

INFO: Found 2 test targets...
INFO: Elapsed time: 296.151s, Critical Path: 62.97s
INFO: 1130 processes: 487 internal, 643 linux-sandbox.
INFO: Build completed successfully, 1130 total actions
//ruby/compatibility_tests/v3.0.0/tests:basic                            PASSED in 0.7s
//ruby/tests:ruby_version                                                PASSED in 0.2s

Executed 2 out of 2 tests: 2 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

I.... wonder if I need to bump the test size?

shaldengeki avatar Dec 13 '23 01:12 shaldengeki

@JasonLunn WDYT?

hlopko avatar Dec 15 '23 14:12 hlopko

@shaldengeki - the tests that are stack overflowing are doing so under JRuby.

If you've bazel to build the tests on your local under MRI previously, you likely need run bazel clean --expunge, then use RVM to select to your JRuby version, then re-run your bazel test invocation.

JasonLunn avatar Dec 15 '23 21:12 JasonLunn

Thanks @JasonLunn (and @hlopko)! Can't believe I missed that this was a JRuby thing.

I'm having difficulty reproducing the error. First, I bazel clean --expunged, then I installed JRuby using rvm and selected it:

charles@AlterCation:~/protobuf$ ruby --version
jruby 9.4.6.0-SNAPSHOT (3.1.4) 2023-12-16 627d8d1667 OpenJDK 64-Bit Server VM 11.0.18+10-LTS on 11.0.18+10-LTS +jit [x86_64-linux]

Then I reran my test invocation, which still passed:

bazel test //ruby/compatibility_tests/v3.0.0/tests:basic //ruby/tests:ruby_version --test_env=KOKORO_RUBY_VERSION --test_env=BAZEL=true --//ruby:ffi=enabled --test_env=PROTOCOL_BUFFERS_RUBY_IMPLEMENTATION=FFI --keep_going --test_output=errors --test_timeout=600
# ...
INFO: Found 2 test targets...
INFO: Elapsed time: 198.274s, Critical Path: 50.68s
INFO: 1180 processes: 502 internal, 671 linux-sandbox, 7 worker.
INFO: Build completed successfully, 1180 total actions
//ruby/compatibility_tests/v3.0.0/tests:basic                            PASSED in 6.8s
//ruby/tests:ruby_version                                                PASSED in 4.9s

Executed 2 out of 2 tests: 2 tests pass.
There were tests whose specified size is too big. Use the --test_verbose_timeout_warnings command line option to see which ones these are.

For what it's worth, I'm on an older merge-base, to work around https://github.com/protocolbuffers/protobuf/issues/15030. I can try switching to another platform, but if you've got a moment and want to try reproducing this error on my branch, I'd be much obliged.

shaldengeki avatar Dec 16 '23 00:12 shaldengeki

Aha, no need -- I'm able to reproduce the StackOverflowError on a different machine; will look into it, and put this in draft status while I figure it out.

shaldengeki avatar Dec 17 '23 06:12 shaldengeki

I saw that we removed the 3.0.0 compatibility tests from main (which this PR was failing on), so I pulled main into this. Mind if we rerun the tests to see if they pass now?

shaldengeki avatar Dec 29 '23 05:12 shaldengeki

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

github-actions[bot] avatar Mar 28 '24 10:03 github-actions[bot]

@shaldengeki - would you mind merging main into this branch one more time?

JasonLunn avatar Mar 28 '24 13:03 JasonLunn

All of the vanilla Ruby tests are now passing, but the JRuby tests now have a conformance failure and the FFI variants have a stack overflow.

Are you able to run either of those tests configurations locally?

JasonLunn avatar Mar 29 '24 14:03 JasonLunn

I can reproduce both errors locally! Appreciate it; let me take a look at this and get back to you.

shaldengeki avatar Mar 29 '24 15:03 shaldengeki

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active, please add a comment.

This PR is labeled inactive because the last activity was over 90 days ago. This PR will be closed and archived after 14 additional days without activity.

github-actions[bot] avatar Jun 28 '24 10:06 github-actions[bot]

We triage inactive PRs and issues in order to make it easier to find active work. If this PR should remain active or becomes active again, please reopen it.

This PR was closed and archived because there has been no new activity in the 14 days since the inactive label was added.

github-actions[bot] avatar Jul 12 '24 10:07 github-actions[bot]