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

Native support of mruby-cli in mruby build system. #3956

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

take-cheeze
Copy link
Contributor

@take-cheeze take-cheeze commented Feb 23, 2018

Just for a proof of concept I've rewritten mruby-bin-strip only with ruby script for mruby-cli feature.
For mrb_irep section modifications, MRubyVM (subset of RubyVM in CRuby) is implemented too.

Since mruby-cli isn't catching up with recent mruby releases, we should support this with mrbgem build system.
~~ This is not complete because I need to add minimal dependency mrb_open for better testings too. Symbol like mrb_open_cli must be generated. ~~
Now implemented: 34a3939

See #1676 for implementation detail.

@take-cheeze take-cheeze changed the title [Don't merge!] mruby-cli Native support of mruby-cli in mruby build system. Mar 20, 2018
@matz
Copy link
Member

matz commented Mar 21, 2018

I am not sure the benefit of the proposed change. I try to read the diffs but they are huge.
Could you explain what we can expect from the change?

Besides that, this pull-request introduces 3 global variables. It's against our basic rule to avoid global variables.

@@ -89,6 +89,11 @@ void mrb_gc_mark_mt(mrb_state*, struct RClass*);
size_t mrb_gc_mark_mt_size(mrb_state*, struct RClass*);
void mrb_gc_free_mt(mrb_state*, struct RClass*);

extern mrb_int
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, sorry these corresponds to those in CRuby:
https://github.com/ruby/ruby/blob/421a73f51a39ff6829f9715b423a928782ec8ec3/vm.c#L339-L341
And it should be mrb_state specific.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should be optional like VM hooks.

@take-cheeze
Copy link
Contributor Author

@matz
This PR is mostly proof of concept so I'll split this into 3 PRs when approved:

  • modification of build system(Rakefile, tasks/cli.rake, tasks/cli/main.c, toolchain script)
    • Forgot to add option to use mrb_open instead of minimal dependencies mrb_open_cli for command like mruby. Will implement it too.
  • mrubyvm mrbgem which implements RubyVM class for mruby (related to: load bytecode from string (in ruby) #3955 )
  • switchmruby-strip implementation to a mruby script

I want to know:

  • modification to build system is OK.(1st PR of above)
  • writing command line tools with mruby script is OK.

We need to know this feature works and all knows it's useful.
If OK I'll start splitting it to 3 PR.

@matz
Copy link
Member

matz commented Mar 22, 2018

The proposed change sounds reasonable. It is OK to change the build system and writing command line tools in mruby.

But I don't want to add members to mrb_state unless they are absolutely necessary.

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

Successfully merging this pull request may close these issues.

None yet

2 participants