Please review pull request #149: Bug/master/12012 global env lang override opened by (cprice-puppet)
Description:
Fix for Facter ticket #12012; basically stop globally overriding the LANG environment variable whenever facter.rb gets 'require'd.
- Opened: Fri Jan 20 01:15:46 UTC 2012
- Based on: puppetlabs:master (4b0c12bf5965e998c48a2b4d66676f0d7a561168)
- Requested merge: cprice-puppet:bug/master/12012-global-ENV-LANG-override (810c465653869c6cacca009aaa477ca389314456)
Diff follows:
diff --git a/lib/facter.rb b/lib/facter.rb
index 5cbf74d..ec7827d 100644
--- a/lib/facter.rb
+++ b/lib/facter.rb
@@ -39,10 +39,6 @@ module Util; end
# puts Facter['operatingsystem']
#
- # Set LANG to force i18n to C
- #
- ENV['LANG'] = 'C'
-
GREEN = " [0;32m"
RESET = " [0m"
@@debug = 0
diff --git a/lib/facter/util/resolution.rb b/lib/facter/util/resolution.rb
index dc58cb5..4913fe2 100644
--- a/lib/facter/util/resolution.rb
+++ b/lib/facter/util/resolution.rb
@@ -26,6 +26,36 @@ def self.have_which
@have_which
end
+ #
+ # Call this method with a block of code for which you would like to temporarily modify
+ # one or more environment variables; the specified values will be set for the duration
+ # of your block, after which the original values (if any) will be restored.
+ #
+ # [values] a Hash containing the key/value pairs of any environment variables that you
+ # would like to temporarily override
+ def self.with_env(values)
+ old = {}
+ values.each do |var, value|
+ # save the old value if it exists
+ if old_val = ENV[var]
+ old[var] = old_val
+ end
+ # set the new (temporary) value for the environment variable
+ ENV[var] = value
+ end
+ # execute the caller's block
+ yield
+ # restore the old values
+ values.each do |var, value|
+ if old.include?(var)
+ ENV[var] = old[var]
+ else
+ # if there was no old value, delete the key from the current environment variables hash
+ ENV.delete(var)
+ end
+ end
+ end
+
# Execute a program and return the output of that program.
#
# Returns nil if the program can't be found, or if there is a problem
@@ -34,47 +64,55 @@ def self.have_which
def self.exec(code, interpreter = nil)
Facter.warnonce "The interpreter parameter to 'exec' is deprecated and will be removed in a future version." if interpreter
- # Try to guess whether the specified code can be executed by looking at the
- # first word. If it cannot be found on the PATH defer on resolving the fact
- # by returning nil.
- # This only fails on shell built-ins, most of which are masked by stuff in
- # /bin or of dubious value anyways. In the worst case, "sh -c 'builtin'" can
- # be used to work around this limitation
- #
- # Windows' %x{} throws Errno::ENOENT when the command is not found, so we
- # can skip the check there. This is good, since builtins cannot be found
- # elsewhere.
- if have_which and !Facter::Util::Config.is_windows?
- path = nil
- binary = code.split.first
- if code =~ /^\//
- path = binary
- 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\./)
+
+ ## Set LANG to force i18n to C for the duration of this exec; this ensures that any code that parses the
+ ## output of the command can expect it to be in a consistent / predictable format / locale
+ with_env "LANG" => "C" do
+
+ # Try to guess whether the specified code can be executed by looking at the
+ # first word. If it cannot be found on the PATH defer on resolving the fact
+ # by returning nil.
+ # This only fails on shell built-ins, most of which are masked by stuff in
+ # /bin or of dubious value anyways. In the worst case, "sh -c 'builtin'" can
+ # be used to work around this limitation
+ #
+ # Windows' %x{} throws Errno::ENOENT when the command is not found, so we
+ # can skip the check there. This is good, since builtins cannot be found
+ # elsewhere.
+ if have_which and !Facter::Util::Config.is_windows?
+ path = nil
+ binary = code.split.first
+ if code =~ /^\//
+ path = binary
+ 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\./)
+ end
+
+ return nil unless FileTest.exists?(path)
end
- return nil unless FileTest.exists?(path)
- end
+ out = nil
- out = nil
+ begin
+ out = %x{#{code}}.chomp
+ rescue Errno::ENOENT => detail
+ # command not found on Windows
+ return nil
+ rescue => detail
+ $stderr.puts detail
+ return nil
+ end
- begin
- out = %x{#{code}}.chomp
- rescue Errno::ENOENT => detail
- # command not found on Windows
- return nil
- rescue => detail
- $stderr.puts detail
- return nil
- end
+ if out == ""
+ return nil
+ else
+ return out
+ end
- if out == ""
- return nil
- else
- return out
end
+
end
# Add a new confine to the resolution mechanism.
diff --git a/spec/unit/util/loader_spec.rb b/spec/unit/util/loader_spec.rb
index 3a83f16..7653484 100755
--- a/spec/unit/util/loader_spec.rb
+++ b/spec/unit/util/loader_spec.rb
@@ -21,24 +21,6 @@ def load_file(file)
Facter::Util::Loader.any_instance.unstub(:load_all)
end
- def with_env(values)
- old = {}
- values.each do |var, value|
- if old_val = ENV[var]
- old[var] = old_val
- end
- ENV[var] = value
- end
- yield
- values.each do |var, value|
- if old.include?(var)
- ENV[var] = old[var]
- else
- ENV.delete(var)
- end
- end
- end
-
it "should have a method for loading individual facts by name" do
Facter::Util::Loader.new.should respond_to(:load)
end
@@ -76,7 +58,7 @@ def with_env(values)
describe "and the FACTERLIB environment variable is set" do
it "should include all paths in FACTERLIB" do
- with_env "FACTERLIB" => "/one/path:/two/path" do
+ Facter::Util::Resolution.with_env "FACTERLIB" => "/one/path:/two/path" do
paths = @loader.search_path
%w{/one/path /two/path}.each do |dir|
paths.should be_include(dir)
@@ -95,7 +77,7 @@ def with_env(values)
it "should load values from the matching environment variable if one is present" do
Facter.expects(:add).with("testing")
- with_env "facter_testing" => "yayness" do
+ Facter::Util::Resolution.with_env "facter_testing" => "yayness" do
@loader.load(:testing)
end
end
@@ -267,7 +249,7 @@ def with_env(values)
Facter.expects(:add).with('one')
Facter.expects(:add).with('two')
- with_env "facter_one" => "yayness", "facter_two" => "boo" do
+ Facter::Util::Resolution.with_env "facter_one" => "yayness", "facter_two" => "boo" do
@loader.load_all
end
end
@@ -281,7 +263,7 @@ def with_env(values)
it "should load facts on the facter search path only once" do
facterlibdir = File.expand_path(File.dirname(__FILE__) + '../../../fixtures/unit/util/loader')
- with_env 'FACTERLIB' => facterlibdir do
+ Facter::Util::Resolution.with_env 'FACTERLIB' => facterlibdir do
Facter::Util::Loader.new.load_all
Facter.value(:nosuchfact).should be_nil
end
-- You received this message because you are subscribed to the Google Groups "Puppet Developers" group.
To post to this group, send email to puppet-dev@googlegroups.com.
To unsubscribe from this group, send email to puppet-dev+unsubscr...@googlegroups.com.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
