(#19875) Package descriptions obtained by rpm/dpkg providers

Previously Puppet::Provider::Package::Rpm and Dpkg implementations
obtained package information from the system without capturing package
description information.

This change modifies the rpm and dpkg-query format strings to include
descriptions.  This can be used for querying package state on a node.

The rpm implementation is a simple change relying on a SUMMARY field
which is a single line and which does not change the one line per
package parsing structure in the Rpm class.

The dpkg implementation is a more complicated change because we are
using a DESCRIPTION field in dpkg-query's format string, which is a
multi-line value.  The first line is a summary, and is all we want, but
dpkg-query then print's x additional lines of long description.  A new
parse_multi_line method has been added to treat these multi-line results
as a single entry, grab the first line with summary and ignore the
excess lines without triggering warnings.  Completely invalid package
entry results should still trigger warnings.

Current dpkg-query versions (as of 1.16.2 from 2012, I believe) have a
binary:Summary field which would return this to a single line parse and
make it simpler and less error prone again.  But earlier Debian/Ubuntu
installations don't have this (Ubuntu 12.04 for instance).

Dpkg.instance, Dpkg#query both used a Dpkg.dpkgquery_piped to expose an
IO pipe for parsing.  Regex match used in Rpm to ensure we only consume
good query output lines.

Author: jpartlow

lib/puppet/provider/package/dpkg.rb

@@ -11,39 +11,75 @@
   commands :dpkg_deb => "/usr/bin/dpkg-deb"
   commands :dpkgquery => "/usr/bin/dpkg-query"
 
+  # Performs a dpkgquery call with a pipe so that output can be processed
+  # inline in a passed block.
+  # @param args [Array<String>] any command line arguments to be appended to the command
+  # @param block expected to be passed on to execpipe
+  # @return whatever the block returns
+  # @see Puppet::Util::Execution.execpipe
+  # @api private
+  def self.dpkgquery_piped(*args, &block)
+    cmd = args.unshift(command(:dpkgquery))
+    Puppet::Util::Execution.execpipe(cmd, &block)
+  end
+
   def self.instances
     packages = []
 
     # list out all of the packages
-    cmd = "#{command(:dpkgquery)} -W --showformat '${Status} ${Package} ${Version}\\n'"
-    Puppet.debug "Executing '#{cmd}'"
-    Puppet::Util::Execution.execpipe(cmd) do |process|
-      # our regex for matching dpkg output
-      regex = %r{^(\S+) +(\S+) +(\S+) (\S+) (\S*)$}
-      fields = [:desired, :error, :status, :name, :ensure]
-      hash = {}
-
-      # now turn each returned line into a package object
-      process.each_line { |line|
-        if hash = parse_line(line)
-          packages << new(hash)
-        end
-      }
+    dpkgquery_piped('-W', '--showformat', self::DPKG_QUERY_FORMAT_STRING) do |pipe|
+      until pipe.eof?
+        hash = parse_multi_line(pipe)
+        packages << new(hash) if hash
+      end
     end
 
     packages
   end
 
-  self::REGEX = %r{^(\S+) +(\S+) +(\S+) (\S+) (\S*)$}
-  self::FIELDS = [:desired, :error, :status, :name, :ensure]
+  private
+
+  # Note: self:: is required here to keep these constants in the context of what will
+  # eventually become this Puppet:Type::Package::ProviderDpkg class.
+  self::DPKG_DESCRIPTION_DELIMITER = ':DESC:'
+  self::DPKG_QUERY_FORMAT_STRING = %Q{'${Status} ${Package} ${Version} #{self::DPKG_DESCRIPTION_DELIMITER} ${Description}\\n#{self::DPKG_DESCRIPTION_DELIMITER}\\n'}
+  self::FIELDS_REGEX = %r{^(\S+) +(\S+) +(\S+) (\S+) (\S*) #{self::DPKG_DESCRIPTION_DELIMITER} (.*)$}
+  self::FIELDS= [:desired, :error, :status, :name, :ensure, :description]
+  self::END_REGEX = %r{^#{self::DPKG_DESCRIPTION_DELIMITER}$}
+
+  # Handles parsing one package's worth of multi-line dpkg-query output.  Will
+  # emit warnings if it encounters an initial line that does not match
+  # DPKG_QUERY_FORMAT_STRING.  Swallows extra description lines silently.
+  #
+  # @param pipe [IO] the pipe yielded while processing dpkg output
+  # @return [Hash,nil] parsed dpkg-query entry as a hash of FIELDS strings or
+  # nil if we failed to parse
+  # @api private
+  def self.parse_multi_line(pipe)
+
+    line = pipe.gets
+    unless hash = parse_line(line)
+      Puppet.warning "Failed to match dpkg-query line #{line.inspect}"
+      return nil
+    end
+
+    consume_excess_description(pipe)
+
+    return hash
+  end
 
+  # @param line [String] one line of dpkg-query output
+  # @return [Hash,nil] a hash of FIELDS or nil if we failed to match
+  # @api private
   def self.parse_line(line)
-    if match = self::REGEX.match(line)
+    hash = nil
+
+    if match = self::FIELDS_REGEX.match(line)
       hash = {}
 
-      self::FIELDS.zip(match.captures) { |field,value|
+      self::FIELDS.zip(match.captures) do |field,value|
         hash[field] = value
-      }
+      end
 
       hash[:provider] = self.name
 
@@ -53,14 +89,33 @@ def self.parse_line(line)
         hash[:ensure] = :absent
       end
       hash[:ensure] = :held if hash[:desired] == 'hold'
-    else
-      Puppet.warning "Failed to match dpkg-query line #{line.inspect}"
-      return nil
     end
 
-    hash
+    return hash
   end
 
+  # Silently consumes the extra description lines from dpkg-query and brings
+  # us to the next package entry start.
+  #
+  # @note dpkg-query Description field has a one line summary and a multi-line
+  # description.  dpkg-query binary:Summary is what we want to use but was
+  # introduced in 2012 dpkg 1.16.2
+  # (https://launchpad.net/debian/+source/dpkg/1.16.2) and is not not available
+  # in older Debian versions.  So we're placing a delimiter marker at the end
+  # of the description so we can consume and ignore the multiline description
+  # without issuing warnings
+  #
+  # @param pipe [IO] the pipe yielded while processing dpkg output
+  # @return nil
+  def self.consume_excess_description(pipe)
+    until pipe.eof?
+      break if self::END_REGEX.match(pipe.gets)
+    end
+    return nil
+  end
+
+  public
+
   def install
     unless file = @resource[:source]
       raise ArgumentError, "You cannot install dpkg packages without a source"
@@ -94,26 +149,24 @@ def latest
   end
 
   def query
-    packages = []
-
-    fields = [:desired, :error, :status, :name, :ensure]
-
-    hash = {}
+    hash = nil
 
     # list out our specific package
     begin
-      output = dpkgquery(
+      self.class.dpkgquery_piped(
         "-W",
         "--showformat",
-        '${Status} ${Package} ${Version}\\n',
+        self.class::DPKG_QUERY_FORMAT_STRING,
         @resource[:name]
-      )
+      ) do |pipe|
+        hash = self.class.parse_multi_line(pipe)
+      end
     rescue Puppet::ExecutionFailure
       # dpkg-query exits 1 if the package is not found.
       return {:ensure => :purged, :status => 'missing', :name => @resource[:name], :error => 'ok'}
     end
 
-    hash = self.class.parse_line(output) || {:ensure => :absent, :status => 'missing', :name => @resource[:name], :error => 'ok'}
+    hash ||= {:ensure => :absent, :status => 'missing', :name => @resource[:name], :error => 'ok'}
 
     if hash[:error] != "ok"
       raise Puppet::Error.new(
@@ -148,4 +201,5 @@ def unhold
       execute([:dpkg, "--set-selections"], :failonfail => false, :combine => false, :stdinfile => tmpfile.path.to_s)
     end
   end
+
 end

lib/puppet/provider/package/rpm.rb

@@ -7,9 +7,13 @@
 
   has_feature :versionable
 
+  # Note: self:: is required here to keep these constants in the context of what will
+  # eventually become this Puppet:Type::Package::ProviderRpm class.
+  self::RPM_DESCRIPTION_DELIMITER = ':DESC:'
   # The query format by which we identify installed packages
-  NEVRAFORMAT = "%{NAME} %|EPOCH?{%{EPOCH}}:{0}| %{VERSION} %{RELEASE} %{ARCH}"
-  NEVRA_FIELDS = [:name, :epoch, :version, :release, :arch]
+  self::NEVRA_FORMAT = %Q{'%{NAME} %|EPOCH?{%{EPOCH}}:{0}| %{VERSION} %{RELEASE} %{ARCH} #{self::RPM_DESCRIPTION_DELIMITER} %{SUMMARY}\\n'}
+  self::NEVRA_REGEX  = %r{^(\S+) (\S+) (\S+) (\S+) (\S+) #{self::RPM_DESCRIPTION_DELIMITER} ?(.*)$}
+  self::NEVRA_FIELDS = [:name, :epoch, :version, :release, :arch, :description]
 
   commands :rpm => "rpm"
 
@@ -44,11 +48,11 @@ def self.instances
 
     # list out all of the packages
     begin
-      execpipe("#{command(:rpm)} -qa #{nosignature} #{nodigest} --qf '#{NEVRAFORMAT}\n'") { |process|
+      execpipe("#{command(:rpm)} -qa #{nosignature} #{nodigest} --qf #{self::NEVRA_FORMAT}") { |process|
         # now turn each returned line into a package object
         process.each_line { |line|
           hash = nevra_to_hash(line)
-          packages << new(hash)
+          packages << new(hash) unless hash.empty?
         }
       }
     rescue Puppet::ExecutionFailure
@@ -65,7 +69,7 @@ def query
     #NOTE: Prior to a fix for issue 1243, this method potentially returned a cached value
     #IF YOU CALL THIS METHOD, IT WILL CALL RPM
     #Use get(:property) to check if cached values are available
-    cmd = ["-q", @resource[:name], "#{self.class.nosignature}", "#{self.class.nodigest}", "--qf", "#{NEVRAFORMAT}\n"]
+    cmd = ["-q", @resource[:name], "#{self.class.nosignature}", "#{self.class.nodigest}", "--qf", self.class::NEVRA_FORMAT]
 
     begin
       output = rpm(*cmd)
@@ -74,7 +78,7 @@ def query
     end
 
     # FIXME: We could actually be getting back multiple packages
-    # for multilib
+    # for multilib and this will only return the first such package
     @property_hash.update(self.class.nevra_to_hash(output))
 
     @property_hash.dup
@@ -86,7 +90,7 @@ def latest
       @resource.fail "RPMs must specify a package source"
     end
 
-    cmd = [command(:rpm), "-q", "--qf", "#{NEVRAFORMAT}\n", "-p", "#{@resource[:source]}"]
+    cmd = [command(:rpm), "-q", "--qf", self.class::NEVRA_FORMAT, "-p", source]
     h = self.class.nevra_to_hash(execfail(cmd, Puppet::Error))
     h[:ensure]
   end
@@ -135,12 +139,25 @@ def update
     self.install
   end
 
+  private
+
+  # @param line [String] one line of rpm package query information
+  # @return [Hash] of NEVRA_FIELDS strings parsed from package info
+  # if we failed to parse
+  # @note warns if failed to match a line, and returns an empty Hash.
+  # @api private
   def self.nevra_to_hash(line)
-    line.chomp!
+    line.strip!
     hash = {}
-    NEVRA_FIELDS.zip(line.split) { |f, v| hash[f] = v }
-    hash[:provider] = self.name
-    hash[:ensure] = "#{hash[:version]}-#{hash[:release]}"
-    hash
+
+    if match = self::NEVRA_REGEX.match(line)
+      self::NEVRA_FIELDS.zip(match.captures) { |f, v| hash[f] = v }
+      hash[:provider] = self.name
+      hash[:ensure] = "#{hash[:version]}-#{hash[:release]}"
+    else
+      Puppet.warning "Failed to match rpm line #{line}"
+    end
+
+    return hash
   end
 end

spec/unit/provider/package/apt_spec.rb

@@ -8,7 +8,11 @@
     @resource = stub 'resource', :[] => "asdf"
     @provider = provider.new(@resource)
 
-    @fakeresult = "install ok installed asdf 1.0\n"
+    @fakeresult = <<-EOF
+install ok installed asdf 1.0 "asdf summary
+ asdf multiline description
+ with multiple lines
+EOF
   end
 
   it "should be versionable" do

spec/unit/provider/package/aptitude_spec.rb

@@ -10,13 +10,13 @@
   it { should be_versionable }
 
   context "when retrieving ensure" do
-    { :absent   => "deinstall ok config-files faff 1.2.3-1\n",
-      "1.2.3-1" => "install ok installed faff 1.2.3-1\n",
+    { :absent   => "deinstall ok config-files faff 1.2.3-1 :DESC: faff summary\n:DESC:\n",
+      "1.2.3-1" => "install ok installed faff 1.2.3-1 :DESC: faff summary\n:DESC:\n",
     }.each do |expect, output|
       it "should detect #{expect} packages" do
-        pkg.provider.expects(:dpkgquery).
-          with('-W', '--showformat', '${Status} ${Package} ${Version}\n', 'faff').
-          returns(output)
+        Puppet::Util::Execution.expects(:execpipe).
+          with([pkg.provider.command(:dpkgquery), '-W', '--showformat', "'${Status} ${Package} ${Version} :DESC: ${Description}\\n:DESC:\\n'", 'faff']).
+          yields(StringIO.new(output))
 
         pkg.property(:ensure).retrieve.should == expect
       end

spec/unit/provider/package/aptrpm_spec.rb

@@ -19,7 +19,7 @@
     def rpm
       pkg.provider.expects(:rpm).
         with('-q', 'faff', '--nosignature', '--nodigest', '--qf',
-             "%{NAME} %|EPOCH?{%{EPOCH}}:{0}| %{VERSION} %{RELEASE} %{ARCH}\n")
+             "'%{NAME} %|EPOCH?{%{EPOCH}}:{0}| %{VERSION} %{RELEASE} %{ARCH} :DESC: %{SUMMARY}\\n'")
     end
 
     it "should report absent packages" do
@@ -28,7 +28,7 @@ def rpm
     end
 
     it "should report present packages correctly" do
-      rpm.returns("faff-1.2.3-1 0 1.2.3-1 5 i686\n")
+      rpm.returns("faff-1.2.3-1 0 1.2.3-1 5 i686 :DESC: faff desc\n")
       pkg.property(:ensure).retrieve.should == "1.2.3-1-5"
     end
   end