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

Check rake environment #13

Closed
wants to merge 4 commits into from
Closed

Conversation

toch
Copy link
Collaborator

@toch toch commented Aug 15, 2015

  • release task depends on compile task
  • abort if not in a docker
  • abort compile task if toolchain is not present

@zzak
Copy link
Contributor

zzak commented Aug 16, 2015

@toch Could you please rebase?

@toch
Copy link
Collaborator Author

toch commented Aug 18, 2015

@zzak rebase done

@toch
Copy link
Collaborator Author

toch commented Aug 22, 2015

Should I have to do something else?

@zzak
Copy link
Contributor

zzak commented Aug 23, 2015

@toch I'm still curious why you had to manually load "#{mruby_root}/test/mrbtest.rake" in the compile task.

This rake task will actually go away once we can merge my patch to extract the test binary stuff upstream in mruby, so your change will actually break (or something).

What happens if you remove it?

@toch
Copy link
Collaborator Author

toch commented Aug 24, 2015

The error I got was: undefined local variable or method target for main:Object.

I cleaned everything, removed the load, and I ran again docker-compose run compile. I do not have the error ><.

In the next commit, I removed the load from Rakefile and setup.rb.

@zzak
Copy link
Contributor

zzak commented Sep 22, 2015

This will need a rebase as well

@toch
Copy link
Collaborator Author

toch commented Sep 22, 2015

@zzak done

@kbrock
Copy link
Contributor

kbrock commented Sep 26, 2015

@toch would it make sense to have this as a task dependency? Are there rake tasks that may make sense to run locally?

task :docker do
  abort("Not running in docker, you should type \"docker-compose run <task>\".") unless is_in_a_docker?
end

task :compile => :docker

@toch
Copy link
Collaborator Author

toch commented Sep 26, 2015

I've squashed the commits in two commits: one is about the compile task, the other about the abortion in case of not being run into a docker container. After discussion with @hone, it is possible to disable that check, so ppl that would like to run it locally can.

@kbrock As setting up a cross toolchain is not easy, I think the idea is to not pull the user into that by default. I've added an option to disable that if you want to do it locally. @hone and @zzak am I wrong?

@hone
Copy link
Owner

hone commented Sep 30, 2015

@kbrock that's an interesting idea.

@toch that's correct. thanks for squashing. I've added some local tasks. I wonder if this is a use case we should support?

@toch
Copy link
Collaborator Author

toch commented Sep 30, 2015

@hone

  • I just updated my last commit that was buggy
  • not sure to follow, what are those local tasks?

@hone
Copy link
Owner

hone commented Oct 1, 2015

Just an example of local rake tasks that can be run/added to a project in
addition to the docker stuff. That should maybe be a separate PR though
where we take these tasks and pull them into a separate file for ease of
upgrading.

On Wed, Sep 30, 2015, 7:24 PM Christophe Philemotte <
notifications@github.com> wrote:

@hone https://github.com/hone

  • just update my last commit that was buggy
  • not sure to follow, what are those local tasks?


Reply to this email directly or view it on GitHub
#13 (comment).

@toch
Copy link
Collaborator Author

toch commented Oct 5, 2015

Just an example of local rake tasks that can be run/added to a project in addition to the docker stuff. That should maybe be a separate PR though where we take these tasks and pull them into a separate file for ease of upgrading.

I think it's a good idea to discuss that in another PR. I don't like to put to many changes in the same PR.

@toch toch force-pushed the check-rake-environment branch 2 times, most recently from 8992579 to 79b4a5e Compare October 11, 2015 21:29
@toch
Copy link
Collaborator Author

toch commented Oct 11, 2015

Due to the line 14 we load "#{mruby_root}/Rakefile" and because of the root permission on the volume mounted from the container, it wasn't possible to run a local task. So I've tried to get rid of the root permission and come with 694e00e.

The only issue is that the local uid and gid are hardcoded. I didn't find a way to resolve that. Do you know by chance how to do that?

Inspired by @kbrock's idea, after several issues, I ended with the last commit: 79b4a5e

  • it's cleaner,
  • you don't have to worry, it's checked by default
  • it allows to add local task

@hone @zzak what do you think of that new version?

The test failure is the same @hone reported #23 + another one linked to bintest/mruby-cli.rb#setup there is indeed a missing = at line 10, but even after fixing that it still fails as it doesn't find the directory of the new_app. When I use manually mruby-cli, it works correctly. Any idea why?


RUN groupadd -r mruby -g 1000 && \
useradd -d /home/mruby/ -u 1000 -g mruby -r -s /sbin/nologin -c "Docker image mruby user" mruby && \
chown -R mruby:mruby /home/mruby/
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go in the upstream container?

I thought we had issues with this on OS X because how boot2docker deals with permissions

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This should go in the upstream container?

Of course, I put it there temporally so you can try it.

I thought we had issues with this on OS X because how boot2docker deals with permissions

I didn't know that. I'll try on OS X. thanks for the info.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@zzak is it the issue you mentioned boot2docker/boot2docker#581 ?

by the way, new version of docker comes with interpolation, very helpful for a non static UID docker/compose#1532 (comment) (available in docker-compose 1.5)

@toch
Copy link
Collaborator Author

toch commented Oct 12, 2015

FYI, the last commits should fix the broken tests. (ofc 8f8b76c is temporary)

@toch
Copy link
Collaborator Author

toch commented Oct 25, 2015

@zzak @hone
Here we go, the commit 02503e6 allows to synchronize the owner/group in Docker with the one of the host. (just the docker-compose shell works for now, that's why the tests fail)

@zzak could you try on MacOSx please?

does one of you has a ready environment on Windows to try it?

@toch toch force-pushed the check-rake-environment branch 3 times, most recently from d25aef8 to 16b30e8 Compare October 26, 2015 10:22
@toch
Copy link
Collaborator Author

toch commented Oct 26, 2015

all docker-compose tasks are supported now, they will be pass to the script correctly setting the environment and the permission according the host permission.

@toch
Copy link
Collaborator Author

toch commented Oct 26, 2015

interesting. yeah, ENTRYPOINT seems really powerful. if we get rid of docker-compose, we could support windows as well. does your ENTRYPOINT script work on windows?

It's a bash script that is run in the container itself. It should work according my reading, I'll test that asap.

And indeed, that would be nice to just do docker run test. The good think with docker-compose is the mounting of volume.

That'd be nice, but not a deal breaker. It does seem a bit hacky, but I'm +1 if it works. I'm not sure I follow the first bullet point, but I'll take a look at the code.

It does what Docker should do :). Honestly, it's not so hacky, I use only standard tools and bash.

@toch
Copy link
Collaborator Author

toch commented Oct 26, 2015

According the doc, there is a a directive VOLUME for Dockerfile.

@toch
Copy link
Collaborator Author

toch commented Oct 26, 2015

I tried to install docker toolbox on my OsX in a VirtualBox ... I have a problem VT-x detection making impossible to run a VM inside a VM. :/

I'll try with a Windows in a VM.

@zzak
Copy link
Contributor

zzak commented Oct 27, 2015

I can test on OS X and I still have a Windows 10 VM available for testing

@toch
Copy link
Collaborator Author

toch commented Oct 27, 2015

Thanks @zzak!

Would be nice to find a way to test on each platforms easily.

@toch
Copy link
Collaborator Author

toch commented Nov 4, 2015

follow up of a call I had with @zzak today

it has drifted too much :p
so we keep only the check we are in a docker container and the env var to disable it
in another branch we ll deal with a better way to allow the dev to play with mruby and gems without having to deal with root permissions

@hone
Copy link
Owner

hone commented Nov 4, 2015

yeah, the docker root permission stuff should be a separate PR. Let's take that out and I'm +1 for the rest of the PR.

@toch
Copy link
Collaborator Author

toch commented Nov 14, 2015

@zzak @hone done, and I put the non-root solution for docker here https://github.com/toch/mruby-cli/tree/non-root-docker I'll open a separated PR about that.

@zzak
Copy link
Contributor

zzak commented Nov 16, 2015

@toch Thank you!!

I will test this out! <3

* release task depends on compile task
* check the presence of toolchain before running the compile task
* remove superfluoous load of mrbtest.rake
* abort the task if not run in-docker
* env var MRUBY_CLI_LOCAL allows to run the rake task locally
* local task can be added
* refactoring of the check is-in-a-docker-container?
it seems that the options weren't correctly passed to the command.
@toch
Copy link
Collaborator Author

toch commented Nov 18, 2015

rebased onto master to resovle conflict

@hone
Copy link
Owner

hone commented Nov 20, 2015

@toch when running the tests b949478 was unnecessary for me. If you look at the capture3 API, it actually takes a *cmd, so it should work fine without this comment. Can you rebase this out and then I'm happy to merge.

hone added a commit that referenced this pull request Nov 20, 2015
@hone
Copy link
Owner

hone commented Nov 20, 2015

Merged in eef2885

@hone hone closed this Nov 20, 2015
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

4 participants