(GH-204) Add support for Puppet 4.x to the Debug Server

Previously the Debug Server only support Puppet 5 agents.  However a large number
of users still run Puppet 4.x.  This commit:

* Refactors function calls which can differ between Puppet 4 and 5 into helper
  methods. Namely positioning information and class name shortcuts.

* Identifies other potential issues which use Puppet 5 only concepts

Author: glennsarti

server/lib/puppet-debugserver/debug_hook_handlers.rb

@@ -1,7 +1,7 @@
 module PuppetDebugServer
   module PuppetDebugSession
     @hook_handler = nil
-    
+
     def self.hooks
       if @hook_handler.nil?
         @hook_handler = PuppetDebugServer::Hooks.new
@@ -25,49 +25,49 @@ def self.hook_before_pops_evaluate(args)
       target = args[1]
       # Ignore this if there is no positioning information available
       return unless target.is_a?(Puppet::Pops::Model::Positioned)
+      target_loc = get_location_from_pops_object(target)
+
       # Even if it's positioned, it can still contain invalid information.  Ignore it if
       # it's missing required information.  This can happen when evaluting strings (e.g. watches from VSCode)
       # i.e. not a file on disk
-      return if target.file.nil? || target.file.empty?
+      return if target_loc.file.nil? || target_loc.file.empty?
+      target_classname = get_puppet_class_name(target)
+      ast_classname = get_ast_class_name(target)
 
       # Break if we hit a specific puppet function
-      if target._pcore_type.simple_name == 'CallNamedFunctionExpression'
+      if target_classname == 'CallNamedFunctionExpression'
         # TODO Do we really need to break on a function called breakpoint?
         if target.functor_expr.value == 'breakpoint'
           # Re-raise the hook as a breakpoint
-          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_function_breakpoint, [target.functor_expr.value, target._pcore_type.name] +args)
+          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_function_breakpoint, [target.functor_expr.value, ast_classname] +args)
           return
         else
           func_names = PuppetDebugServer::PuppetDebugSession.function_breakpoints
           func_names.each do |func|
             next unless func['name'] == target.functor_expr.value
             # Re-raise the hook as a breakpoint
-            PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_function_breakpoint, [target.functor_expr.value, target._pcore_type.name] + args)
+            PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_function_breakpoint, [target.functor_expr.value, ast_classname] + args)
             return
           end
         end
       end
 
-      unless target.length == 0
+      unless target_loc.length == 0
         excluded_classes = ['BlockExpression','HostClassDefinition']
-        file_path = target.file
+        file_path = target_loc.file
         breakpoints = PuppetDebugServer::PuppetDebugSession.source_breakpoints(file_path)
-
-        #target._pcore_type.simple_name
         # TODO should check if it's an object we don't care aount
-
-        unless excluded_classes.include?(target._pcore_type.simple_name) || breakpoints.nil? || breakpoints.empty?
+        unless excluded_classes.include?(target_classname) || breakpoints.nil? || breakpoints.empty?
           # Calculate the start and end lines of the target
-          target_start_line = target.line
-          target_end_line   = target.locator.line_for_offset(target.offset + target.length)
+          target_start_line = target_loc.line
+          target_end_line   = line_for_offset(target, target_loc.offset + target_loc.length)
 
           breakpoints.each do |bp|
             bp_line = bp['line']
             # TODO Hit and conditional BreakPoints?
             if bp_line >= target_start_line && bp_line <= target_end_line
               # Re-raise the hook as a breakpoint
-              PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_breakpoint, [target._pcore_type.name, ''] + args)
-              #require 'pry'; binding.pry
+              PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_breakpoint, [ast_classname, ''] + args)
               return
             end
           end
@@ -79,26 +79,27 @@ def self.hook_before_pops_evaluate(args)
       when :stepin
         # Stepping-in is basically break on everything
         # Re-raise the hook as a step breakpoint
-        PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [target._pcore_type.name, ''] + args)
+        PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [ast_classname, ''] + args)
         return
       when :next
         # Next will break on anything at this Pop depth or shallower
         # Re-raise the hook as a step breakpoint
         run_options = PuppetDebugServer::PuppetDebugSession.run_mode_options
         if !run_options[:pops_depth_level].nil? && @session_pops_eval_depth <= run_options[:pops_depth_level]
-          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [target._pcore_type.name, ''] + args)
+          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [ast_classname, ''] + args)
           return
         end
       when :stepout
         # Stepping-Out will break on anything shallower than this Pop depth
         # Re-raise the hook as a step breakpoint
         run_options = PuppetDebugServer::PuppetDebugSession.run_mode_options
         if !run_options[:pops_depth_level].nil? && @session_pops_eval_depth < run_options[:pops_depth_level]
-          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [target._pcore_type.name, ''] + args)
+          PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_step_breakpoint, [ast_classname, ''] + args)
           return
         end
       end
     end
+
     def self.hook_after_pops_evaluate(args)
       @session_pops_eval_depth = @session_pops_eval_depth - 1
       target = args[1]
@@ -191,7 +192,7 @@ def self.hook_log_message(args)
       str = msg.respond_to?(:multiline) ? msg.multiline : msg.to_s
       str = msg.source == 'Puppet' ? str : "#{msg.source}: #{str}"
 
-      level = msg.level.to_s.capitalize 
+      level = msg.level.to_s.capitalize
 
       category = 'stderr'
       category = 'stdout' if msg.level == :notice || msg.level == :info || msg.level == :debug

server/lib/puppet-debugserver/puppet_debug_session.rb

@@ -15,7 +15,7 @@ module PuppetDebugSession
     @session_compiler = nil # TODO Not sure we need this
     @session_paused_state = {}
     @session_variables_cache = {}
-    
+
     def self.puppet_thread_id
       @puppet_thread.object_id.to_i unless @puppet_thread.nil?
     end
@@ -78,7 +78,7 @@ def self.continue_stepout_session
         clear_paused_state
       }
     end
-    
+
     def self.continue_next_session
       @session_mutex.synchronize {
         @session_paused = false
@@ -159,7 +159,7 @@ def self.raise_and_wait_stopped_event(reason, description, text, options = {})
       @session_paused_state[:pops_target]       = options[:pops_target] unless options[:pops_target].nil?
       @session_paused_state[:scope]             = options[:scope] unless options[:scope].nil?
       @session_paused_state[:pops_depth_level]  = options[:pops_depth_level] unless options[:pops_depth_level].nil?
-      
+
       PuppetDebugServer::PuppetDebugSession.connection.send_stopped_event(reason, {
         'description' => description,
         'text'        => text,
@@ -323,13 +323,14 @@ def self.generate_stackframe_list(options = {})
 
         frame = {
           'id' => stack_frames.count,
-          'name' => target._pcore_type.simple_name.to_s,
+          'name' => get_puppet_class_name(target),
           'line' => 0,
           'column' => 0,
         }
 
         # TODO need to check on the client capabilities of zero or one based indexes
         if target.is_a?(Puppet::Pops::Model::Positioned)
+          # TODO - Potential issue here with 4.10.x not implementing .file on the Positioned class
           frame['source'] = PuppetDebugServer::Protocol::Source.create({
             'path'   => target.file,
           })
@@ -346,7 +347,7 @@ def self.generate_stackframe_list(options = {})
 
         stack_frames << frame
       end
-      
+
       # Generate StackFrame for an error
       unless @session_paused_state[:exception].nil?
         err = @session_paused_state[:exception]
@@ -358,6 +359,7 @@ def self.generate_stackframe_list(options = {})
         }
 
         # TODO need to check on the client capabilities of zero or one based indexes
+        # TODO - Potential issue here with 4.10.x not implementing .file on the Positioned class
         unless err.file.nil? || err.line.nil?
           frame['source'] = PuppetDebugServer::Protocol::Source.create({
             'path'   => err.file,
@@ -388,11 +390,63 @@ def self.generate_stackframe_list(options = {})
           stack_frames << frame
         end
       end
-      
+
       stack_frames
     end
 
+    # Public method but only called publicly for
+    # for testing
+    def self.reset_pops_eval_depth
+      @session_pops_eval_depth = 0
+    end
+
     # Private methods
+    def self.get_location_from_pops_object(obj)
+      pos = SourcePosition.new()
+      return pos unless obj.is_a?(Puppet::Pops::Model::Positioned)
+
+      if obj.respond_to?(:file) && obj.respond_to?(:line)
+        # These methods were added to the Puppet::Pops::Model::Positioned in Puppet 5.x
+        pos.file   = obj.file
+        pos.line   = obj.line
+        pos.offset = obj.offset
+        pos.length = obj.length
+      else
+        # Revert to Puppet 4.x location information.  A little more expensive to call
+        obj_loc = Puppet::Pops::Utils.find_closest_positioned(obj)
+        pos.file   = obj_loc.locator.file
+        pos.line   = obj_loc.line
+        pos.offset = obj_loc.offset
+        pos.length = obj_loc.length
+      end
+
+      pos
+    end
+
+    def self.get_puppet_class_name(obj)
+      # Puppet 5 has PCore Types
+      return obj._pcore_type.simple_name if obj.respond_to?(:_pcore_type)
+      # .. otherwise revert to simple naive text splitting
+      # e.g. Puppet::Pops::Model::CallNamedFunctionExpression becomes CallNamedFunctionExpression
+      obj.class.to_s.split('::').last
+    end
+
+    def self.get_ast_class_name(obj)
+      # Puppet 5 has PCore Types
+      return obj._pcore_type.name if obj.respond_to?(:_pcore_type)
+      # .. otherwise revert to Pops classname
+      obj.class.to_s
+    end
+
+    def self.line_for_offset(obj, offset)
+      # Puppet 5 exposes the source locator on the Pops object
+      return obj.locator.line_for_offset(offset) if obj.respond_to?(:locator)
+
+      # Revert to Puppet 4.x location information.  A little more expensive to call
+      obj_loc = Puppet::Pops::Utils.find_closest_positioned(obj)
+      obj_loc.locator.line_for_offset(offset)
+    end
+
     def self.debug_session_watcher
       loop do
         sleep(1)
@@ -414,8 +468,8 @@ def self.start_puppet
       cmd_args << '--noop' if @session_options['noop'] == true
       cmd_args.push(*@session_options['args']) unless @session_options['args'].nil?
 
-      @session_pops_eval_depth = 0
-      
+      reset_pops_eval_depth
+
       # Send experimental warning
       PuppetDebugServer::PuppetDebugSession.connection.send_output_event({
         'category' => 'console',
@@ -428,5 +482,12 @@ def self.start_puppet
       })
       Puppet::Util::CommandLine.new('puppet.rb',cmd_args).execute
     end
+
+    class SourcePosition
+      attr_accessor :file
+      attr_accessor :line
+      attr_accessor :offset
+      attr_accessor :length
+    end
   end
 end

server/lib/puppet-debugserver/puppet_monkey_patches.rb

@@ -20,7 +20,7 @@ def self.compile(node, code_id = nil)
       # Based on puppet/lib/puppet/parser/compiler.rb
       begin
         node.environment.check_for_reparse
-      
+
         errors = node.environment.validation_errors
         if !errors.empty?
           errors.each { |e| Puppet.err(e) } if errors.size > 1
@@ -52,6 +52,7 @@ def self.compile(node, code_id = nil)
         raise Puppet::Error, message, detail.backtrace
       end
     rescue Puppet::ParseErrorWithIssue => detail
+      # TODO - Potential issue here with 4.10.x not implementing .file on the Positioned class
       # Just re-raise if there is no Puppet manifest file associated with the error
       raise if detail.file.nil? || detail.line.nil? || detail.pos.nil?
       PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_exception, [detail])
@@ -88,7 +89,7 @@ def evaluate(target, scope)
     result = original_evaluate(target, scope)
 
     PuppetDebugServer::PuppetDebugSession.hooks.exec_hook(:hook_after_pops_evaluate, [self, target, scope])
-    
+
     result
   end
 end

server/spec/debugserver/fixtures/cache/facts.d/.gitkeep

@@ -0,0 +1,7 @@
+# Text fixture puppet.conf which redirects all Puppet configuration into the
+# fixtures directory
+[main]
+environment = test-fixtures
+environmentpath = $codedir/environments
+codedir = $vardir/..
+logdir = $vardir/../logdir

server/spec/debugserver/fixtures/cache/lib/facter/.gitkeep

@@ -5,8 +5,12 @@
 
 require 'puppet-debugserver'
 fixtures_dir = File.join(File.dirname(__FILE__),'fixtures')
-
+# Currently there is no way to re-initialize the puppet loader so for the moment
+# all tests must run off the single puppet config settings instead of per example setting
+puppet_settings = ['--vardir',File.join(fixtures_dir,'cache'),
+                   '--confdir',File.join(fixtures_dir,'confdir')]
 PuppetDebugServer::init_puppet(PuppetDebugServer::CommandLineParser.parse([]))
+Puppet.initialize_settings(puppet_settings)
 
 # Custom RSpec Matchers