[CHEF-3376] Load all filetypes in run_list order

* Extract file loading for non-recipe types to implementation class.
* Load libraries, LWRPs, Resource Definitions in approximate run_list
  order
* Should not be a breaking change on chef-client, since the order is
  essentially random right now. On chef-solo, files from cookbooks that
  don't appear in the expanded run_list plus dependency graph will not
  be loaded, which is a change from the previous behavior.

2 files changed, 343 insertions, 157 deletions

diff --git a/lib/chef/run_context.rb b/lib/chef/run_context.rb
index 8358f1402b..6d5f001aac 100644
--- a/lib/chef/run_context.rb
+++ b/lib/chef/run_context.rb
@@ -28,6 +28,230 @@ class Chef
   # Value object that loads and tracks the context of a Chef run
   class RunContext
 
+    # Implements the compile phase of the chef run by loading/eval-ing files
+    # from cookbooks in the correct order and in the correct context.
+    class CookbookCompiler
+      attr_reader :node
+      attr_reader :events
+      attr_reader :run_list_expansion
+      attr_reader :cookbook_collection
+
+      # Resource Definitions from the compiled cookbooks. This is populated by
+      # calling #compile_resource_definitions (which is called by #compile)
+      attr_reader :definitions
+
+      def initialize(node, cookbook_collection, run_list_expansion, events)
+        @node = node
+        @events = events
+        @run_list_expansion = run_list_expansion
+        @cookbook_collection = cookbook_collection
+
+        # @resource_collection = Chef::ResourceCollection.new
+        # @immediate_notification_collection = Hash.new {|h,k| h[k] = []}
+        # @delayed_notification_collection = Hash.new {|h,k| h[k] = []}
+        # @loaded_recipes = {}
+        # @loaded_attributes = {}
+        #
+
+        @definitions = Hash.new
+        @cookbook_order = nil
+      end
+
+      # Run the compile phase of the chef run. Loads files in the following order:
+      # * Libraries
+      # * Attributes
+      # * LWRPs
+      # * Resource Definitions
+      # * Recipes
+      #
+      # Recipes are loaded in precisely the order specified by the expanded run_list.
+      #
+      # Other files are loaded in an order derived from the expanded run_list
+      # and the dependencies declared by cookbooks' metadata. See
+      # #cookbook_order for more information.
+      def compile
+        compile_libraries
+        compile_attributes
+        compile_lwrps
+        compile_resource_definitions
+        #compile_recipes
+      end
+
+      # Extracts the cookbook names from the expanded run list, then iterates
+      # over the list, recursing through dependencies to give a run_list
+      # ordered array of cookbook names with no duplicates. Dependencies appear
+      # before the cookbook they depend on.
+      def cookbook_order
+        @cookbook_order ||= begin
+          ordered_cookbooks = []
+          seen_cookbooks = {}
+          run_list_expansion.recipes.each do |recipe|
+            cookbook = Chef::Recipe.parse_recipe_name(recipe).first
+            add_cookbook_with_deps(ordered_cookbooks, seen_cookbooks, cookbook)
+          end
+          ordered_cookbooks
+        end
+      end
+
+      # Loads library files from cookbooks according to #cookbook_order.
+      def compile_libraries
+        @events.library_load_start(count_files_by_segment(:libraries))
+        cookbook_order.each do |cookbook|
+          load_libraries_from_cookbook(cookbook)
+        end
+        @events.library_load_complete
+      end
+
+      # Loads attributes files from cookbooks. Attributes files are loaded
+      # according to #cookbook_order; within a cookbook, +default.rb+ is loaded
+      # first, then the remaining attributes files in lexical sort order.
+      def compile_attributes
+        @events.attribute_load_start(count_files_by_segment(:attributes))
+        cookbook_order.each do |cookbook|
+          load_attributes_from_cookbook(cookbook)
+        end
+        @events.attribute_load_complete
+      end
+
+      # Loads LWRPs according to #cookbook_order. Providers are loaded before
+      # resources on a cookbook-wise basis.
+      def compile_lwrps
+        lwrp_file_count = count_files_by_segment(:providers) + count_files_by_segment(:resources)
+        @events.lwrp_load_start(lwrp_file_count)
+        cookbook_order.each do |cookbook|
+          load_lwrps_from_cookbook(cookbook)
+        end
+        @events.lwrp_load_complete
+      end
+
+      def compile_resource_definitions
+        @events.definition_load_start(count_files_by_segment(:definitions))
+        cookbook_order.each do |cookbook|
+          load_resource_definitions_from_cookbook(cookbook)
+        end
+        @events.definition_load_complete
+      end
+
+
+      private
+
+      def load_attributes_from_cookbook(cookbook_name)
+        list_of_attr_files = files_in_cookbook_by_segment(cookbook_name, :attributes).dup
+        if default_file = list_of_attr_files.find {|path| File.basename(path) == "default.rb" }
+          list_of_attr_files.delete(default_file)
+          load_attribute_file(cookbook_name.to_s, default_file)
+        end
+
+        list_of_attr_files.each do |filename|
+          load_attribute_file(cookbook_name.to_s, filename)
+        end
+      end
+
+      def load_attribute_file(cookbook_name, filename)
+        Chef::Log.debug("Node #{@node.name} loading cookbook #{cookbook_name}'s attribute file #{filename}")
+        attr_file_basename = ::File.basename(filename, ".rb")
+        @node.include_attribute("#{cookbook_name}::#{attr_file_basename}")
+      rescue Exception => e
+        @events.attribute_file_load_failed(filename, e)
+        raise
+      end
+
+      def load_libraries_from_cookbook(cookbook_name)
+        files_in_cookbook_by_segment(cookbook_name, :libraries).each do |filename|
+          begin
+            Chef::Log.debug("Loading cookbook #{cookbook_name}'s library file: #{filename}")
+            Kernel.load(filename)
+            @events.library_file_loaded(filename)
+          rescue Exception => e
+            @events.library_file_load_failed(filename, e)
+            raise
+          end
+        end
+      end
+
+      def load_lwrps_from_cookbook(cookbook_name)
+        files_in_cookbook_by_segment(cookbook_name, :providers).each do |filename|
+          load_lwrp_provider(cookbook_name, filename)
+        end
+        files_in_cookbook_by_segment(cookbook_name, :resources).each do |filename|
+          load_lwrp_resource(cookbook_name, filename)
+        end
+      end
+
+      def load_lwrp_provider(cookbook_name, filename)
+        Chef::Log.debug("Loading cookbook #{cookbook_name}'s providers from #{filename}")
+        Chef::Provider.build_from_file(cookbook_name, filename, self)
+        @events.lwrp_file_loaded(filename)
+      rescue Exception => e
+        @events.lwrp_file_load_failed(filename, e)
+        raise
+      end
+
+      def load_lwrp_resource(cookbook_name, filename)
+        Chef::Log.debug("Loading cookbook #{cookbook_name}'s resources from #{filename}")
+        Chef::Resource.build_from_file(cookbook_name, filename, self)
+        @events.lwrp_file_loaded(filename)
+      rescue Exception => e
+        @events.lwrp_file_load_failed(filename, e)
+        raise
+      end
+
+
+      def load_resource_definitions_from_cookbook(cookbook_name)
+        files_in_cookbook_by_segment(cookbook_name, :definitions).each do |filename|
+          begin
+            Chef::Log.debug("Loading cookbook #{cookbook_name}'s definitions from #{filename}")
+            resourcelist = Chef::ResourceDefinitionList.new
+            resourcelist.from_file(filename)
+            definitions.merge!(resourcelist.defines) do |key, oldval, newval|
+              Chef::Log.info("Overriding duplicate definition #{key}, new definition found in #{filename}")
+              newval
+            end
+            @events.definition_file_loaded(filename)
+          rescue Exception => e
+            @events.definition_file_load_failed(filename, e)
+            raise
+          end
+        end
+      end
+
+      # Builds up the list of +ordered_cookbooks+ by first recursing through the
+      # dependencies of +cookbook+, and then adding +cookbook+ to the list of
+      # +ordered_cookbooks+. A cookbook is skipped if it appears in
+      # +seen_cookbooks+, otherwise it is added to the set of +seen_cookbooks+
+      # before its dependencies are processed.
+      def add_cookbook_with_deps(ordered_cookbooks, seen_cookbooks, cookbook)
+        return false if seen_cookbooks.key?(cookbook)
+
+        seen_cookbooks[cookbook] = true
+        each_cookbook_dep(cookbook) do |dependency|
+          add_cookbook_with_deps(ordered_cookbooks, seen_cookbooks, dependency)
+        end
+        ordered_cookbooks << cookbook
+      end
+
+
+      def count_files_by_segment(segment)
+        cookbook_collection.inject(0) do |count, ( cookbook_name, cookbook )|
+          count + cookbook.segment_filenames(segment).size
+        end
+      end
+
+      # Lists the local paths to files in +cookbook+ of type +segment+
+      # (attribute, recipe, etc.), sorted lexically.
+      def files_in_cookbook_by_segment(cookbook, segment)
+        cookbook_collection[cookbook].segment_filenames(segment).sort
+      end
+
+      # Yields the name of each cookbook depended on by +cookbook_name+ in
+      # lexical sort order.
+      def each_cookbook_dep(cookbook_name, &block)
+        cookbook = cookbook_collection[cookbook_name]
+        cookbook.metadata.dependencies.keys.sort.each(&block)
+      end
+
+    end
+
     attr_reader :node, :cookbook_collection, :definitions
 
     # Needs to be settable so deploy can run a resource_collection independent
@@ -55,21 +279,16 @@ class Chef
       @loaded_attributes = {}
       @events = events
 
-      @loaded_cookbooks_by_segment = {}
-      CookbookVersion::COOKBOOK_SEGMENTS.each do |segment|
-        @loaded_cookbooks_by_segment[segment] = {}
-      end
-
       @node.run_context = self
     end
 
     def load(run_list_expansion)
-      load_libraries
+      load_libraries_in_run_list_order(run_list_expansion)
 
-      load_lwrps
+      load_lwrps_in_run_list_order(run_list_expansion)
       load_attributes_in_run_list_order(run_list_expansion)
 
-      load_resource_definitions
+      load_resource_definitions_in_run_list_order(run_list_expansion)
 
       @events.recipe_load_start(run_list_expansion.recipes.size)
       run_list_expansion.recipes.each do |recipe|
@@ -93,6 +312,8 @@ class Chef
       cookbook.recipe_filenames_by_name[recipe_short_name]
     end
 
+    # Looks up an attribute file given the +cookbook_name+ and
+    # +attr_file_name+. Used by DSL::IncludeAttribute
     def resolve_attribute(cookbook_name, attr_file_name)
       cookbook = cookbook_collection[cookbook_name]
       raise Chef::Exceptions::CookbookNotFound, "could not find cookbook #{cookbook_name} while loading attribute #{name}" unless cookbook
@@ -179,149 +400,32 @@ class Chef
       @loaded_attributes["#{cookbook}::#{attribute_file}"] = true
     end
 
-    def load_attributes_in_run_list_order(run_list_expansion)
-      @events.attribute_load_start(count_files_by_segment(:attributes))
-      each_cookbook_in_run_list_order(run_list_expansion) do |cookbook|
-        load_attributes_from_cookbook(cookbook)
-      end
-      @events.attribute_load_complete
+    def load_libraries_in_run_list_order(run_list_expansion)
+      @compiler = CookbookCompiler.new(node, cookbook_collection, run_list_expansion, events)
+      @compiler.compile_libraries
     end
 
-    def load_attributes_from_cookbook(cookbook_name)
-      # avoid loading a cookbook again if it's been loaded.
-      return false if @loaded_cookbooks_by_segment[:attributes].key?(cookbook_name)
-      @loaded_cookbooks_by_segment[:attributes][cookbook_name] = true
-      each_cookbook_dep(cookbook_name) do |cookbook_dep|
-        load_attributes_from_cookbook(cookbook_dep)
-      end
-      list_of_attr_files = files_in_cookbook_by_segment(cookbook_name, :attributes).dup
-      if default_file = list_of_attr_files.find {|path| File.basename(path) == "default.rb" }
-        list_of_attr_files.delete(default_file)
-        load_attribute_file(cookbook_name.to_s, default_file)
-      end
-
-      list_of_attr_files.sort.each do |filename|
-        load_attribute_file(cookbook_name.to_s, filename)
-      end
+    def load_attributes_in_run_list_order(run_list_expansion)
+      @compiler = CookbookCompiler.new(node, cookbook_collection, run_list_expansion, events)
+      @compiler.compile_attributes
     end
 
-    private
-
-    def each_cookbook_dep(cookbook_name, &block)
-      cookbook = cookbook_collection[cookbook_name]
-      cookbook.metadata.dependencies.keys.sort.each(&block)
+    def load_lwrps_in_run_list_order(run_list_expansion)
+      @compiler = CookbookCompiler.new(node, cookbook_collection, run_list_expansion, events)
+      @compiler.compile_lwrps
     end
 
-    def each_cookbook_in_run_list_order(run_list_expansion, &block)
-      cookbook_order = run_list_expansion.recipes.map do |recipe|
-        Chef::Recipe.parse_recipe_name(recipe).first
-      end
-      cookbook_order.uniq.each(&block)
+    def load_resource_definitions_in_run_list_order(run_list_expansion)
+      @compiler = CookbookCompiler.new(node, cookbook_collection, run_list_expansion, events)
+      @compiler.compile_resource_definitions
+      @definitions = @compiler.definitions
     end
 
+    private
+
     def loaded_recipe(cookbook, recipe)
       @loaded_recipes["#{cookbook}::#{recipe}"] = true
     end
 
-    def load_libraries
-      @events.library_load_start(count_files_by_segment(:libraries))
-
-      foreach_cookbook_load_segment(:libraries) do |cookbook_name, filename|
-        begin
-          Chef::Log.debug("Loading cookbook #{cookbook_name}'s library file: #{filename}")
-          Kernel.load(filename)
-          @events.library_file_loaded(filename)
-        rescue Exception => e
-          # TODO wrap/munge exception to highlight syntax/name/no method errors.
-          @events.library_file_load_failed(filename, e)
-          raise
-        end
-      end
-
-      @events.library_load_complete
-    end
-
-    def load_lwrps
-      lwrp_file_count = count_files_by_segment(:providers) + count_files_by_segment(:resources)
-      @events.lwrp_load_start(lwrp_file_count)
-      load_lwrp_providers
-      load_lwrp_resources
-      @events.lwrp_load_complete
-    end
-
-    def load_lwrp_providers
-      foreach_cookbook_load_segment(:providers) do |cookbook_name, filename|
-        begin
-          Chef::Log.debug("Loading cookbook #{cookbook_name}'s providers from #{filename}")
-          Chef::Provider.build_from_file(cookbook_name, filename, self)
-          @events.lwrp_file_loaded(filename)
-        rescue Exception => e
-          # TODO: wrap exception with helpful info
-          @events.lwrp_file_load_failed(filename, e)
-          raise
-        end
-      end
-    end
-
-    def load_lwrp_resources
-      foreach_cookbook_load_segment(:resources) do |cookbook_name, filename|
-        begin
-          Chef::Log.debug("Loading cookbook #{cookbook_name}'s resources from #{filename}")
-          Chef::Resource.build_from_file(cookbook_name, filename, self)
-          @events.lwrp_file_loaded(filename)
-        rescue Exception => e
-          @events.lwrp_file_load_failed(filename, e)
-          raise
-        end
-      end
-    end
-
-    def load_attribute_file(cookbook_name, filename)
-      Chef::Log.debug("Node #{@node.name} loading cookbook #{cookbook_name}'s attribute file #{filename}")
-      attr_file_basename = ::File.basename(filename, ".rb")
-      @node.include_attribute("#{cookbook_name}::#{attr_file_basename}")
-    rescue Exception => e
-      @events.attribute_file_load_failed(filename, e)
-      raise
-    end
-
-    def load_resource_definitions
-      @events.definition_load_start(count_files_by_segment(:definitions))
-      foreach_cookbook_load_segment(:definitions) do |cookbook_name, filename|
-        begin
-          Chef::Log.debug("Loading cookbook #{cookbook_name}'s definitions from #{filename}")
-          resourcelist = Chef::ResourceDefinitionList.new
-          resourcelist.from_file(filename)
-          definitions.merge!(resourcelist.defines) do |key, oldval, newval|
-            Chef::Log.info("Overriding duplicate definition #{key}, new definition found in #{filename}")
-            newval
-          end
-          @events.definition_file_loaded(filename)
-        rescue Exception => e
-          @events.definition_file_load_failed(filename, e)
-        end
-      end
-      @events.definition_load_complete
-    end
-
-    def count_files_by_segment(segment)
-      cookbook_collection.inject(0) do |count, ( cookbook_name, cookbook )|
-        count + cookbook.segment_filenames(segment).size
-      end
-    end
-
-    def files_in_cookbook_by_segment(cookbook, segment)
-      cookbook_collection[cookbook].segment_filenames(segment)
-    end
-
-    def foreach_cookbook_load_segment(segment, &block)
-      cookbook_collection.each do |cookbook_name, cookbook|
-        segment_filenames = cookbook.segment_filenames(segment)
-        segment_filenames.each do |segment_filename|
-          block.call(cookbook_name, segment_filename)
-        end
-      end
-    end
-
   end
 end
diff --git a/spec/unit/run_context_spec.rb b/spec/unit/run_context_spec.rb
index b742a2f1c7..48f26fbf5c 100644
--- a/spec/unit/run_context_spec.rb
+++ b/spec/unit/run_context_spec.rb
@@ -22,6 +22,23 @@ require 'spec_helper'
 
 Chef::Log.level = :debug
 
+# Keeps track of what file got loaded in what order.
+module LibraryLoadOrder
+  extend self
+
+  def load_order
+    @load_order ||= []
+  end
+
+  def reset!
+    @load_order = nil
+  end
+
+  def record(file)
+    load_order << file
+  end
+end
+
 describe Chef::RunContext do
   before(:each) do
     @chef_repo_path = File.expand_path(File.join(CHEF_SPEC_DATA, "run_context", "cookbooks"))
@@ -48,18 +65,19 @@ describe Chef::RunContext do
 
   # This test relies on fixture data in spec/data/run_context/cookbooks.
   # The behaviors described in these examples are affected by the metadata.rb
-  # files in those cookbooks.
+  # files and attributes files in those cookbooks.
+  #
+  # Attribute files in the fixture data will append their
+  # "cookbook_name::attribute_file_name" to the node's `:attr_load_order`
+  # attribute when loaded.
   describe "loading attribute files" do
 
     it "loads default.rb first, then other files in sort order" do
       @node.run_list("dependency1::default")
       @expansion = (@node.run_list.expand('_default'))
 
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/default.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/aa_first.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/zz_last.rb")).ordered
-
       @run_context.load_attributes_in_run_list_order(@expansion)
+      @node[:attr_load_order].should == ["dependency1::default", "dependency1::aa_first", "dependency1::zz_last"]
     end
 
     it "loads dependencies before loading the depending cookbook's attributes" do
@@ -68,14 +86,14 @@ describe Chef::RunContext do
       @node.run_list("test-with-deps::default", "test-with-deps::server")
       @expansion = (@node.run_list.expand('_default'))
 
-      # dependencies are stored in a hash so therefore unordered, but they should be loaded in sort order
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/default.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/aa_first.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("dependency1", fixture_cb_path("dependency1/attributes/zz_last.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("dependency2", fixture_cb_path("dependency2/attributes/default.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("test-with-deps", fixture_cb_path("test-with-deps/attributes/default.rb")).ordered
-
       @run_context.load_attributes_in_run_list_order(@expansion)
+
+      # dependencies are stored in a hash so therefore unordered, but they should be loaded in sort order
+      @node[:attr_load_order].should == ["dependency1::default",
+                                         "dependency1::aa_first",
+                                         "dependency1::zz_last",
+                                         "dependency2::default",
+                                         "test-with-deps::default"]
     end
 
     it "does not follow infinite dependency loops" do
@@ -83,22 +101,86 @@ describe Chef::RunContext do
       @expansion = (@node.run_list.expand('_default'))
 
       # Circular deps should not cause infinite loops
-      @run_context.should_receive(:load_attribute_file).with("circular-dep2", fixture_cb_path("circular-dep2/attributes/default.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("circular-dep1", fixture_cb_path("circular-dep1/attributes/default.rb")).ordered
-      @run_context.should_receive(:load_attribute_file).with("test-with-circular-deps", fixture_cb_path("test-with-circular-deps/attributes/default.rb")).ordered
-
       @run_context.load_attributes_in_run_list_order(@expansion)
+
+      @node[:attr_load_order].should == ["circular-dep2::default", "circular-dep1::default", "test-with-circular-deps::default"]
     end
 
     it "loads attributes from cookbooks that don't have a default.rb attribute file" do
       @node.run_list("no-default-attr::default.rb")
       @expansion = (@node.run_list.expand('_default'))
 
-      @run_context.should_receive(:load_attribute_file).with("no-default-attr", fixture_cb_path("no-default-attr/attributes/server.rb"))
+      #@run_context.should_receive(:load_attribute_file).with("no-default-attr", fixture_cb_path("no-default-attr/attributes/server.rb"))
       @run_context.load_attributes_in_run_list_order(@expansion)
+
+      @node[:attr_load_order].should == ["no-default-attr::server"]
+    end
+  end
+
+  describe "loading libraries" do
+    before do
+      LibraryLoadOrder.reset!
+    end
+
+    # One big test for everything. Individual behaviors are tested by the attribute code above.
+    it "loads libraries in run list order" do
+      @node.run_list("test-with-deps::default", "test-with-circular-deps::default")
+      @expansion = (@node.run_list.expand('_default'))
+
+      @run_context.load_libraries_in_run_list_order(@expansion)
+      LibraryLoadOrder.load_order.should == ["dependency1", "dependency2", "test-with-deps", "circular-dep2", "circular-dep1", "test-with-circular-deps"]
+    end
+  end
+
+  describe "loading LWRPs" do
+    before do
+      LibraryLoadOrder.reset!
+    end
+
+    # One big test for everything. Individual behaviors are tested by the attribute code above.
+    it "loads LWRPs in run list order" do
+      @node.run_list("test-with-deps::default", "test-with-circular-deps::default")
+      @expansion = (@node.run_list.expand('_default'))
+
+      @run_context.load_lwrps_in_run_list_order(@expansion)
+      LibraryLoadOrder.load_order.should == ["dependency1-provider",
+                                             "dependency1-resource",
+                                             "dependency2-provider",
+                                             "dependency2-resource",
+                                             "test-with-deps-provider",
+                                             "test-with-deps-resource",
+                                             "circular-dep2-provider",
+                                             "circular-dep2-resource",
+                                             "circular-dep1-provider",
+                                             "circular-dep1-resource",
+                                             "test-with-circular-deps-provider",
+                                             "test-with-circular-deps-resource"]
     end
   end
 
+  describe "loading resource definitions" do
+    before do
+      LibraryLoadOrder.reset!
+    end
+
+    # One big test for all load order concerns. Individual behaviors are tested
+    # by the attribute code above.
+    it "loads resource definitions in run list order" do
+      @node.run_list("test-with-deps::default", "test-with-circular-deps::default")
+      @expansion = (@node.run_list.expand('_default'))
+
+      @run_context.load_resource_definitions_in_run_list_order(@expansion)
+      LibraryLoadOrder.load_order.should == ["dependency1-definition",
+                                             "dependency2-definition",
+                                             "test-with-deps-definition",
+                                             "circular-dep2-definition",
+                                             "circular-dep1-definition",
+                                             "test-with-circular-deps-definition"]
+    end
+
+  end
+
+
   describe "after loading the cookbooks" do
     before do
       @run_context.load(@node.run_list.expand('_default'))