If the OS allows it, build the docs (ri and rdoc) in a forked background process #352

Merged
merged 4 commits into from Jul 6, 2012
View
@@ -7,4 +7,5 @@
/pkg
/tmp
/misc
-/notes.txt
+/notes.txt
+/.idea
@@ -32,6 +32,7 @@ class Gem::DependencyInstaller
:prerelease => false,
:security_policy => nil, # HACK NoSecurity requires OpenSSL. AlmostNo? Low?
:wrappers => true,
+ :build_docs_in_background => true,
}.freeze
##
@@ -77,6 +78,7 @@ def initialize(options = {})
@security_policy = options[:security_policy]
@user_install = options[:user_install]
@wrappers = options[:wrappers]
+ @build_docs_in_background = options[:build_docs_in_background]
# Indicates that we should not try to update any deps unless
# we absolutely must.
@@ -369,10 +371,30 @@ def install dep_or_name, version = Gem::Requirement.default
@installed_gems << spec
end
- Gem.done_installing_hooks.each do |hook|
- hook.call self, @installed_gems
- end
+ # Since this is currently only called for docs, we can be lazy and just say
+ # it's documentation. Ideally the hook adder could decide whether to be in
+ # the background or not, and what to call it.
+ in_background "Installing documentation" do
+ start = Time.now
+ Gem.done_installing_hooks.each do |hook|
+ hook.call self, @installed_gems
+ end
+ finish = Time.now
+ say "Done installing documentation for #{@installed_gems.map(&:name).join(', ')} (#{(finish-start).to_i} sec)."
+ end unless Gem.done_installing_hooks.empty?
@installed_gems
end
+
+ def in_background what
+ if @build_docs_in_background and Process.respond_to?(:fork)
+ say "#{what} in a background process."
+ Process.fork do
+ yield
+ end
+ else
+ yield
+ end
+ end
+
end
@@ -304,6 +304,8 @@ def test_execute_rdoc
assert_equal 0, e.exit_code
end
+ Process.wait rescue Errno::ECHILD if Process.respond_to?(:fork)
@bhenderson
bhenderson Jul 6, 2012

what's the Errno::ECHILD for? It seems like this line isn't doing what it was intended to do. (same for line 277)

@alexch
alexch Jul 7, 2012 Contributor

If the child process has already exited, Process.wait will raise it. Note that the ri docs for Process.wait say it will raise a SystemError but actually, Errno::ECHILD < SystemCallError < StandardError (tested under 1.9.3 and 1.8.7). You can verify this by simply calling Process.wait from irb.

@alexch
alexch Jul 7, 2012 Contributor

To be more precise, the line is to wait for the process to exit, but not to fail if either there never was a child process forked, or if the process has already been reaped by an earlier Process.wait.

@bhenderson
bhenderson Jul 7, 2012

thanks @alexch for getting back to me. What I mean is that when you rescue like that, you're describing the return value, not the Exception class to rescue.

>> Process.wait rescue Errno::ECHILD
=> Errno::ECHILD
>> Process.wait rescue 'return value'
=> "return value"

also,

>> begin Process.wait; rescue; Errno::ECHILD end
=> Errno::ECHILD
>> begin Process.wait; rescue Errno::ECHILD; end
=> nil

(I only have 1.8.7 in front of me to test, but I'm pretty sure 1.9.x does the same)

I realize this is a very minor issue. I do think it would be nice if ruby allowed for specifying the exception class in this way.

thanks for your time.

@alexch
alexch Jul 7, 2012 Contributor

Ouch! You just dropped some knowledge on me.

You're right; as far as I can tell the one-line rescue's argument is
merely a return value, not a specifier. I'll make a new PR to fix that
(since in the rare case Process.wait raises a different exception
it'd be nice not to swallow it).

On Sat, Jul 7, 2012 at 9:41 AM, Brian Henderson
reply@reply.github.com
wrote:

@@ -304,6 +304,8 @@ def test_execute_rdoc
assert_equal 0, e.exit_code
end

  • Process.wait rescue Errno::ECHILD if Process.respond_to?(:fork)

thanks @alexch for getting back to me. What I mean is that when you rescue like that, you're describing the return value, not the Exception class to rescue.

>> Process.wait rescue Errno::ECHILD
=> Errno::ECHILD
>> Process.wait rescue 'return value'
=> "return value"

also,

>> begin Process.wait; rescue; Errno::ECHILD end
=> Errno::ECHILD
>> begin Process.wait; rescue Errno::ECHILD; end
=> nil

(I only have 1.8.7 in front of me to test, but I'm pretty sure 1.9.x does the same)

I realize this is a very minor issue. I do think it would be nice if ruby allowed for specifying the exception class in this way.

thanks for your time.


Reply to this email directly or view it on GitHub:
https://github.com/rubygems/rubygems/pull/352/files#r1118440

Alex Chaffee - alex@stinky.com
http://alexchaffee.com
http://twitter.com/alexch

@zenspider
zenspider Jul 7, 2012 Contributor

applied

+
assert_path_exists File.join(@a2.doc_dir, 'ri')
assert_path_exists File.join(@a2.doc_dir, 'rdoc')
end
@@ -274,6 +274,8 @@ def test_execute_rdoc
@cmd.execute
end
+ Process.wait rescue Errno::ECHILD if Process.respond_to?(:fork)
+
assert_path_exists File.join(@a2.doc_dir, 'ri')
assert_path_exists File.join(@a2.doc_dir, 'rdoc')
end
@@ -187,7 +187,7 @@ def test_install_dependency
FileUtils.mv @b1_gem, @tempdir
Dir.chdir @tempdir do
- inst = Gem::DependencyInstaller.new
+ inst = Gem::DependencyInstaller.new(:build_docs_in_background => false)
inst.install 'b'
end