Please review pull request #159: (#12012) Fix bug with overriding LANG environment variable opened by (cprice-puppet)
Description:
There is a yields? method called with_env which is intended to temporarily override environment variables and then restore them to their previous state after executing the caller's block of code. The Facter::Util::Resolution.exec() method uses this, but the yield block contained several occurrences of the "return" statement; this statement breaks out of the rest of the execution of the yields? method, and thus the environment variables were never being restored by the final stanza of the with_env method. This changeset fixes that bug.
Also adds a spec test to reproduce/verify.
- Opened: Wed Jan 25 23:31:38 UTC 2012
- Based on: puppetlabs:master (f16fcfa0945ef8bf2c405a1995aa0d861626ec48)
- Requested merge: cprice-puppet:bug/master/12012-global-ENV-LANG-override (4595c48ff9ebee2b54dd95badab1a402a4788eb1)
Diff follows:
diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index 4913fe2..690a8cb 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -43,8 +43,8 @@ def self.with_env(values)
# set the new (temporary) value for the environment variable
ENV[var] = value
end
- # execute the caller's block
- yield
+ # execute the caller's block, capture the return value
+ rv = yield
# restore the old values
values.each do |var, value|
if old.include?(var)
@@ -54,6 +54,8 @@ def self.with_env(values)
ENV.delete(var)
end
end
+ # return the captured return value
+ rv
end
# Execute a program and return the output of that program.
@@ -87,10 +89,14 @@ def self.exec(code, interpreter = nil)
else
path = %x{which #{binary} 2>/dev/null}.chomp
# we don't have the binary necessary
- return nil if path == "" or path.match(/Command not found\./)
+ # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
+ # without performing the necessary cleanup of environment variables
+ next if path == "" or path.match(/Command not found\./)
end
- return nil unless FileTest.exists?(path)
+ # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
+ # without performing the necessary cleanup of environment variables
+ next unless FileTest.exists?(path)
end
out = nil
@@ -99,16 +105,22 @@ def self.exec(code, interpreter = nil)
out = %x{#{code}}.chomp
rescue Errno::ENOENT => detail
# command not found on Windows
- return nil
+ # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
+ # without performing the necessary cleanup of environment variables
+ next
rescue => detail
$stderr.puts detail
- return nil
+ # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
+ # without performing the necessary cleanup of environment variables
+ next
end
+ # we need to use "next" here, rather than "return"... because "return" would break out of the with_env wrapper
+ # without performing the necessary cleanup of environment variables
if out == ""
- return nil
+ next
else
- return out
+ next out
end
end
diff --git a/spec/unit/util/resolution_spec.rb b/spec/unit/util/resolution_spec.rb
index b775639..4788d45 100755
--- a/spec/unit/util/resolution_spec.rb
+++ b/spec/unit/util/resolution_spec.rb
@@ -336,28 +336,51 @@
# It's not possible, AFAICT, to mock %x{}, so I can't really test this bit.
describe "when executing code" do
+ # set up some command strings, making sure we get the right version for both unix and windows
+ echo_command = Facter::Util::Config.is_windows? ? 'cmd.exe /c "echo foo"' : 'echo foo'
+ echo_env_var_command = Facter::Util::Config.is_windows? ? 'cmd.exe /c "echo %%%s%%"' : 'echo $%s'
+
it "should deprecate the interpreter parameter" do
Facter.expects(:warnonce).with("The interpreter parameter to 'exec' is deprecated and will be removed in a future version.")
Facter::Util::Resolution.exec("/something", "/bin/perl")
end
+ # execute a simple echo command
it "should execute the binary" do
- Facter::Util::Resolution.exec(
- Facter::Util::Config.is_windows? ? 'cmd.exe /c "echo foo"' : 'echo foo'
- ).should == "foo"
+ Facter::Util::Resolution.exec(echo_command).should == "foo"
end
it "should override the LANG environment variable" do
- Facter::Util::Resolution.exec(
- Facter::Util::Config.is_windows? ? 'cmd.exe /c "echo %LANG%"' : 'echo $LANG'
- ).should == "C"
+ Facter::Util::Resolution.exec(echo_env_var_command % 'LANG').should == "C"
end
it "should respect other overridden environment variables" do
Facter::Util::Resolution.with_env( {"FOO" => "foo"} ) do
- Facter::Util::Resolution.exec(
- Facter::Util::Config.is_windows? ? 'cmd.exe /c "echo %FOO%"' : 'echo $FOO'
- ).should == "foo"
+ Facter::Util::Resolution.exec(echo_env_var_command % 'FOO').should == "foo"
+ end
+ end
+
+ it "should restore overridden LANG environment variable after execution" do
+ # we're going to call with_env in a nested fashion, to make sure that the environment gets restored properly
+ # at each level
+ Facter::Util::Resolution.with_env( {"LANG" => "foo"} ) do
+ # Resolution.exec always overrides 'LANG' for its own execution scope
+ Facter::Util::Resolution.exec(echo_env_var_command % 'LANG').should == "C"
+ # But after 'exec' completes, we should see our value restored
+ ENV['LANG'].should == "foo"
+ # Now we'll do a nested call to with_env
+ Facter::Util::Resolution.with_env( {"LANG" => "bar"} ) do
+ # During 'exec' it should still be 'C'
+ Facter::Util::Resolution.exec(echo_env_var_command % 'LANG').should == "C"
+ # After exec it should be restored to our current value for this level of the nesting...
+ ENV['LANG'].should == "bar"
+ end
+ # Now we've dropped out of one level of nesting,
+ ENV['LANG'].should == "foo"
+ # Call exec one more time just for kicks
+ Facter::Util::Resolution.exec(echo_env_var_command % 'LANG').should == "C"
+ # One last check at our current nesting level.
+ ENV['LANG'].should == "foo"
end
end
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected].
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
