Rakeify rake tasks #43
Conversation
Also, remove () to be consistent across uses of FileUtils methods
This makes it more clear what the commend means
I think that previously the Rakefile would always create mruby_root as "mruby" regardless of what was in the MRUBY_ROOT environment variable. Now the correct directory will be created. A `mruby` task is retained for compatibility.
This moves us a step closer to not having `cd mruby_root` polluting the package tasks and allows us to make loading mruby use `import` as Jim Weirich intended.
If we rewrite the task Bad Things happen (like a Rake::FileTask turning into a Rake::Task). Instead we can use a dependency because that runs before anything the task does that would require Docker. PS: rake-hooks is a pointless library. Rake already has a way to run things before a task, a prerequisite. Rake already has a way to run things after a task, make the after-task have the before-task a prerequisite. With some organization of tasks the before/after isn't an issue, it just falls out with how you hook up your dependencies.
Rake has the #import method to safely import Rakefiles into the current rake application. Using load or require may cause rakefiles to appear in the wrong place or require cumbersome setup. In this case the cumbersome setup was the `cd "mruby"` that required adjustment for the correct path in the release and package tasks.
Otherwise we're making directories when they're not needed.
Missed this one
|
I get the same test failure if I build an app with the latest release of mruby-cli and do nothing but run |
The test task depends upon clearing MRuby's test task but we can't do that until after MRuby's Rakefile loads. Moving the test tasks to a separate file we import allows us to clear MRuby's test actions at the proper time.
|
Figured it out, need to clear the |
|
The patch looks good to me, but we probably need to test this to make sure it doesn't break release or anything (I'm not sure those are thoroughly tested). |
|
Seems fine: PS: |
|
I'm wondering why you want to import the mruby rake tasks atop your own. I find building mruby via subshell is less trouble and easier to use. The same method could be used to run tasks inside docker. |
|
What is the point of running rake inside of a subshell.. in rake? :woah: mruby knows how to compile mruby, and when we started building stuff with mruby, we leaned on the existing tasks to do the work for us. What you're doing with mruby-optparse is fine, but I don't think every gem doesn't want to maintain this much configuration. The Rakefile is a generated file, that users shouldn't have to maintain, it's still there so the smaller the better. But anyways, when I get a chance to finish mruby-build, hopefully most of this stuff can be upstreamed and we don't have to bikeshed in each project. Back to this patch, the other thing is mruby-cli was generated.. with mruby-cli. So we have to update the setup.rb stuff so user apps can take advantage of these changes. The downside to this patch is breaking up the tasks into separate files means having to generate them for the user too (on setup), more files means more things to diff when doing upgrades and such. So we have to consider that as well. |
|
@drbrain Thanks for doing that. It's cleaner 👍. About package task, it seems that the last docker image is not the good one anymore (even if the Dockerfile in the repo is correct). @hone or @zzak could you give me access to the docker registry of mruby-cli image so I can fix it? @drbrain as Zak explains, mruby-cli generates different files, including a Rakefile thanks to About, multiple files. @zzak I'm wondering if it wouldn't be the path to avoid to have twice the same code. If we can isolate the different task and make them generic, |
|
I will update setup.rb soon. @toch I must say I vastly prefer libraries over generated code. It's a ton of work to update generated code, but library code updates are (within compatible versions) labor-free. Unfortunately I'm unsure how a library model can work with mruby-cli as a single binary. (You could do some bizarre stuff with The reason I'm concerned about importing mruby's Rakefile is issues like needing to work around mruby or having to tell users a task must run in docker (which was broken). There's a few reasons I think separation will be better. One is that it makes getting started as a rubyist familiar with rake much easier. I can't Another reason I prefer a docker subshell is that I can't easily get tasks to run outside docker to set up state needed for tasks that must run inside docker. For example, I want to generate some .rb files that end up in With a docker subshell I could use regular rake dependencies to take care of that in a straightforward manner. I could also set up tasks to easily upload my built CLI when docker was finished all from one rake task's dependencies. For the above reasons I'm likely to switch my current mruby-cli build over to the docker subshell model because I will have less to teach my coworkers who will contribute to my CLI. I can bring the instructions down to "download docker, install docker, run The final one is that it's a separation of concerns. Presently mruby-cli doesn't make any changes to mruby's Rakefile (besides the PS: Even rake has a |
|
Oh, the final thing I should add about importing mruby's Rakefile is that to do it in a rake-like fashion it involves features few people know about, like |
Also wrapped some lines at 80 columns to improve readability.
|
setup.rb updated |
This check's dependency is :all which runs the compilers so it can never check what it needs to. Unfortunately there's no place we can move this compiler check so that it will work. Having `task all: :compiler_check` won't work either because the all task depends on the .o files which run the compiler check. I haven't found any tasks in mruby that we can hook into to run such checks. I looked extensively to find a place to hook-in for cross-compiling third party libraries (like libcurl) and such a hook doesn't exist. In order to restore this functionality mruby-cli must not import mruby's Rakefile. Then we can have tasks that run before mruby is built (like cross-compiling libraries or checking the cross-compiler environment).
|
See also @c6adb30 for a reason why mruby-cli should not import mruby's Rakefile. |
|
@drbrain first of all, thanks for your patience and your answer. Let me go through your answers first :). libraries vs generated code.I agree with you. I think it would be cleaner to extract that part so we don't have to duplicate it. It's easy to forget to replicate it in the So far, my only current concern is as you said it well: "Unfortunately I'm unsure how a library model can work with mruby-cli as a single binary." In our case we need to consider: 1. refactoring of local:ensure_in_dockerThanks a lot, it's cleaner and I learned something about Rake :). rake vs docker-composeFrom a user point of view, I really like the idea. It would provide a better UX as we'd provide a very familiar set up, as you said. a docker subshellI like what you suggest. In fact, it would allow us to use even more docker as a tool rather than an environment. There are options to do what you suggest from inside, but it's not simple and it would require in a certain case to run it with privileged mode what we don't want. about separation of concernsagreed I'm going to review your changes now. |
| # since we're in the mruby/ | ||
| release_dir = "releases/v#{APP_VERSION}" | ||
| release_path = Dir.pwd + "/../#{release_dir}" | ||
| release_path = File.expand_path "releases/v#{APP_VERSION}" |
There was a problem hiding this comment.
another good consequence :)
no more headache due to the surprising working directory
|
I've just commented a few lines. Not a lot to say except about the generation of code. I still need to test the whole, I'll do that after my lunch. |
|
There is an issue with the For the task the variable The other tasks work as expected. The scaffold works also as expected except for the Is it correct that for now, your changes don't implement your idea to run docker from rake? |
|
I'll check the loop and missing I don't plan to implement running docker from rake here, it's too much change for one PR. |
|
thanks
👍 and I'd happy to help |
Previously we had a task rewriter that made sure certain tasks were run in docker that prevented this infinite loop by accident. After removing the task rewriter the dependencies were circular (through #sh) and things could run forever. (Note that the `clean` task didn't change prior to this commit.) Rather than trying to unravel this infinite loop let's run `deep_clean` directly via docker and let it do all the right things. The `clean` task remains for documentation purposes. MRuby already defines it for us, but now our users will know to hook into it.
|
Fixed the infinite loop by removing the looping. Somehow the task rewriter caused this loop because I didn't change the clean task before @b6404e4. See commit message for details. |
|
This should be ready to go now, sorry for the delay. |
|
Thanks @drbrain, could you rebase please? I'm going to try the changes again to be sure. |
This PR makes the rake tasks in mruby-cli use more rake features than they presently do.
Most notable is use of import to load the Rakefile from mruby after the appropriate setup has been finished (same for the package tasks).
Ensuring certain tasks are run inside Docker now uses dependencies instead of rewriting the tasks. (Rewriting the tasks is bad because it converts FileTasks into Tasks and changes their behavior.)
Also, FileUtils references are removed as Rake already included that for us.
The result is as Jim Weirich intended.