Move to Puma #8
Move to Puma #8
Conversation
|
Things I don't understand:
|
|
I don't know why It should be possibly to get it working via Alternatively, JRuby 1.7 release notes say that |
|
It uses fork. Fork doesn't work on JRuby. It's not an easy as a change as you think - the dependency is coming from chef-pedant. |
|
Process.spawn on 1.9.2+ implements posix_spawn() -- might be an option here since I'm pretty sure JRuby does it as a part of its 1.9 support. I also know popen works on jruby. |
|
Yeah, definately a sticky problem. It's easy to run the proc via Here's the two uses in The ruby-related environment var blanking implies these both really depend on |
|
wow, edited that comment, seems the formatting was messed up from emailing it. tl;dr -- Process.spawn can help and likely can replace the need for Shellout. here's the docs: http://ruby-doc.org/core-1.9.3/Process.html#method-c-spawn |
|
@erikh @josephholsten right, but I'm not sure |
|
I think chef-pedant will take PRs. Need to run against chef 11 server first though. |
|
Might be better to do this to mixlib-shellout instead -- depends on your need as a company for ruby 1.8 still. No time for it myself (sorry!), but if you look at the docs it shouldn't be too complicated to make shellout play nicely with Process.spawn. |
|
@jkeiser are these legitimate test failures on chef-zero? Or something related to the move to puma/changing pedant? This message was sent from my mobile device. I apologize in advance for any typographical errors or autocorrections. On May 17, 2013, at 10:14 PM, Erik Hollensbe [email protected] wrote:
|
|
Not sure--almost certainly some new pedant tests that I haven't looked at yet. I know there are something like 45 failures right now without any patches (Jamie noticed this). |
|
@jkeiser okay. That's similar to what I'm seeing. Also, the change to puma marks a significant unintended side-effect. CZ was already really fast, but, for some reason (I'm not sure), Puma makes it even faster. I was using it against the berkshelf test suite and the entire runtime decreased by 1 min. |
This reverts commit ddfc767.
|
@jkeiser I just rebased with master and I'm still seeing failures. I also tried master and I'm seeing failures. |
| @generate_real_keys = options[:generate_real_keys] | ||
|
|
||
| ChefZero::Log.level = options[:debug] ? :debug : :info |
There was a problem hiding this comment.
This feels like it should be set outside of ChefZero::Server, so that you can set it to :warning or :error if you want.
There was a problem hiding this comment.
In a780cbb, I changed it to accept a -l flag instead of -d, so the user can set the value via the CLI. The user can already manipulate this value programmatically the same way I've done here.
|
@sethvargo the patch looks great in general, just a few things. Check run-pedant.rb: it should be doing a reset --hard to a specific git tag. Can you check if your "rake test" / "rake spec" is doing that? |
|
@jkeiser I'm not sure why there's these three failures. I don't think it's related to the move to puma: |
| module ChefZero | ||
| autoload :Log, 'chef_zero/log' | ||
| require 'chef_zero/core_ext' | ||
| require 'chef_zero/log' |
There was a problem hiding this comment.
What does requiring these inside ChefZero do? Why get rid of the autoload?
There was a problem hiding this comment.
Requiring them inside guarantees the module is already created. Plus it keeps all the CZ requires in place.
Kernel#autoload is not threadsafe. Puma is multi-threaded.
There was a problem hiding this comment.
Does it matter if the module is already created? It'll get created by whichever require opens it first. This is not an expected place to see a require, so if we don't have to do it this way for a specific reason, I'd love it to be at the top.
There was a problem hiding this comment.
It does if you do this:
class ChefZero::Foo
end
in one of those future requires. It's actually a very common pattern.
|
@jkeiser okay, finally it's just the moneta issue. |
I went a little hackyety-hack-hack tonight.