Use autoloader in application / command line methods

There were a few places where we were loading things
directly from the LOAD_PATH, rather than going through
the autoloader; now that we are considering having
the modulepath lib dirs eligible for loading from,
it is important that we handle all of this in a single,
central spot in the code.  Thus this commit changes
the command_line/application/face_collection classes
to use the autoloader rather than trying to manage
things themselves.

lib/puppet/application.rb

@@ -219,14 +219,14 @@ def option_parser_commands
       @option_parser_commands
     end
 
+    def autoloader
+      @autoloader ||= Puppet::Util::Autoload.new(:application, "puppet/application")
+    end
+
     def find(file_name)
-      # This should probably be using the autoloader, but due to concerns about the fact that
-      #  the autoloader currently considers the modulepath when looking for things to load,
-      #  we're delaying that for now.
-      begin
-        require ::File.join('puppet', 'application', file_name.to_s.downcase)
-      rescue LoadError => e
-        Puppet.log_and_raise(e, "Unable to find application '#{file_name}'.  #{e}")
+      unless autoloader.load(file_name.to_s.downcase)
+        e = LoadError.new("puppet/application/" + file_name.to_s.downcase)
+        Puppet.log_and_raise(e, "Unable to find application '#{file_name}': #{e}")
       end
 
       class_name = Puppet::Util::ConstantInflector.file2constant(file_name.to_s)

lib/puppet/interface/face_collection.rb

@@ -98,20 +98,23 @@ def self.load_face(name, version)
     return get_face(name, version)
   end
 
+
+  def self.autoloader
+    @autoloader ||= Puppet::Util::Autoload.new(:application, "puppet/face")
+  end
+
+
   def self.safely_require(name, version = nil)
-    path = File.join 'puppet' ,'face', version.to_s, name.to_s
-    require path
-    true
-
-  rescue LoadError => e
-    raise unless e.message =~ %r{-- #{path}$}
-    # ...guess we didn't find the file; return a much better problem.
-    nil
-  rescue SyntaxError => e
-    raise unless e.message =~ %r{#{path}\.rb:\d+: }
-    Puppet.err "Failed to load face #{name}:\n#{e}"
-    # ...but we just carry on after complaining.
-    nil
+    path = version ? File.join(version.to_s, name.to_s) : name.to_s
+    return true if autoloader.loaded?(path)
+    begin
+      return true if autoloader.load(path)
+    rescue Puppet::Error => e
+      raise e unless e.message =~ /Could not autoload/
+      Puppet.err "Failed to load face #{name}:\n"
+      # ...but we just carry on after complaining.
+      nil
+    end
   end
 
   def self.register(face)

lib/puppet/util/command_line.rb

@@ -26,10 +26,7 @@ def appdir
 
 
       def self.available_subcommands
-        # Eventually we probably want to replace this with a call to the autoloader.  however, at the moment
-        #  the autoloader considers the module path when loading, and we don't want to allow apps / faces to load
-        #  from there.  Once that is resolved, this should be replaced.  --cprice 2012-03-06
-        absolute_appdirs = $LOAD_PATH.collect do |x|
+        absolute_appdirs = Puppet::Util::Autoload.search_directories(Puppet[:environment]).collect do |x|
           File.join(x,'puppet','application')
         end.select{ |x| File.directory?(x) }
         absolute_appdirs.inject([]) do |commands, dir|
@@ -44,8 +41,13 @@ def available_subcommands
         self.class.available_subcommands
       end
 
+
+      def autoloader
+        @autoloader ||= Puppet::Util::Autoload.new(:application, "puppet/application")
+      end
+
       def require_application(application)
-        require File.join(appdir, application)
+        autoloader.load(application) unless autoloader.loaded?(application)
       end
 
       # This is the main entry point for all puppet applications / faces; it

spec/unit/application_spec.rb

@@ -42,10 +42,9 @@
     end
 
     it "should error if it can't find a class" do
-      Puppet.expects(:err).with do |value|
+      Puppet.expects(:err) .with do |value|
         value =~ /Unable to find application 'ThisShallNeverEverEverExist'/ and
-          value =~ /puppet\/application\/thisshallneverevereverexist/ and
-          value =~ /no such file to load|cannot load such file/
+        value =~ /puppet\/application\/thisshallneverevereverexist/
       end
 
       expect { @klass.find("ThisShallNeverEverEverExist") }.to raise_error(LoadError)

spec/unit/interface/face_collection_spec.rb

@@ -42,49 +42,56 @@
       subject["foo", '0.0.1'].should == 10
     end
 
-    it "should attempt to load the face if it isn't found" do
-      subject.expects(:require).once.with('puppet/face/bar')
-      subject.expects(:require).once.with('puppet/face/0.0.1/bar')
-      subject["bar", '0.0.1']
-    end
+    context "without something on disk" do
+      before :each do
+        @autoloader = mock 'autoloader'
+        @autoloader.stubs(:loaded?).returns(false)
+        subject.stubs(:autoloader).returns(@autoloader)
+      end
 
-    it "should attempt to load the default face for the specified version :current" do
-      subject.expects(:require).with('puppet/face/fozzie')
-      subject['fozzie', :current]
-    end
+      it "should attempt to load the face if it isn't found" do
+        @autoloader.expects(:load).once.with('bar')
+        @autoloader.expects(:load).once.with('0.0.1/bar')
+        subject["bar", '0.0.1']
+      end
 
-    it "should return true if the face specified is registered" do
-      subject.instance_variable_get("@faces")[:foo][SemVer.new('0.0.1')] = 10
-      subject["foo", '0.0.1'].should == 10
-    end
+      it "should attempt to load the default face for the specified version :current" do
+        @autoloader.expects(:load).with('fozzie')
+        subject['fozzie', :current]
+      end
 
-    it "should attempt to require the face if it is not registered" do
-      subject.expects(:require).with do |file|
-        subject.instance_variable_get("@faces")[:bar][SemVer.new('0.0.1')] = true
-        file == 'puppet/face/bar'
+      it "should return true if the face specified is registered" do
+        subject.instance_variable_get("@faces")[:foo][SemVer.new('0.0.1')] = 10
+        subject["foo", '0.0.1'].should == 10
       end
-      subject["bar", '0.0.1'].should be_true
-    end
 
-    it "should return false if the face is not registered" do
-      subject.stubs(:require).returns(true)
-      subject["bar", '0.0.1'].should be_false
-    end
+      it "should attempt to require the face if it is not registered" do
+        @autoloader.expects(:load).with do |file|
+          subject.instance_variable_get("@faces")[:bar][SemVer.new('0.0.1')] = true
+          file == 'bar'
+        end
+        subject["bar", '0.0.1'].should be_true
+      end
 
-    it "should return false if the face file itself is missing" do
-      subject.stubs(:require).
-        raises(LoadError, 'no such file to load -- puppet/face/bar').then.
-        raises(LoadError, 'no such file to load -- puppet/face/0.0.1/bar')
-      subject["bar", '0.0.1'].should be_false
-    end
+      it "should return false if the face is not registered" do
+        @autoloader.stubs(:load).returns(true)
+        subject["bar", '0.0.1'].should be_false
+      end
+
+      it "should return false if the face file itself is missing" do
+        @autoloader.stubs(:load).returns(false)
+        subject["bar", '0.0.1'].should be_false
+      end
 
-    it "should register the version loaded by `:current` as `:current`" do
-      subject.expects(:require).with do |file|
-        subject.instance_variable_get("@faces")[:huzzah]['2.0.1'] = :huzzah_face
-        file == 'puppet/face/huzzah'
+      it "should register the version loaded by `:current` as `:current`" do
+        @autoloader.expects(:load).with do |file|
+          subject.instance_variable_get("@faces")[:huzzah]['2.0.1'] = :huzzah_face
+          file == 'huzzah'
+        end.returns(true)
+        subject["huzzah", :current]
+        subject.instance_variable_get("@faces")[:huzzah][:current].should == :huzzah_face
       end
-      subject["huzzah", :current]
-      subject.instance_variable_get("@faces")[:huzzah][:current].should == :huzzah_face
+
     end
 
     context "with something on disk" do
@@ -96,7 +103,6 @@
 
       it "should index :current when the code was pre-required" do
         subject.instance_variable_get("@faces")[:huzzah].should_not be_key :current
-        require 'puppet/face/huzzah'
         subject[:huzzah, :current].should be_true
       end
     end

spec/unit/interface_spec.rb

@@ -15,6 +15,11 @@
 
   after :each do
     Puppet::Interface::FaceCollection.instance_variable_set("@faces", @faces)
+    # This is a bit scary, but unless we clear out the 'loaded' stuff
+    #  from the autoloader, these tests will fail due to the
+    #  autoloader being out of sync with the FaceCollection and
+    #  $" magic hoo-ha that these tests are doing.
+    Puppet::Util::Autoload.instance_variable_set("@loaded", {})
     $".clear ; @dq.each do |item| $" << item end
   end