Skip to content

Rails2.7 segfaults#7091

Merged
haberman merged 5 commits intoprotocolbuffers:masterfrom
ewalk153:rails27-segfaults
Apr 17, 2020
Merged

Rails2.7 segfaults#7091
haberman merged 5 commits intoprotocolbuffers:masterfrom
ewalk153:rails27-segfaults

Conversation

@ewalk153
Copy link
Copy Markdown
Contributor

@ewalk153 ewalk153 commented Jan 14, 2020 •
edited
Loading

Following the script for ruby 2.6, this adds a build script line for ruby 2.7

Usage:
./tests.sh ruby27

Current failure is a segfault:

[snip]
../src/protoc -I../src -I. --ruby_out=tests ../src/google/protobuf/wrappers.proto
/Users/ericwalker/src/github.com/protocolbuffers/protobuf/ruby/tests/gc_test.rb:94: warning: assigned but unused variable - to
/Users/ericwalker/.rvm/rubies/ruby-2.7.0/lib/ruby/2.7.0/rubygems/core_ext/kernel_require.rb:72: [BUG] Segmentation fault at 0x0000000000000000
ruby 2.7.0p0 (2019-12-25 revision 647ee6f091) [x86_64-darwin19]
[snip]

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 14, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@ewalk153
Copy link
Copy Markdown
Contributor Author

ewalk153 commented Jan 14, 2020

@googlebot I signed it!

@googlebot
Copy link
Copy Markdown

googlebot commented Jan 14, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@ewalk153 ewalk153 mentioned this pull request Jan 14, 2020
Closed
ewalk153
ewalk153 commented Jan 14, 2020
View reviewed changes
Comment thread ruby/tests/repeated_field_test.rb Outdated
Copy link
Copy Markdown
Contributor Author

@ewalk153 ewalk153 Jan 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Not completely sure we can just ignore these methods, either way the segfault is more concerning at the moment

ewalk153
ewalk153 commented Jan 14, 2020
View reviewed changes
Comment thread ruby/tests/common_tests.rb Outdated
Copy link
Copy Markdown
Contributor Author

@ewalk153 ewalk153 Jan 14, 2020

Choose a reason for hiding this comment

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

exception message for FrozenError changed from Ruby 2.6 -> 2.7; it now include something like the inspect version of an object in the error:

2.7.0 :001 > [1, 2, 3].freeze << 4
Traceback (most recent call last):
        4: from /Users/ericwalker/.rvm/rubies/ruby-2.7.0/bin/irb:23:in `<main>'
        3: from /Users/ericwalker/.rvm/rubies/ruby-2.7.0/bin/irb:23:in `load'
        2: from /Users/ericwalker/.rvm/rubies/ruby-2.7.0/lib/ruby/gems/2.7.0/gems/irb-1.2.1/exe/irb:11:in `<top (required)>'
        1: from (irb):1
FrozenError (can't modify frozen Array: [1, 2, 3])

@acozzette acozzette requested a review from TeBoring January 17, 2020 23:48
@TeBoring
Copy link
Copy Markdown
Contributor

TeBoring commented Jan 20, 2020

What's the relation between #7091, #7085 and #7027?

@TeBoring TeBoring requested review from haberman and removed request for TeBoring January 20, 2020 21:48
@TeBoring TeBoring assigned haberman and unassigned TeBoring Jan 20, 2020
@ewalk153
Copy link
Copy Markdown
Contributor Author

ewalk153 commented Jan 20, 2020

(probably best to move this comment to a master issue, this is my read of things)
#7091 - find out what breaks (code and tests) and fix things when we run on Ruby 2.7. I ran this all locally.
#7085 & #7027 - update CI to run and support Ruby 2.7 builds

@sikachu
Copy link
Copy Markdown

sikachu commented Mar 17, 2020

Hello! Please excuse my intrusion to this PR, but we're planning to upgrade our application to Ruby 2.7 and stuck since we can't run the latest released version with Ruby 2.7.

I checked the failure (such as Linux Ruby 2.6) and noticed this line:

--2020-01-20 22:44:48--  http://repo1.maven.org/maven2/com/google/protobuf/protoc/3.0.0/protoc-3.0.0-linux-x86_64.exe
Resolving repo1.maven.org (repo1.maven.org)... 151.101.184.209
Connecting to repo1.maven.org (repo1.maven.org)|151.101.184.209|:80... connected.
HTTP request sent, awaiting response... 501 HTTPS Required

It seems like this problem was already fixed in 39f4240, but the branch in this PR doesn't have it, as I can confirm it here:

https://github.com/protocolbuffers/protobuf/blob/c4698b3c06f012e686f8d46c4871b3f527c99cdd/ruby/compatibility_tests/v3.0.0/test.sh#L13

@ewalk153, would you mind rebase this branch against master again so @TeBoring can re-trigger the build (and hopefully it'll pass this time 🤞)?

Thank you very much!

ewalk153 added 4 commits March 17, 2020 09:47
@ewalk153
Add a test suite for ruby 2.7
42a3f1d
@ewalk153
Call BigDecimal() instead of BigDecimal.new()
0edbb98 
BigDecimal.new was deprecated in ruby 2.6
@ewalk153
Switch FrozenError expectation to a matcher
47cadbe 
The error message for FrozenError changed to include more information
about the mutated object. Switch from an exact match to an aproximate
match (equal => match). This does not change the prefix.
@ewalk153
We can safely ignore newest array methods from ruby 2.7
974dbc7
@ewalk153 ewalk153 force-pushed the rails27-segfaults branch from c4698b3 to 974dbc7 Compare March 17, 2020 13:48
@ewalk153
Copy link
Copy Markdown
Contributor Author

ewalk153 commented Mar 17, 2020 •
edited
Loading

@sikachu I've rebased on master. Let me know if you want me to convert this draft PR to a normal one.

@sikachu
Copy link
Copy Markdown

sikachu commented Mar 23, 2020

I think we need project maintainer to trigger the build (add kokoro:run label to this PR?) to get things moving forward?

@ewalk153 ewalk153 marked this pull request as ready for review April 16, 2020 17:25
@ewalk153
Copy link
Copy Markdown
Contributor Author

ewalk153 commented Apr 16, 2020

@haberman apologies if this was held up by my not marking this 'ready for review'. Let me know if you need anything more from me to move forward.

@haberman haberman merged commit 2c8364b into protocolbuffers:master Apr 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@haberman haberman Awaiting requested review from haberman

Assignees

@haberman haberman

Labels

cla: yes ruby

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@ewalk153 @googlebot @TeBoring @sikachu @haberman @protobuf-kokoro