Merge pull request #205 from glennsarti/gh-204-fix-debug-resource

(GH-204) Fix debug server for Puppet 4.x

Author: jpogran

appveyor.yml

@@ -9,19 +9,33 @@ environment:
   # LTS Agent
   - PUPPET_GEM_VERSION: "~> 4.7.0"
     RUBY_VER: 21-x64
-    RAKE_TASK: test
+    RAKE_TASK: test_languageserver
   # Latest 4.x branch
   - PUPPET_GEM_VERSION: "~> 4.0"
     RUBY_VER: 21-x64
-    RAKE_TASK: test
+    RAKE_TASK: test_languageserver
   # Latest Puppet
   - PUPPET_GEM_VERSION: "~> 5.0"
     RUBY_VER: 24-x64
-    RAKE_TASK: test
+    RAKE_TASK: test_languageserver
   # Specific Puppet version testing
   - PUPPET_GEM_VERSION: "5.1.0"
     RUBY_VER: 24-x64
-    RAKE_TASK: test
+    RAKE_TASK: test_languageserver
+
+  # Debug Server testing
+  # LTS Agent
+  - PUPPET_GEM_VERSION: "~> 4.7.0"
+    RUBY_VER: 21-x64
+    RAKE_TASK: test_debugserver
+  # Latest 4.x branch
+  - PUPPET_GEM_VERSION: "~> 4.0"
+    RUBY_VER: 21-x64
+    RAKE_TASK: test_debugserver
+  # Latest Puppet
+  - PUPPET_GEM_VERSION: "~> 5.0"
+    RUBY_VER: 24-x64
+    RAKE_TASK: test_debugserver
 
   # Ruby style
   - PUPPET_GEM_VERSION: "~> 4.0"

server/Rakefile

@@ -3,16 +3,16 @@ require 'rspec/core/rake_task'
 rubocop_available = Gem::Specification::find_all_by_name('rubocop').any?
 require 'rubocop/rake_task' if rubocop_available
 
-desc 'Run rspec tests with coloring.'
-RSpec::Core::RakeTask.new(:test) do |t|
-  t.rspec_opts = %w[--color --format documentation]
-  t.pattern    = 'spec/'
+desc 'Run rspec tests for the Language Server with coloring.'
+RSpec::Core::RakeTask.new(:test_languageserver) do |t|
+  t.rspec_opts = %w[--color --format documentation --default-path spec/languageserver]
+  t.pattern    = 'spec/languageserver'
 end
 
-desc 'Run rspec tests and save JUnit output to results.xml.'
-RSpec::Core::RakeTask.new(:junit) do |t|
-  t.rspec_opts = %w[-r yarjuf -f JUnit -o results.xml]
-  t.pattern    = 'spec/'
+desc 'Run rspec tests for the Debug Server with coloring.'
+RSpec::Core::RakeTask.new(:test_debugserver) do |t|
+  t.rspec_opts = %w[--color --format documentation --default-path spec/debugserver]
+  t.pattern    = 'spec/debugserver'
 end
 
 if rubocop_available
@@ -54,7 +54,7 @@ task :gem_revendor do
   gem_list.each do |vendor|
     puts "Vendoring #{vendor[:directory]}..."
     gem_dir = File.join(vendor_dir,vendor[:directory])
-    
+
     sh "git clone #{vendor[:github_repo]} #{gem_dir}"
     Dir.chdir(gem_dir) do
       sh 'git fetch origin'

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/hooks.rb

@@ -2,7 +2,7 @@
 
 module PuppetDebugServer
   # This code was inspired from Puppet-Debugger (https://raw.githubusercontent.com/nwops/puppet-debugger/master/lib/puppet-debugger/hooks.rb) which was borrowed from Pry hooks file
-  
+
   # Both puppet-debugger and pry are licensed with MIT
   # https://raw.githubusercontent.com/nwops/puppet-debugger/master/LICENSE.txt
   # https://raw.githubusercontent.com/pry/pry/master/LICENSE
@@ -28,7 +28,7 @@ module PuppetDebugServer
   #     String - Breakpoint full text
   #     Object - self where the function breakpoint was hit
   #     Object[] - optional objects
-  # 
+  #
   # :hook_exception
   #   Fires when a exception is hit
   #   Arguments:
@@ -58,6 +58,18 @@ module PuppetDebugServer
   #   Fires after the Puppet::Parser::Functions is reset
   #   Arguments:
   #     Puppet::Parser::Functions - Instance of Puppet::Parser::Functions
+  #
+  # :hook_before_pops_evaluate
+  #   Fires before an item in the AST is evaluated
+  #   Arguments:
+  #     The Pops object about to be evaluated
+  #     The scope of the Pops object
+  #
+  # :hook_after_pops_evaluate
+  #   Fires after an item in the AST is evaluated
+  #   Arguments:
+  #     The Pops object about to be evaluated
+  #     The scope of the Pops object
 
   class Hooks
     def initialize
@@ -114,6 +126,7 @@ def add_hook(event_name, hook_name, callable=nil, &block)
     # @param [Array] args The arguments to pass to each hook function.
     # @return [Object] The return value of the last executed hook.
     def exec_hook(event_name, *args, &block)
+      PuppetDebugServer.log_message(:debug, "Starting to executing hook #{event_name}") unless event_name == :hook_log_message
       @hooks[event_name.to_s].map do |hook_name, callable|
         begin
           callable.call(*args, &block)
@@ -122,6 +135,7 @@ def exec_hook(event_name, *args, &block)
           e
         end
       end.last
+      PuppetDebugServer.log_message(:debug, "Finished to executing hook #{event_name}") unless event_name == :hook_log_message
     end
 
     # @param [Symbol] event_name The name of the event.

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