From c63e9c2394a30fe653908cd15967218d90fa34d6 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 2 May 2011 13:57:57 -0700
Subject: maint: reset more global state in testing faces...

When we query for all faces we need to scan the entire Ruby load path, look
for everything that looks like a face, and load it up.  That is a fairly
expensive operation, especially on a platform that has slow I/O.

*cough* EC2 *cough*

Because of that we only scan once, and assume that the list is static
thereafter; this works OK out in the field, but sucks in testing where that
global state gets in the way of the rest of our fiddling under the hood.

This resets the '@loaded' member of the collection additionally, which is what
should be done since we have reset the rest of the collection at the same
time.

We don't bother to reset it, as an extra scan during tests is not a problem.

Reviewed-By: Nick Lewis <nick@puppetlabs.com>
---
 spec/unit/interface/face_collection_spec.rb | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 890e06a9e..4358ef4b6 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -16,11 +16,12 @@ describe Puppet::Interface::FaceCollection do
     @original_faces    = subject.instance_variable_get("@faces").dup
     @original_required = $".dup
     $".delete_if do |path| path =~ %r{/face/.*\.rb$} end
-    subject.instance_variable_get("@faces").clear
+    subject.instance_variable_get(:@faces).clear
+    subject.instance_variable_set(:@loaded, false)
   end
 
   after :each do
-    subject.instance_variable_set("@faces", @original_faces)
+    subject.instance_variable_set(:@faces, @original_faces)
     $".clear ; @original_required.each do |item| $" << item end
   end
 
-- 
cgit 


From 86c6ec24f387fc70abc333fc4ac974b06b3ec80a Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 2 May 2011 14:31:54 -0700
Subject: maint: move the indirector face base out of puppet/face

We used to shove the base class Puppet::Face::Indirector next to the actual
faces; this made a bunch of things, including testing, confusing.  Instead,
move it away into the indirector where it lives with the rest of the
indirector related things.

Reviewed-By: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/face/catalog.rb                     |  4 +-
 lib/puppet/face/certificate.rb                 |  4 +-
 lib/puppet/face/certificate_request.rb         |  4 +-
 lib/puppet/face/certificate_revocation_list.rb |  4 +-
 lib/puppet/face/facts.rb                       |  4 +-
 lib/puppet/face/file.rb                        |  4 +-
 lib/puppet/face/indirector.rb                  | 95 --------------------------
 lib/puppet/face/key.rb                         |  4 +-
 lib/puppet/face/node.rb                        |  4 +-
 lib/puppet/face/report.rb                      |  4 +-
 lib/puppet/face/resource.rb                    |  4 +-
 lib/puppet/face/resource_type.rb               |  4 +-
 lib/puppet/face/status.rb                      |  4 +-
 lib/puppet/indirector/face.rb                  | 94 +++++++++++++++++++++++++
 spec/unit/application/indirection_base_spec.rb |  4 +-
 spec/unit/face/indirector_spec.rb              | 59 ----------------
 spec/unit/indirector/face_spec.rb              | 59 ++++++++++++++++
 17 files changed, 179 insertions(+), 180 deletions(-)
 delete mode 100644 lib/puppet/face/indirector.rb
 create mode 100644 lib/puppet/indirector/face.rb
 delete mode 100755 spec/unit/face/indirector_spec.rb
 create mode 100755 spec/unit/indirector/face_spec.rb

diff --git a/lib/puppet/face/catalog.rb b/lib/puppet/face/catalog.rb
index 98f550413..5f1f138ee 100644
--- a/lib/puppet/face/catalog.rb
+++ b/lib/puppet/face/catalog.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:catalog, '0.0.1') do
+Puppet::Indirector::Face.define(:catalog, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/certificate.rb b/lib/puppet/face/certificate.rb
index 7f2998da2..0018c5fd3 100644
--- a/lib/puppet/face/certificate.rb
+++ b/lib/puppet/face/certificate.rb
@@ -1,7 +1,7 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 require 'puppet/ssl/host'
 
-Puppet::Face::Indirector.define(:certificate, '0.0.1') do
+Puppet::Indirector::Face.define(:certificate, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/certificate_request.rb b/lib/puppet/face/certificate_request.rb
index 0f7f72205..809758423 100644
--- a/lib/puppet/face/certificate_request.rb
+++ b/lib/puppet/face/certificate_request.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:certificate_request, '0.0.1') do
+Puppet::Indirector::Face.define(:certificate_request, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/certificate_revocation_list.rb b/lib/puppet/face/certificate_revocation_list.rb
index 9a8fe303d..9913fad4b 100644
--- a/lib/puppet/face/certificate_revocation_list.rb
+++ b/lib/puppet/face/certificate_revocation_list.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:certificate_revocation_list, '0.0.1') do
+Puppet::Indirector::Face.define(:certificate_revocation_list, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/facts.rb b/lib/puppet/face/facts.rb
index 88e3c7ba9..9add63dd0 100644
--- a/lib/puppet/face/facts.rb
+++ b/lib/puppet/face/facts.rb
@@ -1,7 +1,7 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 require 'puppet/node/facts'
 
-Puppet::Face::Indirector.define(:facts, '0.0.1') do
+Puppet::Indirector::Face.define(:facts, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/file.rb b/lib/puppet/face/file.rb
index 1b2e62b6d..1f2fc9f55 100644
--- a/lib/puppet/face/file.rb
+++ b/lib/puppet/face/file.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:file, '0.0.1') do
+Puppet::Indirector::Face.define(:file, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/indirector.rb b/lib/puppet/face/indirector.rb
deleted file mode 100644
index 16ffcd311..000000000
--- a/lib/puppet/face/indirector.rb
+++ /dev/null
@@ -1,95 +0,0 @@
-require 'puppet'
-require 'puppet/face'
-
-class Puppet::Face::Indirector < Puppet::Face
-  option "--terminus TERMINUS" do
-    description %q{
-REVISIT: You can select a terminus, which has some bigger effect
-that we should describe in this file somehow.
-}.strip
-
-    before_action do |action, args, options|
-      set_terminus(options[:terminus])
-    end
-
-    after_action do |action, args, options|
-      indirection.reset_terminus_class
-    end
-  end
-
-  def self.indirections
-    Puppet::Indirector::Indirection.instances.collect { |t| t.to_s }.sort
-  end
-
-  def self.terminus_classes(indirection)
-    Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
-  end
-
-  def call_indirection_method(method, key, options)
-    begin
-      result = indirection.__send__(method, key, options)
-    rescue => detail
-      puts detail.backtrace if Puppet[:trace]
-      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
-    end
-
-    return result
-  end
-
-  action :destroy do
-    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
-  end
-
-  action :find do
-    when_invoked { |key, options| call_indirection_method(:find, key, options) }
-  end
-
-  action :save do
-    when_invoked { |key, options| call_indirection_method(:save, key, options) }
-  end
-
-  action :search do
-    when_invoked { |key, options| call_indirection_method(:search, key, options) }
-  end
-
-  # Print the configuration for the current terminus class
-  action :info do
-    when_invoked do |*args|
-      if t = indirection.terminus_class
-        puts "Run mode '#{Puppet.run_mode.name}': #{t}"
-      else
-        $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
-      end
-    end
-  end
-
-  attr_accessor :from
-
-  def indirection_name
-    @indirection_name || name.to_sym
-  end
-
-  # Here's your opportunity to override the indirection name.  By default it
-  # will be the same name as the face.
-  def set_indirection_name(name)
-    @indirection_name = name
-  end
-
-  # Return an indirection associated with a face, if one exists;
-  # One usually does.
-  def indirection
-    unless @indirection
-      @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
-      @indirection or raise "Could not find terminus for #{indirection_name}"
-    end
-    @indirection
-  end
-
-  def set_terminus(from)
-    begin
-      indirection.terminus_class = from
-    rescue => detail
-      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{self.class.terminus_classes(indirection.name).join(", ") }"
-    end
-  end
-end
diff --git a/lib/puppet/face/key.rb b/lib/puppet/face/key.rb
index 5d1a9ab26..148dc06c2 100644
--- a/lib/puppet/face/key.rb
+++ b/lib/puppet/face/key.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:key, '0.0.1') do
+Puppet::Indirector::Face.define(:key, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/node.rb b/lib/puppet/face/node.rb
index 3591dd92b..c4cf30b98 100644
--- a/lib/puppet/face/node.rb
+++ b/lib/puppet/face/node.rb
@@ -1,5 +1,5 @@
-require 'puppet/face/indirector'
-Puppet::Face::Indirector.define(:node, '0.0.1') do
+require 'puppet/indirector/face'
+Puppet::Indirector::Face.define(:node, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/report.rb b/lib/puppet/face/report.rb
index 9855f3d2d..dabf83702 100644
--- a/lib/puppet/face/report.rb
+++ b/lib/puppet/face/report.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:report, '0.0.1') do
+Puppet::Indirector::Face.define(:report, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/resource.rb b/lib/puppet/face/resource.rb
index 55a14f280..9ded46c0c 100644
--- a/lib/puppet/face/resource.rb
+++ b/lib/puppet/face/resource.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:resource, '0.0.1') do
+Puppet::Indirector::Face.define(:resource, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/resource_type.rb b/lib/puppet/face/resource_type.rb
index 8776dc105..648cf1191 100644
--- a/lib/puppet/face/resource_type.rb
+++ b/lib/puppet/face/resource_type.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:resource_type, '0.0.1') do
+Puppet::Indirector::Face.define(:resource_type, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/face/status.rb b/lib/puppet/face/status.rb
index d35d7e1b3..7ef05fa0e 100644
--- a/lib/puppet/face/status.rb
+++ b/lib/puppet/face/status.rb
@@ -1,6 +1,6 @@
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-Puppet::Face::Indirector.define(:status, '0.0.1') do
+Puppet::Indirector::Face.define(:status, '0.0.1') do
   copyright "Puppet Labs", 2011
   license   "Apache 2 license; see COPYING"
 
diff --git a/lib/puppet/indirector/face.rb b/lib/puppet/indirector/face.rb
new file mode 100644
index 000000000..0fd44dfea
--- /dev/null
+++ b/lib/puppet/indirector/face.rb
@@ -0,0 +1,94 @@
+require 'puppet/face'
+
+class Puppet::Indirector::Face < Puppet::Face
+  option "--terminus TERMINUS" do
+    description %q{
+REVISIT: You can select a terminus, which has some bigger effect
+that we should describe in this file somehow.
+}.strip
+
+    before_action do |action, args, options|
+      set_terminus(options[:terminus])
+    end
+
+    after_action do |action, args, options|
+      indirection.reset_terminus_class
+    end
+  end
+
+  def self.indirections
+    Puppet::Indirector::Indirection.instances.collect { |t| t.to_s }.sort
+  end
+
+  def self.terminus_classes(indirection)
+    Puppet::Indirector::Terminus.terminus_classes(indirection.to_sym).collect { |t| t.to_s }.sort
+  end
+
+  def call_indirection_method(method, key, options)
+    begin
+      result = indirection.__send__(method, key, options)
+    rescue => detail
+      puts detail.backtrace if Puppet[:trace]
+      raise "Could not call '#{method}' on '#{indirection_name}': #{detail}"
+    end
+
+    return result
+  end
+
+  action :destroy do
+    when_invoked { |key, options| call_indirection_method(:destroy, key, options) }
+  end
+
+  action :find do
+    when_invoked { |key, options| call_indirection_method(:find, key, options) }
+  end
+
+  action :save do
+    when_invoked { |key, options| call_indirection_method(:save, key, options) }
+  end
+
+  action :search do
+    when_invoked { |key, options| call_indirection_method(:search, key, options) }
+  end
+
+  # Print the configuration for the current terminus class
+  action :info do
+    when_invoked do |*args|
+      if t = indirection.terminus_class
+        puts "Run mode '#{Puppet.run_mode.name}': #{t}"
+      else
+        $stderr.puts "No default terminus class for run mode '#{Puppet.run_mode.name}'"
+      end
+    end
+  end
+
+  attr_accessor :from
+
+  def indirection_name
+    @indirection_name || name.to_sym
+  end
+
+  # Here's your opportunity to override the indirection name.  By default it
+  # will be the same name as the face.
+  def set_indirection_name(name)
+    @indirection_name = name
+  end
+
+  # Return an indirection associated with a face, if one exists;
+  # One usually does.
+  def indirection
+    unless @indirection
+      @indirection = Puppet::Indirector::Indirection.instance(indirection_name)
+      @indirection or raise "Could not find terminus for #{indirection_name}"
+    end
+    @indirection
+  end
+
+  def set_terminus(from)
+    begin
+      indirection.terminus_class = from
+    rescue => detail
+      raise "Could not set '#{indirection.name}' terminus to '#{from}' (#{detail}); valid terminus types are #{self.class.terminus_classes(indirection.name).join(", ") }"
+    end
+  end
+end
diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb
index e0a9bebb4..910774c14 100755
--- a/spec/unit/application/indirection_base_spec.rb
+++ b/spec/unit/application/indirection_base_spec.rb
@@ -1,14 +1,14 @@
 #!/usr/bin/env rspec
 require 'spec_helper'
 require 'puppet/application/indirection_base'
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
 ########################################################################
 # Stub for testing; the names are critical, sadly. --daniel 2011-03-30
 class Puppet::Application::TestIndirection < Puppet::Application::IndirectionBase
 end
 
-face = Puppet::Face::Indirector.define(:testindirection, '0.0.1') do
+face = Puppet::Indirector::Face.define(:testindirection, '0.0.1') do
   summary "fake summary"
 end
 # REVISIT: This horror is required because we don't allow anything to be
diff --git a/spec/unit/face/indirector_spec.rb b/spec/unit/face/indirector_spec.rb
deleted file mode 100755
index e7dd44f3d..000000000
--- a/spec/unit/face/indirector_spec.rb
+++ /dev/null
@@ -1,59 +0,0 @@
-#!/usr/bin/env rspec
-require 'spec_helper'
-require 'puppet/face/indirector'
-
-describe Puppet::Face::Indirector do
-  subject do
-    instance = Puppet::Face::Indirector.new(:test, '0.0.1')
-    indirection = stub('indirection',
-                       :name => :stub_indirection,
-                       :reset_terminus_class => nil)
-    instance.stubs(:indirection).returns indirection
-    instance
-  end
-
-  it "should be able to return a list of indirections" do
-    Puppet::Face::Indirector.indirections.should be_include("catalog")
-  end
-
-  it "should be able to return a list of terminuses for a given indirection" do
-    Puppet::Face::Indirector.terminus_classes(:catalog).should be_include("compiler")
-  end
-
-  describe "as an instance" do
-    it "should be able to determine its indirection" do
-      # Loading actions here an get, um, complicated
-      Puppet::Face.stubs(:load_actions)
-      Puppet::Face::Indirector.new(:catalog, '0.0.1').indirection.should equal(Puppet::Resource::Catalog.indirection)
-    end
-  end
-
-  [:find, :search, :save, :destroy].each do |method|
-    it "should define a '#{method}' action" do
-      Puppet::Face::Indirector.should be_action(method)
-    end
-
-    it "should call the indirection method with options when the '#{method}' action is invoked" do
-      subject.indirection.expects(method).with(:test, {})
-      subject.send(method, :test)
-    end
-    it "should forward passed options" do
-      subject.indirection.expects(method).with(:test, {'one'=>'1'})
-      subject.send(method, :test, {'one'=>'1'})
-    end
-  end
-
-  it "should be able to override its indirection name" do
-    subject.set_indirection_name :foo
-    subject.indirection_name.should == :foo
-  end
-
-  it "should be able to set its terminus class" do
-    subject.indirection.expects(:terminus_class=).with(:myterm)
-    subject.set_terminus(:myterm)
-  end
-
-  it "should define a class-level 'info' action" do
-    Puppet::Face::Indirector.should be_action(:info)
-  end
-end
diff --git a/spec/unit/indirector/face_spec.rb b/spec/unit/indirector/face_spec.rb
new file mode 100755
index 000000000..1530f7270
--- /dev/null
+++ b/spec/unit/indirector/face_spec.rb
@@ -0,0 +1,59 @@
+#!/usr/bin/env rspec
+require 'spec_helper'
+require 'puppet/indirector/face'
+
+describe Puppet::Indirector::Face do
+  subject do
+    instance = Puppet::Indirector::Face.new(:test, '0.0.1')
+    indirection = stub('indirection',
+                       :name => :stub_indirection,
+                       :reset_terminus_class => nil)
+    instance.stubs(:indirection).returns indirection
+    instance
+  end
+
+  it "should be able to return a list of indirections" do
+    Puppet::Indirector::Face.indirections.should be_include("catalog")
+  end
+
+  it "should be able to return a list of terminuses for a given indirection" do
+    Puppet::Indirector::Face.terminus_classes(:catalog).should be_include("compiler")
+  end
+
+  describe "as an instance" do
+    it "should be able to determine its indirection" do
+      # Loading actions here an get, um, complicated
+      Puppet::Face.stubs(:load_actions)
+      Puppet::Indirector::Face.new(:catalog, '0.0.1').indirection.should equal(Puppet::Resource::Catalog.indirection)
+    end
+  end
+
+  [:find, :search, :save, :destroy].each do |method|
+    it "should define a '#{method}' action" do
+      Puppet::Indirector::Face.should be_action(method)
+    end
+
+    it "should call the indirection method with options when the '#{method}' action is invoked" do
+      subject.indirection.expects(method).with(:test, {})
+      subject.send(method, :test)
+    end
+    it "should forward passed options" do
+      subject.indirection.expects(method).with(:test, {'one'=>'1'})
+      subject.send(method, :test, {'one'=>'1'})
+    end
+  end
+
+  it "should be able to override its indirection name" do
+    subject.set_indirection_name :foo
+    subject.indirection_name.should == :foo
+  end
+
+  it "should be able to set its terminus class" do
+    subject.indirection.expects(:terminus_class=).with(:myterm)
+    subject.set_terminus(:myterm)
+  end
+
+  it "should define a class-level 'info' action" do
+    Puppet::Indirector::Face.should be_action(:info)
+  end
+end
-- 
cgit 


From 1b42725b5caab6f8e457e11fec2488fbe94e8e43 Mon Sep 17 00:00:00 2001
From: Daniel Pittman <daniel@puppetlabs.com>
Date: Mon, 2 May 2011 15:14:25 -0700
Subject: (#7314) Faces fail horribly when one has a syntax error.

When we hit a syntax error in any face, a whole bunch of unrelated face things
would blow up in horrible ways.  Stack traces for all...

Now, instead, we catch that fault   but specifically only in the face file
and report it through our error logs, then quietly ignore the face.

Reviewed-By: Nick Lewis <nick@puppetlabs.com>
---
 lib/puppet/interface/face_collection.rb         | 21 ++++++++-------------
 spec/fixtures/faulty_face/puppet/face/syntax.rb |  8 ++++++++
 spec/unit/interface/face_collection_spec.rb     | 19 ++++++++++++++++++-
 3 files changed, 34 insertions(+), 14 deletions(-)
 create mode 100644 spec/fixtures/faulty_face/puppet/face/syntax.rb

diff --git a/lib/puppet/interface/face_collection.rb b/lib/puppet/interface/face_collection.rb
index 6e6afc545..baa424692 100644
--- a/lib/puppet/interface/face_collection.rb
+++ b/lib/puppet/interface/face_collection.rb
@@ -10,21 +10,12 @@ module Puppet::Interface::FaceCollection
     unless @loaded
       @loaded = true
       $LOAD_PATH.each do |dir|
-        next unless FileTest.directory?(dir)
-        Dir.chdir(dir) do
-          Dir.glob("puppet/face/*.rb").collect { |f| f.sub(/\.rb/, '') }.each do |file|
-            iname = file.sub(/\.rb/, '')
-            begin
-              require iname
-            rescue Exception => detail
-              puts detail.backtrace if Puppet[:trace]
-              raise "Could not load #{iname} from #{dir}/#{file}: #{detail}"
-            end
-          end
-        end
+        Dir.glob("#{dir}/puppet/face/*.rb").
+          collect {|f| File.basename(f, '.rb') }.
+          each {|name| self[name, :current] }
       end
     end
-    return @faces.keys.select {|name| @faces[name].length > 0 }
+    @faces.keys.select {|name| @faces[name].length > 0 }
   end
 
   def self.validate_version(version)
@@ -124,6 +115,10 @@ module Puppet::Interface::FaceCollection
     rescue LoadError => e
       raise unless e.message =~ %r{-- puppet/face/#{name}$}
       # ...guess we didn't find the file; return a much better problem.
+    rescue SyntaxError => e
+      raise unless e.message =~ %r{puppet/face/#{name}\.rb:\d+: }
+      Puppet.err "Failed to load face #{name}:\n#{e}"
+      # ...but we just carry on after complaining.
     end
 
     return get_face(name, version)
diff --git a/spec/fixtures/faulty_face/puppet/face/syntax.rb b/spec/fixtures/faulty_face/puppet/face/syntax.rb
new file mode 100644
index 000000000..3b1e36c3f
--- /dev/null
+++ b/spec/fixtures/faulty_face/puppet/face/syntax.rb
@@ -0,0 +1,8 @@
+Puppet::Face.define(:syntax, '1.0.0') do
+  action :foo do
+    when_invoked do |whom|
+      "hello, #{whom}"
+    end
+  # This 'end' is deliberately omitted, to induce a syntax error.
+  # Please don't fix that, as it is used for testing. --daniel 2011-05-02
+end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 4358ef4b6..4ad8787c5 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -22,7 +22,7 @@ describe Puppet::Interface::FaceCollection do
 
   after :each do
     subject.instance_variable_set(:@faces, @original_faces)
-    $".clear ; @original_required.each do |item| $" << item end
+    @original_required.each {|f| $".push f unless $".include? f }
   end
 
   describe "::prefix_match?" do
@@ -160,4 +160,21 @@ describe Puppet::Interface::FaceCollection do
       end
     end
   end
+
+  context "faulty faces" do
+    before :each do
+      $:.unshift "#{PuppetSpec::FIXTURE_DIR}/faulty_face"
+    end
+
+    after :each do
+      $:.delete_if {|x| x == "#{PuppetSpec::FIXTURE_DIR}/faulty_face"}
+    end
+
+    it "should not die if a face has a syntax error" do
+      subject.faces.should be_include :help
+      subject.faces.should_not be_include :syntax
+      @logs.should_not be_empty
+      @logs.first.message.should =~ /syntax error/
+    end
+  end
 end
-- 
cgit