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

stop using fork, which means JRuby support #148

Merged
merged 1 commit into from
Jun 24, 2014

Conversation

mrbrdo
Copy link
Contributor

@mrbrdo mrbrdo commented Oct 2, 2013

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Oct 31, 2013

@nadarei any plans to merge this? as of current master jruby support is still broken

@ricardodovalle
Copy link

👍

@smecsia
Copy link

smecsia commented Dec 12, 2013

+1 for this

@sfritz
Copy link

sfritz commented Feb 12, 2014

👍

@alisnic
Copy link

alisnic commented Feb 24, 2014

I wasted half a day configuring mina for our project only to discover "fork is not available on this platform"

Please merge this 👍

@kucaahbe
Copy link

kucaahbe commented Mar 3, 2014

just install as system gem for system ruby(it's often MRI isn't it?) and use, there no need for deployer to wait JVM to boot and run deploy script, it isn't fast.

@alisnic
Copy link

alisnic commented Mar 3, 2014

@kucaahbe true that, however it's cumbersome to switch rubies every time I want to deploy. It kind of beats the purpose of automatic deploy if I have to do some stuff by hand to make it work.

Also, given modern hardware and tools, JVM boot time is rarely an argument. (even on my laptop)

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Mar 3, 2014

@kucaahbe that's not a valid argument, especially when the fix is so trivial.
And I never install a system ruby. Also then I cannot manage which version of mina I want to use for a specific project.
I've been using mina mostly with JRuby projects and I have no performance issues with the boot time.

@kucaahbe
Copy link

kucaahbe commented Mar 3, 2014

@mrbrdo " I never install a system ruby"- on most linux distributions and OSX there already pre-installed ruby(mri) as dependency and disinclination to install ruby isn't enough argument than supporting one more interpreter, take a look at travis build, there already 6 builds and lack of 2.x.x, and you want author to mess with jruby just because you just want and you fix is (currently) trivial? In any case I hope maintainer will read this and take into account.

@alisnic - no need to do that, any gem already know which interpreter to use on adequate installations. Also this simple wrapper could be used: https://gist.github.com/kucaahbe/9324386. We use it (project itself on jruby) and we happy.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Mar 3, 2014

@kucaahbe I don't agree with you and I see no reason not to support JRuby at this point. I provided a pull request and I don't see a reason not to merge it, there is no JRuby specific code in it, it just uses threads instead of fork. If it comes to a point where supporting JRuby is a problem, support can be dropped at that point, but at the moment I don't see a reason not to support it. Not accommodating a change like that just because "XY interpreter support might break anyway some time in the future" is ridiculous.

The threading solution is also more efficient and makes more sense than forking. So regardless of the fact that it also works on JRuby it should probably be merged.

@kucaahbe
Copy link

kucaahbe commented Mar 3, 2014

@mrbrdo You misunderstood me, I don't see any issues with code, furthermore I agree that threading could be more efficient than forking, my point that there no reason to explicitly support jruby as there much simpler and native way to use gem. After bundler ruby people forget about what PATH environment variable is and often confused with gems which needed for project itself (e.g. dependencies) and gems that only provide some support stuff, in this case mina is not direct dependency (for instance anyone could use fabric to deploy any project written in any language, and no one will complain about lack of jruby support)

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Mar 3, 2014

@kucaahbe it might not matter for your setup, but it matters for mine. It introduces a dependency that I neither want nor need. Also I do not like your argument at all, how would you feel if I made a gem that only worked on rubinius and then told you that there is no reason for me to support MRI since you can use rvm or rbenv and use my gem with rubinius, even if you use MRI on your project.

And especially when the gem is not doing anything that would make it hard or inconvenient to add support. I understand some gems depend heavily on C extensions or really need fork, but here this is simply not the case.

@kucaahbe
Copy link

kucaahbe commented Mar 3, 2014

@mrbrdo If I you create some good solution that will work only in rubinius I'll install rubinius, because I can and think in terms of solving project's problems.

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Mar 3, 2014

It's your opinion. If you want to express it go to a forum or mailing list, this is not the place for it.

@alisnic
Copy link

alisnic commented Mar 3, 2014

@mrbrdo , @kucaahbe guys, let's keep this conversation nice and constructive, shall we?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented Mar 3, 2014

Just keep to the point. And anyway, it doesn't really seem like maintainer is active for this project anymore, so I doubt we will accomplish anything.

@rstacruz
Copy link
Member

rstacruz commented May 8, 2014

Will this still work on MRI?

@mrbrdo
Copy link
Contributor Author

mrbrdo commented May 8, 2014

Of course it will, MRI supports threads

@gabskoro
Copy link
Member

@rstacruz I'm also for this, what do you think?

@rstacruz
Copy link
Member

...let's do it and release 0.4.0 soon. this is swell.

rstacruz added a commit that referenced this pull request Jun 24, 2014
stop using fork, which means JRuby support
@rstacruz rstacruz merged commit d10201c into mina-deploy:master Jun 24, 2014
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.

8 participants