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.

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

Compile mruby compiler as mrbgem. #2760

Closed
wants to merge 1 commit into from

Conversation

@take-cheeze
Copy link
Contributor

@take-cheeze take-cheeze commented Apr 8, 2015

This pull request separates mruby compiler mrbc to mrbgem "mruby-bin-mrbc".

Remarkable changes:

  • MRuby compiler codes are moved to "mrbgems/mruby-bin-mrbc/core".
  • mrbc is only built in "host" build target by default.
    • Needs to call MRuby::Build#build_mrbc_exec to build mrbc for build target and use it in build
      since mrbc won't be built by "mruby-bin-mrbc" by default.
      (MRuby::Build#mrbcfile would be the build target's mrbc if MRuby::Build#bins contains 'mrbc')
  • In test driver report method is called with mrb_funcall instead of mrb_load_string.
  • "src/mrb_throw.h" is moved to "include/mruby/throw.h" since it's used in compiler build.
  • mrb_codedump_all and related functions are moved to "src/codedump.c" since it's used in mrb_load_irep_file_cxt.
@matz
Copy link
Member

@matz matz commented Apr 10, 2015

This pull-request does too many things at once. Please separate issues.

@zzak
Copy link
Member

@zzak zzak commented Apr 10, 2015

I think extract to gem can be one patch, and the other fixes can be separate tickets.

@matz
Copy link
Member

@matz matz commented Apr 10, 2015

By the way, why do you want to separate mrbc as a gem, where even the simplest test requires it?

/* evaluate the test */
mrb_load_string(mrb, prog);
mrb_funcall(mrb, mrb_top_self(mrb), "report", 0);

This comment has been minimized.

@zzak

zzak Apr 10, 2015
Member

Is this even related to mrbc gem?

This comment has been minimized.

@take-cheeze

take-cheeze Apr 10, 2015
Author Contributor

MRuby compiler codes(codegen.c and parse.y) is moved to mruby-bin-mrbc so mrb_load_string won't be defined by default.

@@ -113,11 +115,38 @@ module MRuby

def enable_cxx_abi
return if @cxx_exception_disabled or @cxx_abi_enabled
compilers.each { |c| c.defines += %w(MRB_ENABLE_CXX_EXCEPTION) }
cxx.defines += %w(MRB_ENABLE_CXX_EXCEPTION)

This comment has been minimized.

@zzak

zzak Apr 10, 2015
Member

I think we probably shouldn't change this only for mrbgem

This comment has been minimized.

@take-cheeze

take-cheeze Apr 10, 2015
Author Contributor

Sorry that was workaround for a compilation error in "codegen.c".
Applying following patch should fix it.

diff --git a/mrbgems/mruby-bin-mrbc/mrbgem.rake b/mrbgems/mruby-bin-mrbc/mrbgem.rake
index 0620735..b0f5d7e 100644
--- a/mrbgems/mruby-bin-mrbc/mrbgem.rake
+++ b/mrbgems/mruby-bin-mrbc/mrbgem.rake
@@ -15,7 +15,7 @@ MRuby::Gem::Specification.new 'mruby-bin-mrbc' do |spec|

   lex_def = "#{current_dir}/core/lex.def"
   core_objs = Dir.glob("#{current_dir}/core/*.c").map { |f|
-    next nil if build.cxx_abi_enabled? and f =~ /^(codegen).c$/
+    next nil if build.cxx_abi_enabled? and f =~ /(codegen).c$/
     objfile(f.pathmap("#{current_build_dir}/core/%n"))
   }.compact

    Modified   tasks/mruby_build.rake
@take-cheeze
Copy link
Contributor Author

@take-cheeze take-cheeze commented Apr 10, 2015

@matz
Well I heard that there's some environment that can't (or is hard to) compile parse.y so I've tried to separate it.
(In this tweet it says sometimes there's a limit to section size.)
And mrbc is only required in "host" build target when the build target always use compiled mruby binary.

Though this PR is mostly from interest so if you think this isn't required I'll close this.

@beoran
Copy link

@beoran beoran commented Apr 14, 2015

I think it's essential for embedded environments that you can compile mruby without the parser in the run time. On such small platforms precompiled byte code should be used to reduce the space needed to run an mruby program. So something like this pull request is a good idea.

@bovi
Copy link
Member

@bovi bovi commented May 12, 2015

@matz is there any chance that a pull-request like this (removing the parser from the core) might be accepted if it is separated?

@matz
Copy link
Member

@matz matz commented May 13, 2015

@bovi Yes, but separating it might be harder than it seems. Not only tests, even building full libmruby.a requires mrbc to compile .rb files.

@bovi
Copy link
Member

@bovi bovi commented May 13, 2015

@take-cheeze will you try to separate the code? otherwise I would like to give it a try maybe over the weekend.

@bovi
Copy link
Member

@bovi bovi commented May 13, 2015

CC @matz, @take-cheeze

Just to throw around a thought to split this change into small little pieces..

What about we take all *.rb files from the core and put them into a GEM? Then we would have a pure C core and everything what is Ruby related would be added via GEMs. In this way we could split this change into smaller steps:

  • extract all *.rb files into a GEM and make them (temporarily) build always (so no change compared to current state)
  • implement the option to not build this "core-lib GEM" and make it optional
  • extract mrbc to a separate GEM and make it (temporarily) build always (still no change compared to current state)
  • now we can manipulate the build environment to fix all possible dependency issues like:
    • if any GEM with a Ruby file is build, build also mrbc
    • implement option to re-use mrbc for a cross-compilation build without building mrbc for the actual cross-target (is actually already now available and just needs some tuning)
    • build mruby without parser and without any in Ruby implemented functionality
    • build mruby without parser but with in Ruby implemented functionality (Ruby code will be compiled by a helper mrbc tool for the host only)

In this way we would not only shrink the core (for several projects I actually deleted the *.rb files in the core to port mruby to MCUs), but we would also gain flexibility in what kind of functions the mruby-core really has. I'm not going so far to propose here a separation of the mruby-core-lib into GEMs like "mruby-array-core", "mruby-numeric-core", etc. but it would be an option for the future and I personally would like to see it, specially due to the reason that we have now a robust enough dependency system in mrbgeqms to describe the relations between all GEMs.

@matz
Copy link
Member

@matz matz commented May 14, 2015

@bovi the process looks reasonable.

@take-cheeze
Copy link
Contributor Author

@take-cheeze take-cheeze commented May 14, 2015

@bovi
Actually files under repository root's "mrblib" won't be a part of "libmruby_core".
They are compiled and included in "libmruby".
Anyway shrinking more is little out from this pull request so needs improvements to your idea.
(Mostly ruby API dependency in tests would be a problem.)

Separating this PR isn't much hard.
The last 3 of PR comment can be easily separated.

  • Call report with mrb_funcall instead of mrb_load_string in test result reporting.
  • Move "src/mrb_throw.h" to "include/mruby/throw.h".
  • Move mrb_codedump_all to new file "src/codedump.c".

Though the rest would be a big change anyway.
(Moving compiler codes is huge and modifying build script.)

@bovi
Copy link
Member

@bovi bovi commented May 14, 2015

@take-cheeze yes I know about the libmruby_core and libmruby difference. I just called the proposed GEM "core" to differentiate it from the "-ext" GEMs. I think it is quite hard to find a proper name for the individual libraries. "core" is as you say not correct as "core" actually only contains C based implementation code. But "stdlib" for example would also not be correct as there is more to the stdlib of Ruby then the Ruby files under mrblib. Maybe another suggestion is to just merge the mrblib code with the -ext GEMs?

My biggest problem with only moving mrbc to a GEM is, that after such a change we would have a GEM dependency to every built of mruby. As long as we can't deactivate the usage of the code under mrblib, the built system will always require to built the mrbc GEM. That pushes the GEM system to not being an extension system anymore but a hard dependency to the mruby core. Nevertheless the proposed steps were only a suggestion, it is of course also possible to use the existing pull request for now. Having mrbc as a GEM is in either way a good step forward and if you plan to do the change I'm looking forward.

PS: Concerning the Ruby API dependencies of the tests I don't see a big issue, as we can move the test code for the Ruby based libraries into the GEM too. Or did I misunderstand your point?

@take-cheeze
Copy link
Contributor Author

@take-cheeze take-cheeze commented May 15, 2015

@bovi
stdlib seems reasonable.

Non-"host" build target uses "host" build targets **mrbc** by default. So unless it's usingeval`, compiler API or mrbc explicitly.

There's not much clear definition for what should be mrbgem.
Struct class is mrbgem even though it's part of standard.(Same as regex library.)

My concern is that splitting API makes dependencies complex.
And (not about the concern) I think moving C code APIs to stdlib seems effective for shrinking library size.

take-cheeze added a commit to take-cheeze/mruby that referenced this pull request May 15, 2015
take-cheeze added a commit to take-cheeze/mruby that referenced this pull request May 23, 2015
Related to mruby#2760.
take-cheeze added a commit to take-cheeze/mruby that referenced this pull request May 25, 2015
@take-cheeze take-cheeze force-pushed the take-cheeze:mruby-bin-mrbc branch 3 times, most recently from 03112b6 to 2d9c84a May 30, 2015
@take-cheeze
Copy link
Contributor Author

@take-cheeze take-cheeze commented May 30, 2015

@matz Recommitted files with few improvements.

  • To make Build#build_mrbc_exec simple moved "mrbc" executable build to "mruby-bin-mrbc".
  • Build#mrbcfile is cached to instance variable.
  • Each build in "travis_config.rb" always builds "mrbc".

Though it seems like there's few fixes to compiler code from @cremno so I'll rebase it if needed.

Compiler codes is moved to "mruby-compiler".
Executable `mrbc` is moved to "mruby-bin-mrbc".
@take-cheeze take-cheeze force-pushed the take-cheeze:mruby-bin-mrbc branch from 2d9c84a to 971908b Jun 1, 2015
@take-cheeze
Copy link
Contributor Author

@take-cheeze take-cheeze commented Jun 1, 2015

@matz Rebased for recent changes. Ready to merge now.

@matz
Copy link
Member

@matz matz commented Jun 1, 2015

Merged. Somehow github didn't noticed the merge. closed.

@matz matz closed this Jun 1, 2015
@bovi
Copy link
Member

@bovi bovi commented Jun 3, 2015

This is great news. Thanks @take-cheeze for the work!

On Jun 1, 2015, at 21:00, matz notifications@github.com wrote:

Merged. Somehow github didn't noticed the merge. closed.


Reply to this email directly or view it on GitHub.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.