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.

Sign up for GitHub

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

Merged
merged 8 commits into from
Oct 5, 2021
Merged

Adopt pure ruby DSL implementation for JRuby #9047

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
3581d85
Update JRuby to leverage pure-Ruby DSL.
JasonLunn Oct 3, 2021
a09c8e1
Fix logic to decouple from the order of fields.keySet
JasonLunn Oct 4, 2021
b568781
Fix a test regression observed after branch update
JasonLunn Oct 4, 2021
f5ef289
Add an explicit RUBYLIB path to fix a load error under JRuby
JasonLunn Oct 4, 2021
e3ba5ec
Add trailing backslashes at end of line consistently
JasonLunn Oct 4, 2021
f88c8ef
Remove vestigial function that makes a dated claim that Ruby doesn't …
JasonLunn Oct 4, 2021
793c66d
Include JRuby's pom.xml file when updating version numbers
JasonLunn Oct 4, 2021
05ddf82
Remove blank line
JasonLunn Oct 4, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 0 additions & 5 deletions Makefile.am
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1150,19 +1150,14 @@ ruby_EXTRA_DIST= \
ruby/lib/google/protobuf/well_known_types.rb \
ruby/lib/google/protobuf.rb \
ruby/pom.xml \
ruby/src/main/java/com/google/protobuf/jruby/RubyBuilder.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyDescriptor.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyDescriptorPool.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyEnum.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyEnumBuilderContext.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyFieldDescriptor.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyFileBuilderContext.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyFileDescriptor.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyMap.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyMessage.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyMessageBuilderContext.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyOneofBuilderContext.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyOneofDescriptor.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyProtobuf.java \
ruby/src/main/java/com/google/protobuf/jruby/RubyRepeatedField.java \
Expand Down
771 changes: 28 additions & 743 deletions conformance/failure_list_jruby.txt
View file
Open in desktop

Large diffs are not rendered by default.

29 changes: 23 additions & 6 deletions ruby/Rakefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,16 +67,18 @@ unless ENV['IN_DOCKER'] == 'true'
end

if RUBY_PLATFORM == "java"
if `which mvn` == ''
raise ArgumentError, "maven needs to be installed"
end
task :clean do
task :clean => :require_mvn do
Copy link
Member

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?

Copy link
Contributor Author

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.

system("mvn --batch-mode clean")
end

task :compile do
task :compile => :require_mvn do
system("mvn --batch-mode package")
end

task :require_mvn do
raise ArgumentError, "maven needs to be installed" if `which mvn` == ''
end

else
Rake::ExtensionTask.new("protobuf_c", spec) do |ext|
unless RUBY_PLATFORM =~ /darwin/
Expand All @@ -95,7 +97,22 @@ else
]
end

task 'gem:java' do
sh "rm Gemfile.lock"
require 'rake_compiler_dock'
# Specify the repo root as the working and mount directory to provide access
# to the java directory
repo_root = File.realdirpath File.join(Dir.pwd, '..')
RakeCompilerDock.sh <<-"EOT", platform: 'jruby', rubyvm: :jruby, mountdir: repo_root, workdir: repo_root
sudo apt-get install maven -y && \
cd java && mvn install -Dmaven.test.skip=true && cd ../ruby && \
bundle && \
IN_DOCKER=true rake compile gem
EOT
end

task 'gem:windows' do
sh "rm Gemfile.lock"
require 'rake_compiler_dock'
['x86-mingw32', 'x64-mingw32', 'x86_64-linux', 'x86-linux'].each do |plat|
RakeCompilerDock.sh <<-"EOT", platform: plat
Expand All @@ -111,7 +128,7 @@ else
system "rake cross native gem RUBY_CC_VERSION=3.0.0:2.7.0:2.6.0:2.5.1:2.4.0:2.3.0"
end
else
task 'gem:native' => [:genproto, 'gem:windows']
task 'gem:native' => [:genproto, 'gem:windows', 'gem:java']
end
end

Expand Down
3 changes: 2 additions & 1 deletion ruby/lib/google/protobuf.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,10 @@ class TypeError < ::TypeError; end
require 'google/protobuf_c'
end

require 'google/protobuf/descriptor_dsl'
end

require 'google/protobuf/descriptor_dsl'

Copy link
Member

Choose a reason for hiding this comment

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

rm blank line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

require 'google/protobuf/repeated_field'

module Google
Expand Down
2 changes: 1 addition & 1 deletion ruby/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor

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.

Copy link
Contributor Author

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.

</dependency>
</dependencies>
</project>
147 changes: 0 additions & 147 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyBuilder.java
View file
Open in desktop

This file was deleted.

21 changes: 17 additions & 4 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyDescriptorPool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,9 +37,11 @@
import com.google.protobuf.Descriptors.DescriptorValidationException;
import com.google.protobuf.Descriptors.EnumDescriptor;
import com.google.protobuf.Descriptors.FileDescriptor;
import com.google.protobuf.InvalidProtocolBufferException;
import org.jruby.*;
import org.jruby.anno.JRubyClass;
import org.jruby.anno.JRubyMethod;
import org.jruby.exceptions.RaiseException;
import org.jruby.runtime.*;
import org.jruby.runtime.builtin.IRubyObject;

Expand All @@ -61,7 +63,6 @@ public IRubyObject allocate(Ruby runtime, RubyClass klazz) {

cDescriptorPool.defineAnnotatedMethods(RubyDescriptorPool.class);
descriptorPool = (RubyDescriptorPool) cDescriptorPool.newInstance(runtime.getCurrentContext(), Block.NULL_BLOCK);
cBuilder = (RubyClass) runtime.getClassFromPath("Google::Protobuf::Internal::Builder");
cDescriptor = (RubyClass) runtime.getClassFromPath("Google::Protobuf::Descriptor");
cEnumDescriptor = (RubyClass) runtime.getClassFromPath("Google::Protobuf::EnumDescriptor");
}
Expand All @@ -74,9 +75,10 @@ public RubyDescriptorPool(Ruby runtime, RubyClass klazz) {

@JRubyMethod
public IRubyObject build(ThreadContext context, Block block) {
RubyBuilder ctx = (RubyBuilder) cBuilder.newInstance(context, this, Block.NULL_BLOCK);
RubyClass cBuilder = (RubyClass) context.runtime.getClassFromPath("Google::Protobuf::Internal::Builder");
RubyBasicObject ctx = (RubyBasicObject) cBuilder.newInstance(context, this, Block.NULL_BLOCK);
ctx.instance_eval(context, block);
ctx.build(context); // Needs to be called to support the deprecated syntax
ctx.callMethod(context, "build"); // Needs to be called to support the deprecated syntax
return context.nil;
}

Expand Down Expand Up @@ -109,6 +111,18 @@ public static IRubyObject generatedPool(ThreadContext context, IRubyObject recv)
return descriptorPool;
}

@JRubyMethod(required = 1)
public IRubyObject add_serialized_file (ThreadContext context, IRubyObject data ) {
byte[] bin = data.convertToString().getBytes();
try {
FileDescriptorProto.Builder builder = FileDescriptorProto.newBuilder().mergeFrom(bin);
registerFileDescriptor(context, builder);
} catch (InvalidProtocolBufferException e) {
throw RaiseException.from(context.runtime, (RubyClass) context.runtime.getClassFromPath("Google::Protobuf::ParseError"), e.getMessage());
}
return context.nil;
}

protected void registerFileDescriptor(ThreadContext context, FileDescriptorProto.Builder builder) {
final FileDescriptor fd;
try {
Expand Down Expand Up @@ -157,7 +171,6 @@ private FileDescriptor[] existingFileDescriptors() {
return fileDescriptors.toArray(new FileDescriptor[fileDescriptors.size()]);
}

private static RubyClass cBuilder;
private static RubyClass cDescriptor;
private static RubyClass cEnumDescriptor;
private static RubyDescriptorPool descriptorPool;
Expand Down
91 changes: 0 additions & 91 deletions ruby/src/main/java/com/google/protobuf/jruby/RubyEnumBuilderContext.java
View file
Open in desktop

This file was deleted.

Loading