Skip to content

Don't chdir globally #49

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

Merged
merged 3 commits into from
Sep 27, 2016
Merged

Don't chdir globally #49

merged 3 commits into from
Sep 27, 2016

Conversation

tagomoris
Copy link
Contributor

Calling Dir.chdir in top level of Rakefile makes us not to add any tasks which should run on project root directory.
IMO, it's better to show working directory per tasks explicitly than doing chdir it globally.

@tagomoris
Copy link
Contributor Author

I know #43 (and I'm waiting it'll be merged and released), but it seems a bit large for quick merge.
This one is just removing global Dir.chdir to add another tasks.

@zzak
Copy link
Contributor

zzak commented Sep 16, 2016

Agreed, this is a good first step

@tagomoris tagomoris closed this Sep 23, 2016
@tagomoris tagomoris reopened this Sep 23, 2016
@zzak
Copy link
Contributor

zzak commented Sep 23, 2016

Could you rebase? <3

@tagomoris
Copy link
Contributor Author

I want to add this change on #47. What's the status of it?

@zzak
Copy link
Contributor

zzak commented Sep 23, 2016

@tagomoris merged.

* Calling Dir.chdir in top level of Rakefile makes us not to add any tasks which should run on project root directory.
@tagomoris
Copy link
Contributor Author

@zzak done.

@toch
Copy link
Collaborator

toch commented Sep 23, 2016

Thanks @tagomoris
those changes need also to be applied to the Rakefile. Could you also please add them there?

have you checked how it plays with the changes that will be soon introduced by #43 ?

@zzak
Copy link
Contributor

zzak commented Sep 23, 2016

@toch I think this patch as a smaller version of Eric's patch that should be easier to swallow, and allow us to move into that direction.

@tagomoris
Copy link
Contributor Author

I pushed a commit to update Rakefile by updated setup.rb

@tagomoris
Copy link
Contributor Author

ping?

@@ -25,27 +24,31 @@ current_gem = MRuby::Gem.current
app_version = MRuby::Gem.current.version
APP_VERSION = (app_version.nil? || app_version.empty?) ? "unknown" : app_version

desc "compile all the binaries"
desc "compile binary"
Copy link
Contributor

Choose a reason for hiding this comment

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

this still compiles "all" binaries, right? why change the comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I previously worked with Rakefile generated by mruby-cli v0.0.4.
Now I fixed and pushed it.

end
end
end
end

desc "run all tests"
Rake::Task['test'].clear
task :test => ['test:bintest', 'test:mtest']
task :test => ["test:mtest", "test:bintest"]
Copy link
Contributor

Choose a reason for hiding this comment

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

why change the order and quotations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed commits without such unneeded changes.

@@ -133,8 +140,7 @@ Rake.application.tasks.each do |task|
old_task = Rake.application.instance_variable_get('@tasks').delete(task.name)
desc old_task.full_comment
task old_task.name => old_task.prerequisites do
abort("Not running in docker, you should type \"docker-compose run <task>\".") \
unless is_in_a_docker_container?
abort("Not running in docker, you should type \"docker-compose run <task>\".") unless is_in_a_docker_container?
old_task.invoke
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 put this back on two lines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ditto.

@zzak
Copy link
Contributor

zzak commented Sep 27, 2016

setup.rb LGTM, please make the changes I mentioned to Rakefile and I will merge

@tagomoris
Copy link
Contributor Author

@zzak Thank you to notify me about it. I pushed fixed commits, and another fix for release task.
It works well for my project.

@zzak zzak merged commit bf1fc6e into hone:master Sep 27, 2016
@zzak zzak changed the title Not to chdir globally Don't chdir globally Sep 27, 2016
@tagomoris tagomoris deleted the fix-rakefile branch December 3, 2016 03:51
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