cucumber-messages: Add stricter gem restrictions for google-protobuf #578
Conversation
As the comments elude to; this gem is a key runtime dependency. And on Ruby 2.6 this only works when using the latest version For Running on JRuby and a couple of other environments, a weaker requirement is required (And not the latest), due to some issues with those environments Once the team maintaining this has done some additional work, we can probably relax this requirement. But for now this makes cucumber build properly again X-Ruby Versions
|
I'm aware we could dive into this and maybe experiment more with restrictions, but I'd rather just get this in. As it's blocking a whole heap of other PR's which have failing CI |
| if RbConfig::CONFIG["MINOR"] == "6" | ||
| s.add_dependency('google-protobuf', '~> 3.7') | ||
| else | ||
| s.add_dependency('google-protobuf', ['>= 3.2', '< 3.6']) |
There was a problem hiding this comment.
I remember this kind of logic not being included in the gem once it's built. Does this work now?
There was a problem hiding this comment.
I ran this locally on jruby 2.2 and 2.6 and it seemed to work.
That's not to say that's a rigorous test. Just what I'm capable of doing locally. I think we should merge this as is. And if it breaks things we can always revert it.
There was a problem hiding this comment.
I'm worried about what happens when a gem is build and pushed.
Can you try gem build please and see if the resulting gem file contains this logic or just picks one of the two add_dependency lines?
There was a problem hiding this comment.
I've built it, but I cannot inspect it as it's all machine code. Is there a way I can directly test if this logic is here? Might need help to maybe add it to a gemfile or something
There was a problem hiding this comment.
You can extract the data using, e.g.:
tar xvf cucumber-messages-2.1.1.gem
This will give you
metadata.gz
data.tar.gz
checksums.yaml.gz
Then you can look at the metadata:
gunzip metadata
more metadata
There was a problem hiding this comment.
Nope, it's not pulling through the conditionals. Urm I'll revert the bit in the gemfile. See if that fixes things. After that I'm not sure.
Although (cynically), I'd rather get it working on 2.2-2.6 than on JRuby. So I'll give it a bit more of a whirl, but if I can't make any progress, I'd still like to get this merged in as is, and then maybe try fix the jruby stuff later?
There was a problem hiding this comment.
Yes, resigning to fixing JRuby later is probably the best course of action at the moment.
cucumber-messages/ruby/Gemfile
Outdated
| @@ -1,6 +1,3 @@ | |||
| # frozen_string_literal: true | |||
| source "https://rubygems.org" | |||
| gemspec | |||
|
|
|||
| # Pick a known-good version when running builds for cucumber-messages on JRuby. | |||
| gem 'google-protobuf', '~> 3.2.0.2', platform: :jruby | |||
There was a problem hiding this comment.
I don't know why the build doesn't pick this up, or even if the build builds on any jruby, but this change makes the gem installation phase of the build fail locally for me with jRuby 9.2.5.0 and jRuby 9.2.6.0.
|
Oki, locally I've tested this again. Plus building the gem. I can build on 2.2 - 2.6 and JRuby. I'll try testing the meta-data using the same methods just now. |
|
I'm going to merge this then @mvz, and I'll raise a separate issue for tackling JRuby |
|
Thanks for the fix! 👍 |
|
@luke-hill thanks for the fix. Unfortunately this won't work. As @mvz pointed out, the gemspec is only evaluated at build time, not at install time. @tooky and I changed this in 3cba369#diff-e363e0e386df0084a887f93a1461d054 JRuby and MRI<2.6 users should add an explicit dependency on |
@take-cheeze we don't do release plans because nobody is currently paid to work on Cucumber. It will happen when it happens. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
As the comments elude to; this gem is a key runtime dependency. And on Ruby 2.6 this only works when using the latest version
Details
Stricter gemspec restrictions as per the different ruby requirements
Motivation and Context
For Running on JRuby and a couple of other environments, a weaker requirement is required (And not the latest), due to some issues with those environments.
Once the team maintaining this has done some additional work, we can probably relax this requirement. But for now this makes cucumber build properly again X-Ruby Versions
Checklist: