This refactor fixes about a quarter of the test failures on master
and (I
hope) will simplify some of the integration issues on the testing
branch.
It is my best guess at The Right Thing To Do (or at least a step in
that
direction) but I could be persuaded otherwise.
The basic idea is to take responsibility for maintaining scope
hierarchy and
class_name -> class_scope mapping out of the compiler class and put
it in the
scope class where it arguably belongs. To maintain the semantics,
class
scopes are all tracked by the "top level" scope, though this could
be relaxed
if the nesting semantics were ever needed.
If this winds up being the right thing to do, related routines (e.g.
newscope)
should be sorted out as well.
Signed-off-by: Markus Roberts <[email protected]>
---
lib/puppet/parser/compiler.rb | 52 ++
+--------------------------
lib/puppet/parser/resource_type.rb | 1 +
lib/puppet/parser/scope.rb | 41 +++++++++++++++++------
spec/unit/parser/compiler.rb | 62 ++++++++++
+-------------------------
spec/unit/parser/resource_type.rb | 5 +++
spec/unit/parser/scope.rb | 37 +++++++++++++++++++++-
6 files changed, 97 insertions(+), 101 deletions(-)
diff --git a/lib/puppet/parser/compiler.rb b/lib/puppet/parser/
compiler.rb
index 6b6cd68..51df86e 100644
--- a/lib/puppet/parser/compiler.rb
+++ b/lib/puppet/parser/compiler.rb
@@ -51,32 +51,11 @@ class Puppet::Parser::Compiler
parser.nodes?
end
- # Store the fact that we've evaluated a class, and store a
reference to
- # the scope in which it was evaluated, so that we can look it
up later.
- def class_set(name, scope)
- if existing = @class_scopes[name]
- if existing.nodescope? != scope.nodescope?
- raise Puppet::ParseError, "Cannot have classes,
nodes, or definitions with the same name"
- else
- raise Puppet::DevError, "Somehow evaluated %s %s
twice" % [ existing.nodescope? ? "node" : "class", name]
- end
- end
- @class_scopes[name] = scope
+ # Store the fact that we've evaluated a class
+ def add_class(name)
@catalog.add_class(name) unless name == ""
end
- # Return the scope associated with a class. This is just here so
- # that subclasses can set their parent scopes to be the scope of
- # their parent class, and it's also used when looking up
qualified
- # variables.
- def class_scope(klass)
- # They might pass in either the class or class name
- if klass.respond_to?(:name)
- @class_scopes[klass.name]
- else
- @class_scopes[klass]
- end
- end
# Return a list of all of the defined classes.
def classlist
@@ -138,7 +117,7 @@ class Puppet::Parser::Compiler
classes.each do |name|
# If we can find the class, then make a resource that
will evaluate it.
if klass = scope.find_hostclass(name)
- found << name and next if class_scope(klass)
+ found << name and next if scope.class_scope(klass)
resource = klass.mk_plain_resource(scope)
@@ -180,27 +159,16 @@ class Puppet::Parser::Compiler
end
# Create a new scope, with either a specified parent scope or
- # using the top scope. Adds an edge between the scope and
- # its parent to the graph.
+ # using the top scope.
def newscope(parent, options = {})
parent ||= topscope
options[:compiler] = self
options[:parser] ||= self.parser
scope = Puppet::Parser::Scope.new(options)
- @scope_graph.add_edge(parent, scope)
+ scope.parent = parent
scope
end
- # Find the parent of a given scope. Assumes scopes only ever
have
- # one in edge, which will always be true.
- def parent(scope)
- if ary = @scope_graph.adjacent(scope, :direction => :in)
and ary.length > 0
- ary[0]
- else
- nil
- end
- end
-
# Return any overrides for the given resource.
def resource_overrides(resource)
@resource_overrides[resource.ref]
@@ -236,7 +204,7 @@ class Puppet::Parser::Compiler
# Now set the node scope appropriately, so that :topscope can
# behave differently.
- @node_scope = class_scope(astnode)
+ @node_scope = topscope.class_scope(astnode)
end
# Evaluate our collections and return true if anything returned
an object.
@@ -384,15 +352,10 @@ class Puppet::Parser::Compiler
def init_main
# Create our initial scope and a resource that will evaluate
main.
@topscope = Puppet::Parser::Scope.new(:compiler =>
self, :parser => self.parser)
- @scope_graph.add_vertex(@topscope)
end
# Set up all of our internal variables.
def initvars
- # The table for storing class singletons. This will only
actually
- # be used by top scopes and node scopes.
- @class_scopes = {}
-
# The list of objects that will available for export.
@exported_resources = {}
@@ -406,9 +369,6 @@ class Puppet::Parser::Compiler
# but they each refer back to the scope that created them.
@collections = []
- # A graph for maintaining scope relationships.
- @scope_graph = Puppet::SimpleGraph.new
-
# For maintaining the relationship between scopes and their
resources.
@catalog = Puppet::Resource::Catalog.new(@node.name)
@catalog.version = @parser.version
diff --git a/lib/puppet/parser/resource_type.rb b/lib/puppet/parser/
resource_type.rb
index a679659..3dbcbcb 100644
--- a/lib/puppet/parser/resource_type.rb
+++ b/lib/puppet/parser/resource_type.rb
@@ -165,6 +165,7 @@ class Puppet::Parser::ResourceType
scope.setvar("title", resource.title) unless
set.include? :title
scope.setvar("name", resource.name) unless set.include? :name
+ scope.class_set(self.name,scope)
end
# Create a new subscope in which to evaluate our code.
diff --git a/lib/puppet/parser/scope.rb b/lib/puppet/parser/scope.rb
index d6d6630..eda53a8 100644
--- a/lib/puppet/parser/scope.rb
+++ b/lib/puppet/parser/scope.rb
@@ -18,7 +18,7 @@ class Puppet::Parser::Scope
attr_accessor :level, :parser, :source, :resource
attr_accessor :base, :keyword, :nodescope
attr_accessor :top, :translated, :compiler
- attr_writer :parent
+ attr_accessor :parent
# A demeterific shortcut to the catalog.
def catalog
@@ -139,6 +139,34 @@ class Puppet::Parser::Scope
@defaults = Hash.new { |dhash,type|
dhash[type] = {}
}
+
+ # The table for storing class singletons. This will only
actually
+ # be used by top scopes and node scopes.
+ @class_scopes = {}
+ end
+
+ # Store the fact that we've evaluated a class, and store a
reference to
+ # the scope in which it was evaluated, so that we can look it
up later.
+ def class_set(name, scope)
+ return parent.class_set(name,scope) if parent
+ if existing = @class_scopes[name]
+ if existing.nodescope? != scope.nodescope?
+ raise Puppet::ParseError, "Cannot have classes,
nodes, or definitions with the same name"
+ else
+ raise Puppet::DevError, "Somehow evaluated %s %s
twice" % [ existing.nodescope? ? "node" : "class", name]
+ end
+ end
+ @class_scopes[name] = scope
+ end
+
+ # Return the scope associated with a class. This is just here so
+ # that subclasses can set their parent scopes to be the scope of
+ # their parent class, and it's also used when looking up
qualified
+ # variables.
+ def class_scope(klass)
+ # They might pass in either the class or class name
+ k = klass.respond_to?(:name) ? klass.name : klass
+ @class_scopes[k] || (parent && parent.class_scope(k))
end
# Collect all of the defaults set at any higher scopes.
@@ -182,7 +210,7 @@ class Puppet::Parser::Scope
warning "Could not look up qualified variable '%s';
class %s could not be found" % [name, klassname]
return usestring ? "" : :undefined
end
- unless kscope = compiler.class_scope(klass)
+ unless kscope = class_scope(klass)
warning "Could not look up qualified variable '%s';
class %s has not been evaluated" % [name, klassname]
return usestring ? "" : :undefined
end
@@ -251,15 +279,6 @@ class Puppet::Parser::Scope
self.nodescope
end
- # We probably shouldn't cache this value... But it's a lot
faster
- # than doing lots of queries.
- def parent
- unless defined?(@parent)
- @parent = compiler.parent(self)
- end
- @parent
- end
-
# Return the list of scopes up to the top scope, ordered with
our own first.
# This is used for looking up variables and defaults.
def scope_path
diff --git a/spec/unit/parser/compiler.rb b/spec/unit/parser/
compiler.rb
index 8a41242..d23d225 100755
--- a/spec/unit/parser/compiler.rb
+++ b/spec/unit/parser/compiler.rb
@@ -40,26 +40,10 @@ describe Puppet::Parser::Compiler do
@compiler = Puppet::Parser::Compiler.new(@node, @parser)
end
- it "should be able to store references to class scopes" do
- lambda { @compiler.class_set "myname",
"myscope" }.should_not raise_error
- end
-
- it "should be able to retrieve class scopes by name" do
- @compiler.class_set "myname", "myscope"
- @compiler.class_scope("myname").should == "myscope"
- end
-
- it "should be able to retrieve class scopes by object" do
- klass = mock 'ast_class'
- klass.expects(:name).returns("myname")
- @compiler.class_set "myname", "myscope"
- @compiler.class_scope(klass).should == "myscope"
- end
-
- it "should be able to return a class list containing all set
classes" do
- @compiler.class_set "", "empty"
- @compiler.class_set "one", "yep"
- @compiler.class_set "two", "nope"
+ it "should be able to return a class list containing all added
classes" do
+ @compiler.add_class ""
+ @compiler.add_class "one"
+ @compiler.add_class "two"
@compiler.classlist.sort.should == %w{one two}.sort
end
@@ -116,7 +100,14 @@ describe Puppet::Parser::Compiler do
scope = mock 'scope'
newscope = @compiler.newscope(scope)
- @compiler.parent(newscope).should equal(scope)
+ newscope.parent.should equal(scope)
+ end
+
+ it "should set the parent scope of the new scope to its
topscope if the parent passed in is nil" do
+ scope = mock 'scope'
+ newscope = @compiler.newscope(nil)
+
+ newscope.parent.should equal(@compiler.topscope)
end
end
@@ -406,6 +397,7 @@ describe Puppet::Parser::Compiler do
@compiler.catalog.stubs(:tag)
@class.expects(:mk_plain_resource).with(@scope)
+ @scope.stubs(:class_scope).with(@class)
@compiler.evaluate_classes(%w{myclass}, @scope)
end
@@ -416,6 +408,7 @@ describe Puppet::Parser::Compiler do
@resource.expects(:evaluate).never
@class.expects(:mk_plain_resource).returns(@resource)
+ @scope.stubs(:class_scope).with(@class)
@compiler.evaluate_classes(%w{myclass}, @scope)
end
@@ -425,6 +418,7 @@ describe Puppet::Parser::Compiler do
@resource.expects(:evaluate)
@class.expects(:mk_plain_resource).returns(@resource)
+ @scope.stubs(:class_scope).with(@class)
@compiler.evaluate_classes(%w{myclass}, @scope, false)
end
@@ -432,7 +426,7 @@ describe Puppet::Parser::Compiler do
it "should skip classes that have already been evaluated" do
@compiler.catalog.stubs(:tag)
-
@compiler.expects(:class_scope).with(@class).returns("something")
+
@scope.stubs(:class_scope).with(@class).returns("something")
@compiler.expects(:add_resource).never
@@ -445,7 +439,7 @@ describe Puppet::Parser::Compiler do
it "should skip classes previously evaluated with different
capitalization" do
@compiler.catalog.stubs(:tag)
@scope.stubs(:find_hostclass).with("MyClass").returns(@class)
-
@compiler.expects(:class_scope).with(@class).returns("something")
+
@scope.stubs(:class_scope).with(@class).returns("something")
@compiler.expects(:add_resource).never
@resource.expects(:evaluate).never
Puppet::Parser::Resource.expects(:new).never
@@ -457,6 +451,7 @@ describe Puppet::Parser::Compiler do
@compiler.stubs(:add_resource)
@scope.stubs(:find_hostclass).with("notfound").returns(nil)
+ @scope.stubs(:class_scope).with(@class)
Puppet::Parser::Resource.stubs(:new).returns(@resource)
@class.stubs :mk_plain_resource
@@ -534,7 +529,7 @@ describe Puppet::Parser::Compiler do
# The #evaluate method normally does this.
scope = stub 'scope', :source => "mysource"
- @compiler.class_set(node_class.name, scope)
+
@compiler
.topscope.expects(:class_scope).with(node_class).returns(scope)
node_resource.stubs(:evaluate)
@compiler.compile
@@ -582,23 +577,4 @@ describe Puppet::Parser::Compiler do
lambda { @compiler.compile }.should
raise_error(Puppet::ParseError)
end
end
-
- # #620 - Nodes and classes should conflict, else classes don't
get evaluated
- describe "when evaluating nodes and classes with the same name
(#620)" do
-
- before do
- @node = stub :nodescope? => true
- @class = stub :nodescope? => false
- end
-
- it "should fail if a node already exists with the same name
as the class being evaluated" do
- @compiler.class_set("one", @node)
- lambda { @compiler.class_set("one", @class) }.should
raise_error(Puppet::ParseError)
- end
-
- it "should fail if a class already exists with the same
name as the node being evaluated" do
- @compiler.class_set("one", @class)
- lambda { @compiler.class_set("one", @node) }.should
raise_error(Puppet::ParseError)
- end
- end
end
diff --git a/spec/unit/parser/resource_type.rb b/spec/unit/parser/
resource_type.rb
index ddf8ce9..3afab69 100755
--- a/spec/unit/parser/resource_type.rb
+++ b/spec/unit/parser/resource_type.rb
@@ -214,6 +214,7 @@ describe Puppet::Parser::ResourceType do
@scope.expects(:setvar).with("foo", "bar")
@scope.expects(:setvar).with("boo", "baz")
+ @scope.stubs(:class_set).with("foo",@scope)
@type.set_resource_parameters(@resource, @scope)
end
@@ -222,6 +223,7 @@ describe Puppet::Parser::ResourceType do
@type.set_arguments :foo => nil
@resource.expects(:to_hash).returns(:foo => "bar")
@scope.expects(:setvar).with("foo", "bar")
+ @scope.stubs(:class_set).with("foo",@scope)
@type.set_resource_parameters(@resource, @scope)
end
@@ -238,6 +240,7 @@ describe Puppet::Parser::ResourceType do
@resource.expects(:to_hash).returns({})
@scope.expects(:setvar).with("foo", "something")
+ @scope.stubs(:class_set).with("foo",@scope)
@type.set_resource_parameters(@resource, @scope)
end
@@ -254,6 +257,7 @@ describe Puppet::Parser::ResourceType do
@resource.expects(:title).returns 'teetle'
@scope.expects(:setvar).with("title", "teetle")
+ @scope.stubs(:class_set).with("foo",@scope)
@type.set_resource_parameters(@resource, @scope)
end
@@ -263,6 +267,7 @@ describe Puppet::Parser::ResourceType do
@resource.expects(:name).returns 'nombre'
@scope.expects(:setvar).with("name", "nombre")
+ @scope.stubs(:class_set).with("foo",@scope)
@type.set_resource_parameters(@resource, @scope)
end
diff --git a/spec/unit/parser/scope.rb b/spec/unit/parser/scope.rb
index d7800e4..04ae304 100755
--- a/spec/unit/parser/scope.rb
+++ b/spec/unit/parser/scope.rb
@@ -10,6 +10,41 @@ describe Puppet::Parser::Scope do
@scope = Puppet::Parser::Scope.new()
@scope.parent = @topscope
end
+
+ it "should be able to store references to class scopes" do
+ lambda { @scope.class_set "myname", "myscope" }.should_not
raise_error
+ end
+
+ it "should be able to retrieve class scopes by name" do
+ @scope.class_set "myname", "myscope"
+ @scope.class_scope("myname").should == "myscope"
+ end
+
+ it "should be able to retrieve class scopes by object" do
+ klass = mock 'ast_class'
+ klass.expects(:name).returns("myname")
+ @scope.class_set "myname", "myscope"
+ @scope.class_scope(klass).should == "myscope"
+ end
+
+ # #620 - Nodes and classes should conflict, else classes don't
get evaluated
+ describe "when evaluating nodes and classes with the same name
(#620)" do
+
+ before do
+ @node = stub :nodescope? => true
+ @class = stub :nodescope? => false
+ end
+
+ it "should fail if a node already exists with the same name
as the class being evaluated" do
+ @scope.class_set("one", @node)
+ lambda { @scope.class_set("one", @class) }.should
raise_error(Puppet::ParseError)
+ end
+
+ it "should fail if a class already exists with the same
name as the node being evaluated" do
+ @scope.class_set("one", @class)
+ lambda { @scope.class_set("one", @node) }.should
raise_error(Puppet::ParseError)
+ end
+ end
describe "when looking up a variable" do
it "should default to an empty string" do
@@ -52,7 +87,7 @@ describe Puppet::Parser::Scope do
klass = @parser.newclass(name)
Puppet::Parser::Resource.new(:type =>
"class", :title => name, :scope => @scope, :source =>
mock('source')).evaluate
- return @compiler.class_scope(klass)
+ return @scope.class_scope(klass)
end
it "should be able to look up explicitly fully qualified
variables from main" do
--
1.6.4
--
You received this message because you are subscribed to the Google
Groups "Puppet Developers" group.
To post to this group, send email to [email protected].
To unsubscribe from this group, send email to [email protected]
.
For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en
.
