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

Add some information into the doc about development #9

Merged
merged 1 commit into from
Aug 15, 2015

Conversation

toch
Copy link
Collaborator

@toch toch commented Aug 2, 2015

As I missed first the docker-compose run compile and was confused, I've bumped in several errors before figuring out what to do. Here some notes that could help others that would like to contribute to mruby-cli. Do you find it useful?


#### Common error

If rather than typing `docker-compose run compile`, you type `rake compile`, you could get on a nix OS the following error:
Copy link
Contributor

Choose a reason for hiding this comment

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

This happens because users must* compile their binary in the mruby-cli docker container.

  • Actually only if they want to use the cross-build configuration we generate.

Just using rake compile will not work because the main Build is designed to compile on a 64-bit Linux Host.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I eventually got that the cross-toolchain is in the container, but I don't at first.

For the text, what do you think of:

This app is built as a `mruby-cli` app. To compile the binaries, you **must** type

docker-compose run compile

and find the binaries in the appropriate directories (mruby/build/<target>/bin/).

The docker container contains the necessary cross toolchain to compile a binary for each supported target. Just using `rake compile` will not work out of the box because the main build is designed to compile on a 64-bit Linux host. It could work if you are on a 64-Linux host and you have an cross toolchain equivalent to the one we provide into the docker container.

Just using rake compile will not work because the main Build is designed to compile on a 64-bit Linux Host.

but it will work if I have a equivalent cross-toolchain available in my path and I'm on a 64-bit. right?

If so, maybe rather than just making it clearer in the documentation, I would warn the user. In that way, I'd consider two possible options:

  • when using one or another rake task requiring the container, checking that we are into the right container
  • when using compile task (or any other task requiring it), checking that a compatible toolchain is available

Copy link
Owner

Choose a reason for hiding this comment

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

How far did you get on the detection work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hone I've just pushed a new commit including both suggestions.

The first check tests if it is run inside a Docker container. That could be maybe soften if on a compatible host.

When compiling, the second check tests the presence of cc command for every target.

What do you think?

@toch
Copy link
Collaborator Author

toch commented Aug 4, 2015

@hone @zzak thanks for the feedback, I'll look at that tonight.

@toch
Copy link
Collaborator Author

toch commented Aug 5, 2015

changes done.


Indeed, just using `rake compile` will not work out of the box because the main build is designed to compile on a 64-bit Linux host. It could work if you are on a 64-Linux host and you have an cross toolchain equivalent to the one we provide into the docker container.

It means that if you want to add a new rake task `my_task`, you need to add it to the `docker-compose.yml` to make it available through `docker-compose run my_task`.
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick: "This means.."

@zzak
Copy link
Contributor

zzak commented Aug 7, 2015

Just some nitpicks, otherwise good! 👍

@toch
Copy link
Collaborator Author

toch commented Aug 8, 2015

Thanks @zzak for the nitpicks, just pushed the corrections.

Do you want me to squash the all into one commit? What do you prefer?

MRuby.each_target do |target|
`#{target.cc.command} --version`
abort("Command #{target.cc.command} for #{target.name} is missing.") unless $?.success?
end
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also add this and is_in_a_docker? to mrblib/setup.rb?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess into the rakefile method? could you explain me why do you output again those files via setup.rb?

Copy link
Contributor

Choose a reason for hiding this comment

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

@toch Exactly, we use the rakefile() method to generate Rakefile for the user.

Since mruby-cli itself was generated.. by itself, we use MRubyCLI::Setup to deal with the file generation for us. Meaning, when you type mruby-cli --setup foo that setup.rb will know to create all of the files for your "foo" cli app. It's like rails new but better.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, thanks for the explanations.

@toch
Copy link
Collaborator Author

toch commented Aug 11, 2015

@zzak I've rebased my branch onto master to include last commits and make it mergeable.

@toch
Copy link
Collaborator Author

toch commented Aug 11, 2015

:/ well I have a problem with the test and setup.rb when generating the Rakefile. I'm looking at it.

@toch
Copy link
Collaborator Author

toch commented Aug 11, 2015

@zzak found the issue, now the tests pass \o/

@zzak
Copy link
Contributor

zzak commented Aug 12, 2015

Now that we're making code changes, would you be able to split up the patch into a separate PR for the docker stuff?

I want to be able to merge your docs in before we continue improving the runtime situation! :)

Also, thanks for working on this stuff <3

@toch
Copy link
Collaborator Author

toch commented Aug 15, 2015

Now that we're making code changes, would you be able to split up the patch into a separate PR for the docker stuff?

I've cleaned up the commits and split the changes into two PRs as asked:

Also, thanks for working on this stuff <3

My pleasure :).

zzak pushed a commit that referenced this pull request Aug 15, 2015
Add some information into the doc about development
@zzak zzak merged commit 19b75a7 into hone:master Aug 15, 2015
@zzak
Copy link
Contributor

zzak commented Aug 15, 2015

@toch Thank you!!

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.

3 participants