From 0bcbca53f3248137de517a4942e4db70ab06e296 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Thu, 26 May 2011 14:36:19 -0700 Subject: maint: Dedup the loadpath so we don't have to walk it multiple times If the user's path has duplicate entries, we end up looking at them multiple times. This has bitten people in the past in that if you get a lot of duplication it can make autloading a lot slower. Really the user shouldn't be duplicating their path, but since we can't control that, this is a safe fix to prevent them from having autoload slowness. Paired-with: Max Martin --- lib/puppet/util/autoload.rb | 2 +- spec/unit/util/autoload_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb index f0dd0a5c5..0e7025aef 100644 --- a/lib/puppet/util/autoload.rb +++ b/lib/puppet/util/autoload.rb @@ -124,7 +124,7 @@ class Puppet::Util::Autoload # The list of directories to search through for loadable plugins. def searchpath(env=nil) - search_directories(env).collect { |d| File.join(d, @path) }.find_all { |d| FileTest.directory?(d) } + search_directories(env).uniq.collect { |d| File.join(d, @path) }.find_all { |d| FileTest.directory?(d) } end def module_directories(env=nil) diff --git a/spec/unit/util/autoload_spec.rb b/spec/unit/util/autoload_spec.rb index 512f06c75..d61b7689e 100755 --- a/spec/unit/util/autoload_spec.rb +++ b/spec/unit/util/autoload_spec.rb @@ -51,9 +51,9 @@ describe Puppet::Util::Autoload do @autoload.search_directories.should == %w{/one /two /libdir1 /lib/dir/two /third/lib/dir} + $LOAD_PATH end - it "should include in its search path all of the search directories that have a subdirectory matching the autoload path" do + it "should include in its search path all of the unique search directories that have a subdirectory matching the autoload path" do @autoload = Puppet::Util::Autoload.new("foo", "loaddir") - @autoload.expects(:search_directories).returns %w{/one /two /three} + @autoload.expects(:search_directories).returns %w{/one /two /three /three} FileTest.expects(:directory?).with("/one/loaddir").returns true FileTest.expects(:directory?).with("/two/loaddir").returns false FileTest.expects(:directory?).with("/three/loaddir").returns true -- cgit From 654783564afd082c2e3e436cdf1bc3dc60308497 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Thu, 26 May 2011 14:54:11 -0700 Subject: (#7690) Don't blow up when listing terminuses available for faces Previously, in order to list the available terminuses for an indirected face we loaded all the the terminuses in order to list them. This meant that if a terminus like active_record didn't have the dependencies it needed, the documentation would raise errors and not list terminuses. Now we just list the terminuses available in the load path without trying to load them. The terminus will still raise an error if you try to use it without its dependencies being met. Paired-with: Max Martin --- lib/puppet/application/faces.rb | 6 +----- lib/puppet/indirector/terminus.rb | 9 +++------ lib/puppet/util/autoload.rb | 28 +++++++++++++++------------- spec/unit/indirector/terminus_spec.rb | 6 ++++++ 4 files changed, 25 insertions(+), 24 deletions(-) diff --git a/lib/puppet/application/faces.rb b/lib/puppet/application/faces.rb index e7fce66b1..3145da821 100644 --- a/lib/puppet/application/faces.rb +++ b/lib/puppet/application/faces.rb @@ -66,7 +66,7 @@ Copyright (c) 2011 Puppet Labs, LLC Licensed under the Apache 2.0 License str = "#{name}:\n" if arguments.include?("terminuses") begin - terms = terminus_classes(name.to_sym) + terms = Puppet::Indirector::Face.terminus_classes(name.to_sym) str << "\tTerminuses: #{terms.join(", ")}\n" rescue => detail puts detail.backtrace if Puppet[:trace] @@ -107,10 +107,6 @@ Copyright (c) 2011 Puppet Labs, LLC Licensed under the Apache 2.0 License Puppet::Face.faces end - def terminus_classes(indirection) - Puppet::Indirector::Terminus.terminus_classes(indirection).collect { |t| t.to_s }.sort - end - def actions(indirection) return [] unless face = Puppet::Face[indirection, '0.0.1'] face.load_actions diff --git a/lib/puppet/indirector/terminus.rb b/lib/puppet/indirector/terminus.rb index 4ebd0d004..d488869d1 100644 --- a/lib/puppet/indirector/terminus.rb +++ b/lib/puppet/indirector/terminus.rb @@ -111,12 +111,9 @@ class Puppet::Indirector::Terminus # Return all terminus classes for a given indirection. def terminus_classes(indirection_name) setup_instance_loading indirection_name - - # Load them all. - instance_loader(indirection_name).loadall - - # And return the list of names. - loaded_instances(indirection_name) + instance_loader(indirection_name).files_to_load.map do |file| + File.basename(file).chomp(".rb").intern + end end private diff --git a/lib/puppet/util/autoload.rb b/lib/puppet/util/autoload.rb index 0e7025aef..6537a4a4e 100644 --- a/lib/puppet/util/autoload.rb +++ b/lib/puppet/util/autoload.rb @@ -105,23 +105,25 @@ class Puppet::Util::Autoload # so that already-loaded files don't get reloaded unnecessarily. def loadall # Load every instance of everything we can find. - searchpath.each do |dir| - Dir.glob("#{dir}/*.rb").each do |file| - name = File.basename(file).sub(".rb", '').intern - next if loaded?(name) - begin - Kernel.require file - loaded(name, file) - rescue SystemExit,NoMemoryError - raise - rescue Exception => detail - puts detail.backtrace if Puppet[:trace] - raise Puppet::Error, "Could not autoload #{file}: #{detail}" - end + files_to_load.each do |file| + name = File.basename(file).chomp(".rb").intern + next if loaded?(name) + begin + Kernel.require file + loaded(name, file) + rescue SystemExit,NoMemoryError + raise + rescue Exception => detail + puts detail.backtrace if Puppet[:trace] + raise Puppet::Error, "Could not autoload #{file}: #{detail}" end end end + def files_to_load + searchpath.map { |dir| Dir.glob("#{dir}/*.rb") }.flatten + end + # The list of directories to search through for loadable plugins. def searchpath(env=nil) search_directories(env).uniq.collect { |d| File.join(d, @path) }.find_all { |d| FileTest.directory?(d) } diff --git a/spec/unit/indirector/terminus_spec.rb b/spec/unit/indirector/terminus_spec.rb index 33932cfca..2f37c1ff5 100755 --- a/spec/unit/indirector/terminus_spec.rb +++ b/spec/unit/indirector/terminus_spec.rb @@ -242,3 +242,9 @@ describe Puppet::Indirector::Terminus, " when creating terminus class types", :' end end +describe Puppet::Indirector::Terminus, " when listing terminus classes" do + it "should list the terminus files available to load" do + Puppet::Util::Autoload.any_instance.stubs(:files_to_load).returns ["/foo/bar/baz", "/max/runs/marathon"] + Puppet::Indirector::Terminus.terminus_classes('my_stuff').should == [:baz, :marathon] + end +end -- cgit From bc71266f4f76439bc7fc5ba5b78895e801cf8736 Mon Sep 17 00:00:00 2001 From: Matt Robinson Date: Thu, 26 May 2011 15:44:43 -0700 Subject: maint: Fix order dependent spec failure for face indirection An indirected face is being created in spec/unit/application/indirection_base_spec.rb that uses a stubbed out indirection. This stub didn't have a name method defined, which caused the documentation_spec.rb, that does checks against every available face, to blow up with: expected no Exception, got # in lib/puppet/face/indirector/face.rb when listing the indirections for the help text. I toyed with creating a real indirection for the test, but that was harder than expected. Paired-with: Max Martin --- spec/unit/application/indirection_base_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/unit/application/indirection_base_spec.rb b/spec/unit/application/indirection_base_spec.rb index d72def6cf..8a5eee2c6 100755 --- a/spec/unit/application/indirection_base_spec.rb +++ b/spec/unit/application/indirection_base_spec.rb @@ -19,7 +19,6 @@ face.instance_variable_set('@version', :current) Puppet::Face.register(face) ######################################################################## - describe Puppet::Application::IndirectionBase do subject { Puppet::Application::TestIndirection.new } @@ -27,6 +26,8 @@ describe Puppet::Application::IndirectionBase do # It would be nice not to have to stub this, but whatever... writing an # entire indirection stack would cause us more grief. --daniel 2011-03-31 terminus = stub_everything("test indirection terminus") + terminus.stubs(:name).returns(:testindirection) + Puppet::Indirector::Indirection.expects(:instance). with(:testindirection).returns(terminus) -- cgit