Possible fix for #2766 and general approach to deal with stderr

[Peter Meier]

Hi

while looking for a fix for #2766 I realized that facter should have
in general a way to deal with output to stderr. So I changed
Facter::Util::Resolution.exec to wrap the command exection with
Open3.popen3.

Letme know what you think about it. I use it already in production
as I wanted to get rid off all these cron-mails due to facter
printing to stderr.

cheers pete

[Peter Meier]

So far messages to stderr haven't been catched by
Facter::Util::Resolution.exec and were insted printed out to
stderr. This will cause facter and even puppet to print to stderr
themself, which is not very nice when running puppetd by cron,
as you might get every run a mail if a command outputs to stderr.

We are now wrapping the command execution with Open3.popen3 to
catch stderr and logging it to Facter.debug

We are also catching multiline outputs chomping newlines and
returning an array if there have been more than one line. Otherwise
we return an array containing the different lines.

This prevents in general cases as described in #2766 and should
handle command execution in a bit saner way.

Signed-off-by: Peter Meier <peter...@immerda.ch>
---
lib/facter/util/resolution.rb | 23 ++++++++++++++++++-----
spec/unit/util/resolution.rb | 26 ++++++++++++++++++++++++++
2 files changed, 44 insertions(+), 5 deletions(-)

diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index b9e28e8..b9d0cf8 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -7,6 +7,7 @@ require 'facter/util/confine'

require 'timeout'
require 'rbconfig'
+require 'open3'

class Facter::Util::Resolution
attr_accessor :interpreter, :code, :name, :timeout
@@ -42,17 +43,29 @@ class Facter::Util::Resolution
end

out = nil
+ err = nil
begin
- out = %x{#{code}}.chomp
+ Open3.popen3(code) do |stdin,stdout,stderr|
+ out = self.parse_output(stdout.readlines)
+ err = self.parse_output(stderr.readlines)
+ end
rescue => detail
$stderr.puts detail
return nil
end
- if out == ""
- return nil
- else
- return out
+ if err and err != ''
+ Facter.debug("#{code} printed \"#{err}\" to stderr")
end
+
+ return nil if out == ""
+ out
+ end
+
+ def self.parse_output(output)
+ return nil unless output and output.size > 0
+ result = output.collect{|line| line.chomp }
+ return result.first unless result.size > 1
+ result
end

# Add a new confine to the resolution mechanism.
diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb
index d4bb781..2ad7f93 100755
--- a/spec/unit/util/resolution.rb
+++ b/spec/unit/util/resolution.rb
@@ -227,10 +227,36 @@ describe Facter::Util::Resolution do
Facter::Util::Resolution.should respond_to(:exec)
end

+ it "should have a class method to parse output" do
+ Facter::Util::Resolution.should respond_to(:parse_output)
+ end
+
# It's not possible, AFAICT, to mock %x{}, so I can't really test this bit.
describe "when executing code" do
it "should fail if any interpreter other than /bin/sh is requested" do
lambda { Facter::Util::Resolution.exec("/something", "/bin/perl") }.should raise_error(ArgumentError)
end
end
+
+ describe "when parsing output" do
+ it "should return nil on nil" do
+ Facter::Util::Resolution.parse_output(nil).should be_nil
+ end
+
+ it "should return nil on empty string" do
+ Facter::Util::Resolution.parse_output('').should be_nil
+ end
+
+ it "should return nil on an empty array" do
+ Facter::Util::Resolution.parse_output([]).should be_nil
+ end
+
+ it "should return a string on a 1 size array" do
+ Facter::Util::Resolution.parse_output(["aaa\n"]).should == "aaa"
+ end
+
+ it "should return a string on a multi sized array containing a seperator" do
+ result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]
+ end
+ end
end
--
1.6.3.3

[Peter Meier]

> + if err and err != ''
> + Facter.debug("#{code} printed \"#{err}\" to stderr")
> end

I rethought this part: Maybe we should use warn to print the captured
stderr in debug Mode to stderr? So it would be more or less the same
behavior as before but only if we're in debug mode. But maybe printing
stderr to debug is also not that bad, but maybe I should join the array?

cheers pete

[Luke Kanies]

They're pretty similar, but I think I'd prefer 'warn' over 'debug',
given that we don't use debug that much in Facter.

--
Neonle will continue to be rude, and will nretend that you had a small
stroke which makes you unable to say or see the letter "n". Stunid
nractical joke, if you ask me. Bunch of noon-heads, huh?
-- Fred Barling, Humorscope
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

[Luke Kanies]

It'd be nice to have the tests demonstrate that something is done with
stderr, but otherwise the patch looks good.

--
I had a linguistics professor who said that it's man's ability to use
language that makes him the dominant species on the planet. That may
be. But I think there's one other thing that separates us from animals.
We aren't afraid of vacuum cleaners. --Jeff Stilson

[Peter Meier]

>>> + if err and err != ''
>>> + Facter.debug("#{code} printed \"#{err}\" to stderr")
>>> end
>> I rethought this part: Maybe we should use warn to print the captured
>> stderr in debug Mode to stderr? So it would be more or less the same
>> behavior as before but only if we're in debug mode. But maybe printing
>> stderr to debug is also not that bad, but maybe I should join the
>> array?
>
> They're pretty similar, but I think I'd prefer 'warn' over 'debug',
> given that we don't use debug that much in Facter.

fine, I'll do another round with warn and the tests from your other mail.

cheers pete

[Peter Meier]

> It'd be nice to have the tests demonstrate that something is done with
> stderr, but otherwise the patch looks good.

I was thinking about that too, in general I was thinking about mocking
popen3, however this doesn't seem to be trivial, but I can try. Give me
some days.

cheers pete

[Luke Kanies]

It shouldn't be too hard; something like:

it "should produce stderr content as a warning if debug is enabled" do
stdout = stdin = stub 'fh', :readlines => []
stderr = stub 'stderr', :readlines => %w{my content}

Open3.expects(:popen3).with("/my/code").yields(stdin, stdout, stderr)

Resolution.expects(:warn).with { |msg| msg.include?("my content") }

Resolution.exec
end

Something like that anyway. :)

--
This space intentionally has nothing but text explaining why this
space has nothing but text explaining that this space would otherwise
have been left blank, and would otherwise have been left blank.

[Peter Meier]

>>> It'd be nice to have the tests demonstrate that something is done
>>> with
>>> stderr, but otherwise the patch looks good.
>> I was thinking about that too, in general I was thinking about mocking
>> popen3, however this doesn't seem to be trivial, but I can try. Give
>> me
>> some days.
>
> It shouldn't be too hard; something like:
>
> it "should produce stderr content as a warning if debug is enabled" do
> stdout = stdin = stub 'fh', :readlines => []
> stderr = stub 'stderr', :readlines => %w{my content}
>
> Open3.expects(:popen3).with("/my/code").yields(stdin, stdout, stderr)
>
> Resolution.expects(:warn).with { |msg| msg.include?("my content") }
>
> Resolution.exec
> end
>
> Something like that anyway. :)

oh cool. All the other solutions I looked at in the web were completely
different and I even thought more in a different way of testing. But
what I can read from your code seems nice. I'll put something together.

cheers pete

[Markus Roberts]

>  stdout = stdin = stub 'fh', :readlines => []

  stderr = stub 'stderr', :readlines => %w{my content}

Wouldn't using StringIO be a whole lot easier (& significantly less brittle)?

-- Markus
 

[Peter Meier]

So far messages to stderr haven't been catched by
Facter::Util::Resolution.exec and were insted printed out to
stderr. This will cause facter and even puppet to print to stderr
themself, which is not very nice when running puppetd by cron,
as you might get every run a mail if a command outputs to stderr.

We are now wrapping the command execution with Open3.popen3 to

catch stderr and passing them to the new introduced Facter.warn
method.

We are also catching multiline outputs chomping newlines and
returning an array if there have been more than one line. Otherwise
we return an array containing the different lines.

This prevents in general cases as described in #2766 and should
handle command execution in a bit saner way.

Signed-off-by: Peter Meier <peter...@immerda.ch>
---

lib/facter/util/resolution.rb | 23 +++++++++++++++++------
spec/unit/util/resolution.rb | 35 +++++++++++++++++++++++++++++++++++
2 files changed, 52 insertions(+), 6 deletions(-)

diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index b9e28e8..f6afce6 100644

--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -7,6 +7,7 @@ require 'facter/util/confine'

require 'timeout'
require 'rbconfig'
+require 'open3'

class Facter::Util::Resolution
attr_accessor :interpreter, :code, :name, :timeout

@@ -42,17 +43,27 @@ class Facter::Util::Resolution

end

out = nil
+ err = nil
begin
- out = %x{#{code}}.chomp
+ Open3.popen3(code) do |stdin,stdout,stderr|
+ out = self.parse_output(stdout.readlines)
+ err = self.parse_output(stderr.readlines)
+ end
rescue => detail
$stderr.puts detail
return nil
end
- if out == ""
- return nil
- else
- return out

- end
+ Facter.warn(err)

+
+ return nil if out == ""
+ out
+ end
+
+ def self.parse_output(output)
+ return nil unless output and output.size > 0
+ result = output.collect{|line| line.chomp }
+ return result.first unless result.size > 1
+ result
end

# Add a new confine to the resolution mechanism.
diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb

index d4bb781..a2d48a5 100755
--- a/spec/unit/util/resolution.rb
+++ b/spec/unit/util/resolution.rb
@@ -227,10 +227,45 @@ describe Facter::Util::Resolution do

Facter::Util::Resolution.should respond_to(:exec)
end

+ it "should have a class method to parse output" do
+ Facter::Util::Resolution.should respond_to(:parse_output)
+ end
+
# It's not possible, AFAICT, to mock %x{}, so I can't really test this bit.
describe "when executing code" do
it "should fail if any interpreter other than /bin/sh is requested" do

lambda { Facter::Util::Resolution.exec("/something", "/bin/perl") }.should raise_error(ArgumentError)
end

+
+ it "should produce stderr content as a warning if debug is enabled" do
+ stdout = stdin = stub('fh', :readlines => ["aaa\n"])
+ stderr = stub('stderr', :readlines => %w{my content})
+ Open3.expects(:popen3).with("/bin/true").yields(stdin, stdout, stderr)
+
+ Facter.expects(:warn).with(['my','content'])
+ Facter::Util::Resolution.exec("/bin/true").should == 'aaa'
+ end
+ end

+
+ describe "when parsing output" do
+ it "should return nil on nil" do
+ Facter::Util::Resolution.parse_output(nil).should be_nil
+ end
+
+ it "should return nil on empty string" do
+ Facter::Util::Resolution.parse_output('').should be_nil
+ end
+
+ it "should return nil on an empty array" do
+ Facter::Util::Resolution.parse_output([]).should be_nil
+ end
+
+ it "should return a string on a 1 size array" do
+ Facter::Util::Resolution.parse_output(["aaa\n"]).should == "aaa"
+ end
+
+ it "should return a string on a multi sized array containing a seperator" do
+ result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]
+ end

end
end
--
1.6.3.3

[Peter Meier]

this one unfortunately started off too fast. Please look only at the 2
others sent with the summary email.

cheers pete

[Paul Nasrat]

2009/11/6 Peter Meier <peter...@immerda.ch>:

> We are now wrapping the command execution with Open3.popen3 to
> catch stderr and passing them to the new introduced Facter.warn
> method.

Cool.

> We are also catching multiline outputs chomping newlines and
> returning an array if there have been more than one line. Otherwise

> we return an array containing the different lines.

> +        it "should return a string on a multi sized array containing a seperator" do
> +            result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]
> +        end
>     end
>  end

The test doesn't test what you say it should. Either the test name is
wrong (it should return an array ...) or the expectation is wrong.
Based on your earlier comments I'd say it's the test name.

Paul

[Peter Meier]

>> We are also catching multiline outputs chomping newlines and
>> returning an array if there have been more than one line. Otherwise
>
>> we return an array containing the different lines.
>> + it "should return a string on a multi sized array containing a seperator" do
>> + result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]
>> + end
>> end
>> end
>
> The test doesn't test what you say it should. Either the test name is
> wrong (it should return an array ...) or the expectation is wrong.
> Based on your earlier comments I'd say it's the test name.

err right. My bad. This was the name of the test for a first way I
wanted to solve multlines from execution.

As the branch seems already pushed I do an additional commit to adjust
the test name. Ok?

cheers pete

[Peter Meier]

The description was wrong and didn't explain what is actually
tested.

Signed-off-by: Peter Meier <peter...@immerda.ch>
---

spec/unit/util/resolution.rb | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/spec/unit/util/resolution.rb b/spec/unit/util/resolution.rb
index fcdc04c..27cb150 100755
--- a/spec/unit/util/resolution.rb
+++ b/spec/unit/util/resolution.rb
@@ -273,7 +273,7 @@ describe Facter::Util::Resolution do
Facter::Util::Resolution.parse_output(["aaa\n"]).should == "aaa"
end

- it "should return a string on a multi sized array containing a seperator" do
+ it "should return an array with chomped new lines on an array" do

result = Facter::Util::Resolution.parse_output(["aaa\n","bbb\n","ccc\n"]).should == [ "aaa", "bbb", "ccc" ]

end
end
--
1.6.3.3

[Peter Meier]

pushed to the existing branch.