-
Notifications
You must be signed in to change notification settings - Fork 28
replace mruby-getopts with mruby-docopt #32
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
base: master
Are you sure you want to change the base?
Conversation
@@ -11,6 +11,7 @@ def gem_config(conf) | |||
conf.enable_bintest | |||
conf.enable_debug | |||
conf.enable_test | |||
conf.enable_cxx_abi |
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.
is there a way we can have this flag be autodetected instead of manually being added for anyone who wants to mruby-docopt?
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 just tried without it, and it failed.
http://ruby-doc.org/core-mruby/doc/guides/compile_md.html#label-C-2B-2B+ABI
mruby can use C++ exception to raise exception internally. It is called C++ ABI mode. By using C++ exception it can release C++ stack object correctly. Whenever you mix C++ code C++ ABI mode would be enabled automatically. If you need to enable C++ ABI mode explicitly add the following: ruby conf.enable_cxx_abi
So it should be detected automatically, moreover, I've enabled it into mruby-docopt. So it seems that it's too late when it detects and enables it.
Should MRuby::Build
and MRuby::CrossBuild
configuration of mrbgems infect the project depending on them?
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.
We should be able to add defines in the mrbgem.rake of mruby-docopt
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.
Could you explain @zzak ? How does it work?
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 dug into mruby code and find the following:
- When setting up the gems, it detects if any cpp files are involved and enable c++ abi
- If C++ abi is enabled, the mruby core is compiled with that feature, It consists in listing the files to compile, in particular, if cxx abi is enabled, to select vm.cxx and error.cxx rather than their C version.
- Unfortunately, the detection in the gems is done too late, as it occurs after the preparation of mruby core.
- When fixing it (see patch below), I bumped into the following issue
CC build/host/mrbgems/mruby-compiler/core/y.tab.c -> build/host/mrbgems/mruby-compiler/core/y.tab.o
This issue is the same than previously. As the C++ code has been enabled, some files should be appropriately selected as for vm.cxx and error.cxx
y.tab.c
belongs to mruby-compiler. Its C++ version should be selected, it's not yet enabled when it comes to that gem, explaining why it fails.
The patch I've mentioned above:
diff --git a/Rakefile b/Rakefile
index 3021bcc..6b4a9ec 100644
--- a/Rakefile
+++ b/Rakefile
@@ -21,9 +21,28 @@ end
# load custom rules
load "#{MRUBY_ROOT}/src/mruby_core.rake"
+cxx_abi_enabled_before_gems = MRuby.each_target.inject({}) { |hsh, target| hsh[target] = target.cxx_abi_enabled?; hsh }
load "#{MRUBY_ROOT}/mrblib/mrblib.rake"
load "#{MRUBY_ROOT}/tasks/mrbgems.rake"
+MRuby.each_target do
+ if !cxx_abi_enabled_before_gems[self] && cxx_abi_enabled?
+ path_mruby_core_rake = "#{MRUBY_ROOT}/src/mruby_core.rake"
+ current_dir = File.dirname(path_mruby_core_rake).relative_path_from(Dir.pwd)
+ relative_from_root = File.dirname(path_mruby_core_rake).relative_path_from(MRUBY_ROOT)
+ current_build_dir = "#{build_dir}/#{relative_from_root}"
+ libmruby.each do |objs|
+ Array(objs).each do |obj|
+ if found = obj.match(/(error|vm)#{exts.object}$/)
+ filename = File.basename(found.to_s).sub(/#{exts.object}$/, "")
+ obj.replace(compile_as_cxx "#{current_dir}/#{filename}.c", "#{current_build_dir}/#{filename}.cxx")
+ end
+ end
+ end
+ end
+end
+
I don't like it at all, but it helps to understand the problem and what to do.
All of that makes me ask the following question:
- does the automatic detection works correctly for other C++ cases?
- shouldn't the C++ detection should be done through all the mruby and mrbgems before preparing the files to compile?
- which mruby contributor may I contact about that?
@hone @zzak here the replacement of getopts with docopt :)