Merge branch 'bug/2.7.x/7314-backtrace-when-face-has-syntax-error' into 2.7.x

18 files changed, 72 insertions, 52 deletions

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/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/face/indirector.rb b/lib/puppet/indirector/face.rb
index 16ffcd311..0fd44dfea 100644
--- a/lib/puppet/face/indirector.rb
+++ b/lib/puppet/indirector/face.rb
@@ -1,7 +1,6 @@
-require 'puppet'
 require 'puppet/face'
 
-class Puppet::Face::Indirector < 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
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/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/indirector/face_spec.rb
index e7dd44f3d..1530f7270 100755
--- a/spec/unit/face/indirector_spec.rb
+++ b/spec/unit/indirector/face_spec.rb
@@ -1,10 +1,10 @@
 #!/usr/bin/env rspec
 require 'spec_helper'
-require 'puppet/face/indirector'
+require 'puppet/indirector/face'
 
-describe Puppet::Face::Indirector do
+describe Puppet::Indirector::Face do
   subject do
-    instance = Puppet::Face::Indirector.new(:test, '0.0.1')
+    instance = Puppet::Indirector::Face.new(:test, '0.0.1')
     indirection = stub('indirection',
                        :name => :stub_indirection,
                        :reset_terminus_class => nil)
@@ -13,24 +13,24 @@ describe Puppet::Face::Indirector do
   end
 
   it "should be able to return a list of indirections" do
-    Puppet::Face::Indirector.indirections.should be_include("catalog")
+    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::Face::Indirector.terminus_classes(:catalog).should be_include("compiler")
+    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::Face::Indirector.new(:catalog, '0.0.1').indirection.should equal(Puppet::Resource::Catalog.indirection)
+      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::Face::Indirector.should be_action(method)
+      Puppet::Indirector::Face.should be_action(method)
     end
 
     it "should call the indirection method with options when the '#{method}' action is invoked" do
@@ -54,6 +54,6 @@ describe Puppet::Face::Indirector do
   end
 
   it "should define a class-level 'info' action" do
-    Puppet::Face::Indirector.should be_action(:info)
+    Puppet::Indirector::Face.should be_action(:info)
   end
 end
diff --git a/spec/unit/interface/face_collection_spec.rb b/spec/unit/interface/face_collection_spec.rb
index 890e06a9e..4ad8787c5 100755
--- a/spec/unit/interface/face_collection_spec.rb
+++ b/spec/unit/interface/face_collection_spec.rb
@@ -16,12 +16,13 @@ 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)
-    $".clear ; @original_required.each do |item| $" << item end
+    subject.instance_variable_set(:@faces, @original_faces)
+    @original_required.each {|f| $".push f unless $".include? f }
   end
 
   describe "::prefix_match?" do
@@ -159,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