-
Notifications
You must be signed in to change notification settings - Fork 15.5k
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
Adopt pure ruby DSL implementation for JRuby #9047
Adopt pure ruby DSL implementation for JRuby #9047
Conversation
JRuby unit and conformance test fixes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
ruby/pom.xml
Outdated
@@ -88,7 +88,7 @@ | |||
<dependency> | |||
<groupId>com.google.protobuf</groupId> | |||
<artifactId>protobuf-java-util</artifactId> | |||
<version>3.13.0</version> | |||
<version>3.18.0</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you'll need to add an entry to this:
https://github.com/protocolbuffers/protobuf/blob/master/update_version.py
to ensure this stays updated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done - thanks for the pointer.
raise ArgumentError, "maven needs to be installed" | ||
end | ||
task :clean do | ||
task :clean => :require_mvn do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need to require Maven for this? Could we just run mvn
only when it is present?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JRuby gem assembly process has a dependency on Maven - they just don't need to be enforced unless you're actually running one of the JRuby tasks; previously it was enforcing the availability of Maven when the tasks were defined.
ruby/lib/google/protobuf.rb
Outdated
end | ||
|
||
require 'google/protobuf/descriptor_dsl' | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rm blank line?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
JRuby unit and conformance test fixes.
Includes changes originally submitted in #8997 and #8954