(6911) Refactor to localize eval_generate dependency assumptions

To implement #6911 we will need to be able to make incremental descisions about
order of application based only on the contents of the resource graph and local
"working data."  This commit begins to pull the needed structure into a method
(visit_resources) while, for the moment, maintaining the original semantic.

Paired-with: Jesse Wolfe

4 files changed, 76 insertions, 89 deletions

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index a8668d844..fed0f46da 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -61,36 +61,32 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph
     [$1, $2]
   end
 
-  # Add one or more resources to our graph and to our resource table.
+  # Add a resource to our graph and to our resource table.
   # This is actually a relatively complicated method, because it handles multiple
   # aspects of Catalog behaviour:
   # * Add the resource to the resource table
   # * Add the resource to the resource graph
   # * Add the resource to the relationship graph
   # * Add any aliases that make sense for the resource (e.g., name != title)
-  def add_resource(*resources)
-    resources.each do |resource|
-      raise ArgumentError, "Can only add objects that respond to :ref, not instances of #{resource.class}" unless resource.respond_to?(:ref)
-    end.each { |resource| fail_on_duplicate_type_and_title(resource) }.each do |resource|
-      title_key = title_key_for_ref(resource.ref)
-
-      @transient_resources << resource if applying?
-      @resource_table[title_key] = resource
-
-      # If the name and title differ, set up an alias
-
-      if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.respond_to?(:isomorphic?) and resource.name != resource.title
-        self.alias(resource, resource.uniqueness_key) if resource.isomorphic?
-      end
-
-      resource.catalog = self if resource.respond_to?(:catalog=)
-
-      add_vertex(resource)
-
-      @relationship_graph.add_vertex(resource) if @relationship_graph
-
-      yield(resource) if block_given?
+  def add_resource(*resource)
+    add_resource(*resource[0..-2]) if resource.length > 1
+    resource = resource.pop
+    raise ArgumentError, "Can only add objects that respond to :ref, not instances of #{resource.class}" unless resource.respond_to?(:ref)
+    fail_on_duplicate_type_and_title(resource)
+    title_key = title_key_for_ref(resource.ref)
+ 
+    @transient_resources << resource if applying?
+    @resource_table[title_key] = resource
+
+    # If the name and title differ, set up an alias
+
+    if resource.respond_to?(:name) and resource.respond_to?(:title) and resource.respond_to?(:isomorphic?) and resource.name != resource.title
+      self.alias(resource, resource.uniqueness_key) if resource.isomorphic?
     end
+
+    resource.catalog = self if resource.respond_to?(:catalog=)
+    add_vertex(resource)
+    @relationship_graph.add_vertex(resource) if @relationship_graph
   end
 
   # Create an alias for a resource.
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index eba601cfe..aa0eec3ee 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -12,7 +12,7 @@ class Puppet::Transaction
   require 'puppet/resource/status'
 
   attr_accessor :component, :catalog, :ignoreschedules
-  attr_accessor :sorted_resources, :configurator
+  attr_accessor :configurator
 
   # The report, once generated.
   attr_accessor :report
@@ -57,32 +57,23 @@ class Puppet::Transaction
     report.resource_statuses.values.find_all { |status| status.changed }.collect { |status| catalog.resource(status.resource) }
   end
 
+  # Find all of the applied resources (including failed attempts).
+  def applied_resources
+    report.resource_statuses.values.collect { |status| catalog.resource(status.resource) }
+  end
+
   # Copy an important relationships from the parent to the newly-generated
   # child resource.
-  def make_parent_child_relationship(resource, children)
-    depthfirst = resource.depthfirst?
-
-    children.each do |gen_child|
-      if depthfirst
-        edge = [gen_child, resource]
-      else
-        edge = [resource, gen_child]
-      end
-      relationship_graph.add_vertex(gen_child)
-
-      unless relationship_graph.edge?(edge[1], edge[0])
-        relationship_graph.add_edge(*edge)
-      else
-        resource.debug "Skipping automatic relationship to #{gen_child}"
-      end
+  def make_parent_child_relationship(parent, child)
+    relationship_graph.add_vertex(child)
+    edge = parent.depthfirst? ? [child, parent] : [parent, child]
+    if relationship_graph.edge?(*edge.reverse)
+      parent.debug "Skipping automatic relationship to #{child}"
+    else
+      relationship_graph.add_edge(*edge)
     end
   end
 
-  # See if the resource generates new resources at evaluation time.
-  def eval_generate(resource)
-    generate_additional_resources(resource, :eval_generate)
-  end
-
   # Evaluate a single resource.
   def eval_resource(resource, ancestor = nil)
     if skip?(resource)
@@ -97,26 +88,7 @@ class Puppet::Transaction
 
   def eval_children_and_apply_resource(resource, ancestor = nil)
     resource_status(resource).scheduled = true
-
-    # We need to generate first regardless, because the recursive
-    # actions sometimes change how the top resource is applied.
-    children = eval_generate(resource)
-
-    if ! children.empty? and resource.depthfirst?
-      children.each do |child|
-        # The child will never be skipped when the parent isn't
-        eval_resource(child, ancestor || resource)
-      end
-    end
-
-    # Perform the actual changes
     apply(resource, ancestor)
-
-    if ! children.empty? and ! resource.depthfirst?
-      children.each do |child|
-        eval_resource(child, ancestor || resource)
-      end
-    end
   end
 
   # This method does all the actual work of running a transaction.  It
@@ -131,7 +103,7 @@ class Puppet::Transaction
     Puppet.info "Applying configuration version '#{catalog.version}'" if catalog.version
 
     begin
-      @sorted_resources.each do |resource|
+      visit_resources do |resource|
         next if stop_processing?
         if resource.is_a?(Puppet::Type::Component)
           Puppet.warning "Somehow left a component in the relationship graph"
@@ -177,9 +149,31 @@ class Puppet::Transaction
     found_failed
   end
 
+  def eval_generate(resource)
+    return [] unless resource.respond_to?(:eval_generate)
+    begin
+      made = resource.eval_generate
+    rescue => detail
+      puts detail.backtrace if Puppet[:trace]
+      resource.err "Failed to generate additional resources using 'eval_generate: #{detail}"
+    end
+    parents = [resource]
+    [made].flatten.compact.uniq.each do |res|
+      begin
+        res.tag(*resource.tags)
+        @catalog.add_resource(res)
+        res.finish
+        make_parent_child_relationship(parents.reverse.find { |r| r.name == res.name[0,r.name.length]}, res)
+        parents << res
+      rescue Puppet::Resource::Catalog::DuplicateResourceError
+        res.info "Duplicate generated resource; skipping"
+      end
+    end
+  end
+
   # A general method for recursively generating new resources from a
   # resource.
-  def generate_additional_resources(resource, method)
+  def generate_additional_resources(resource, method, presume_prefix_dependencies=false)
     return [] unless resource.respond_to?(method)
     begin
       made = resource.send(method)
@@ -192,13 +186,10 @@ class Puppet::Transaction
     made.uniq.find_all do |res|
       begin
         res.tag(*resource.tags)
-        @catalog.add_resource(res) do |r|
-          r.finish
-          make_parent_child_relationship(resource, [r])
-
-          # Call 'generate' recursively
-          generate_additional_resources(r, method)
-        end
+        @catalog.add_resource(res)
+        res.finish
+        make_parent_child_relationship(resource, res)
+        generate_additional_resources(res, method, presume_prefix_dependencies)
         true
       rescue Puppet::Resource::Catalog::DuplicateResourceError
         res.info "Duplicate generated resource; skipping"
@@ -269,9 +260,21 @@ class Puppet::Transaction
     # Then prefetch.  It's important that we generate and then prefetch,
     # so that any generated resources also get prefetched.
     prefetch
+  end
 
-    # This will throw an error if there are cycles in the graph.
-    @sorted_resources = relationship_graph.topsort
+  def visit_resources(&block)
+    eval_generated = {}
+    while r = relationship_graph.topsort.find { |r| !applied_resources.include?(r) }
+      #p relationship_graph.topsort.collect { |r0| r0.title[/(-0.*)/,1] }
+      if !eval_generated[r]
+        #p [:generate,r.title]
+        eval_generate(r)
+        eval_generated[r] = true
+      else
+        #p [:apply,r.title]
+        yield r
+      end
+    end
   end
 
   def relationship_graph
diff --git a/lib/puppet/type/file.rb b/lib/puppet/type/file.rb
index 2c5a95bbc..cbc1f77da 100644
--- a/lib/puppet/type/file.rb
+++ b/lib/puppet/type/file.rb
@@ -462,8 +462,7 @@ Puppet::Type.newtype(:file) do
   # be used to copy remote files, manage local files, and/or make links
   # to map to another directory.
   def recurse
-    children = {}
-    children = recurse_local if self[:recurse] != :remote
+    children = (self[:recurse] == :remote) ? {} : recurse_local
 
     if self[:target]
       recurse_link(children)
@@ -500,11 +499,7 @@ Puppet::Type.newtype(:file) do
 
   # A simple method for determining whether we should be recursing.
   def recurse?
-    return false unless @parameters.include?(:recurse)
-
-    val = @parameters[:recurse].value
-
-    !!(val and (val == true or val == :remote))
+    self[:recurse] == true or self[:recurse] == :remote
   end
 
   # Recurse the target of the link.
@@ -576,13 +571,10 @@ Puppet::Type.newtype(:file) do
   end
 
   def perform_recursion(path)
-
     Puppet::FileServing::Metadata.indirection.search(
-
       path,
       :links => self[:links],
       :recurse => (self[:recurse] == :remote ? true : self[:recurse]),
-
       :recurselimit => self[:recurselimit],
       :ignore => self[:ignore],
       :checksum_type => (self[:source] || self[:content]) ? self[:checksum] : :none
diff --git a/lib/puppet/type/tidy.rb b/lib/puppet/type/tidy.rb
index d2e9ebe22..1a308e17d 100755
--- a/lib/puppet/type/tidy.rb
+++ b/lib/puppet/type/tidy.rb
@@ -240,10 +240,6 @@ Puppet::Type.newtype(:tidy) do
     []
   end
 
-  def eval_generate
-    []
-  end
-
   def generate
     return [] unless stat(self[:path])