fix virtual package logic in check_package_state

since check_package_state calls itself to check virtual packages the
array building it does was getting indexed incorrectly as the virtual
packages were getting inserted into arrays that were supposed to be
indexed by the package arguments the user gave.  there was also a bug
which broke idempotency with virtual packages and would cause virtual
packages to get reinstalled every time (installed_version not geting
correctly inherited from the virtual package).

WIP2

remove debug

fix specs

2 files changed, 79 insertions, 89 deletions

diff --git a/lib/chef/provider/package/apt.rb b/lib/chef/provider/package/apt.rb
index 584d1cf75a..e426b51992 100644
--- a/lib/chef/provider/package/apt.rb
+++ b/lib/chef/provider/package/apt.rb
@@ -38,7 +38,7 @@ class Chef
         def load_current_resource
           @current_resource = Chef::Resource::Package.new(@new_resource.name)
           @current_resource.package_name(@new_resource.package_name)
-          check_package_state(@new_resource.package_name)
+          check_all_packages_state(@new_resource.package_name)
           @current_resource
         end
 
@@ -56,89 +56,91 @@ class Chef
           "-o APT::Default-Release=#{@new_resource.default_release}" if @new_resource.respond_to?(:default_release) && @new_resource.default_release
         end
 
-        def check_package_state(package)
-          final_installed_version = []
-          final_candidate_version = []
-          final_installed = []
-          final_virtual = []
-
-          [package].flatten.each do |pkg|
-            installed = virtual = false
-            installed_version = candidate_version = nil
-            shell_out!("apt-cache#{expand_options(default_release_options)} policy #{pkg}", {:timeout=>900}).stdout.each_line do |line|
-              case line
-              when /^\s{2}Installed: (.+)$/
-                installed_version = $1
-                if installed_version == '(none)'
-                  Chef::Log.debug("#{@new_resource} current version is nil")
-                  installed_version = nil
-                else
-                  Chef::Log.debug("#{@new_resource} current version is #{installed_version}")
-                  installed = true
-                end
-              when /^\s{2}Candidate: (.+)$/
-                candidate_version = $1
-                if candidate_version == '(none)'
-                  # This may not be an appropriate assumption, but it shouldn't break anything that already worked -- btm
-                  virtual = true
-                  showpkg = shell_out!("apt-cache showpkg #{pkg}", {:timeout => 900}).stdout
-                  providers = Hash.new
-                  showpkg.rpartition(/Reverse Provides: ?#{$/}/)[2].each_line do |line|
-                    provider, version = line.split
-                    providers[provider] = version
-                  end
-                  # Check if the package providing this virtual package is installed
-                  num_providers = providers.length
-                  raise Chef::Exceptions::Package, "#{@new_resource.package_name} has no candidate in the apt-cache" if num_providers == 0
-                  # apt will only install a virtual package if there is a single providing package
-                  raise Chef::Exceptions::Package, "#{@new_resource.package_name} is a virtual package provided by #{num_providers} packages, you must explicitly select one to install" if num_providers > 1
-                  # Check if the package providing this virtual package is installed
-                  Chef::Log.info("#{@new_resource} is a virtual package, actually acting on package[#{providers.keys.first}]")
-                  installed = check_package_state(providers.keys.first)
-                else
-                  Chef::Log.debug("#{@new_resource} candidate version is #{$1}")
+        def check_package_state(pkg)
+          is_virtual_package = false
+          installed          = false
+          installed_version  = nil
+          candidate_version  = nil
+
+          shell_out!("apt-cache#{expand_options(default_release_options)} policy #{pkg}", {:timeout=>900}).stdout.each_line do |line|
+            case line
+            when /^\s{2}Installed: (.+)$/
+              installed_version = $1
+              if installed_version == '(none)'
+                Chef::Log.debug("#{@new_resource} current version is nil")
+                installed_version = nil
+              else
+                Chef::Log.debug("#{@new_resource} current version is #{installed_version}")
+                installed = true
+              end
+            when /^\s{2}Candidate: (.+)$/
+              candidate_version = $1
+              if candidate_version == '(none)'
+                # This may not be an appropriate assumption, but it shouldn't break anything that already worked -- btm
+                is_virtual_package = true
+                showpkg = shell_out!("apt-cache showpkg #{pkg}", {:timeout => 900}).stdout
+                providers = Hash.new
+                showpkg.rpartition(/Reverse Provides: ?#{$/}/)[2].each_line do |line|
+                  provider, version = line.split
+                  providers[provider] = version
                 end
+                # Check if the package providing this virtual package is installed
+                num_providers = providers.length
+                raise Chef::Exceptions::Package, "#{@new_resource.package_name} has no candidate in the apt-cache" if num_providers == 0
+                # apt will only install a virtual package if there is a single providing package
+                raise Chef::Exceptions::Package, "#{@new_resource.package_name} is a virtual package provided by #{num_providers} packages, you must explicitly select one to install" if num_providers > 1
+                # Check if the package providing this virtual package is installed
+                Chef::Log.info("#{@new_resource} is a virtual package, actually acting on package[#{providers.keys.first}]")
+                ret = check_package_state(providers.keys.first)
+                installed = ret[:installed]
+                installed_version = ret[:installed_version]
+              else
+                Chef::Log.debug("#{@new_resource} candidate version is #{$1}")
               end
             end
-            if package.is_a?(Array)
-              final_installed_version << installed_version
-              final_candidate_version << candidate_version
-              final_installed << installed
-              final_virtual << virtual
-            else
-              final_installed_version = installed_version
-              final_candidate_version = candidate_version
-              final_installed = installed
-              final_virtual = virtual
-            end
           end
-          @candidate_version = final_candidate_version
-          @current_resource.version(final_installed_version)
+
+          return {
+            installed_version:   installed_version,
+            installed:           installed,
+            candidate_version:   candidate_version,
+            is_virtual_package:  is_virtual_package,
+          }
+        end
+
+        def check_all_packages_state(package)
+          installed_version = {}
+          candidate_version = {}
+          installed = {}
 
           [package].flatten.each do |pkg|
-            @is_virtual_package[pkg] = final_virtual
+            ret = check_package_state(pkg)
+            is_virtual_package[pkg] = ret[:is_virtual_package]
+            installed[pkg]          = ret[:installed]
+            installed_version[pkg]  = ret[:installed_version]
+            candidate_version[pkg]  = ret[:candidate_version]
           end
 
-          return final_installed.is_a?(Array) ? final_installed.any? : final_installed
+          if package.is_a?(Array)
+            @candidate_version = []
+            final_installed_version = []
+            [package].flatten.each do |pkg|
+              @candidate_version << candidate_version[pkg]
+              final_installed_version << installed_version[pkg]
+            end
+            @current_resource.version(final_installed_version)
+          else
+            @candidate_version = candidate_version[package]
+            @current_resource.version(installed_version[package])
+          end
         end
 
         def install_package(name, version)
-          if name.is_a?(Array)
-            index = 0
-            package_name = name.zip(version).map do |x, y|
-              namestr = nil
-              if is_virtual_package[name]
-                namestr = x
-              else
-                namestr = "#{x}=#{y}"
-              end
-              index += 1
-              namestr
-            end.join(' ')
-          else
-            package_name = "#{name}=#{version}"
-            package_name = name if is_virtual_package[name]
-          end
+          name_array = [ name ].flatten
+          version_array = [ version ].flatten
+          package_name = name_array.zip(version_array).map do |n, v|
+            is_virtual_package[n] ? n : "#{n}=#{v}"
+          end.join(' ')
           run_noninteractive("apt-get -q -y#{expand_options(default_release_options)}#{expand_options(@new_resource.options)} install #{package_name}")
         end
 
@@ -147,20 +149,12 @@ class Chef
         end
 
         def remove_package(name, version)
-          if name.is_a?(Array)
-            package_name = name.join(' ')
-          else
-            package_name = name
-          end
+          package_name = [ name ].flatten.join(' ')
           run_noninteractive("apt-get -q -y#{expand_options(@new_resource.options)} remove #{package_name}")
         end
 
         def purge_package(name, version)
-          if name.is_a?(Array)
-            package_name = name.join(' ')
-          else
-            package_name = "#{name}"
-          end
+          package_name = [ name ].flatten.join(' ')
           run_noninteractive("apt-get -q -y#{expand_options(@new_resource.options)} purge #{package_name}")
         end
 
@@ -170,11 +164,7 @@ class Chef
         end
 
         def reconfig_package(name, version)
-          if name.is_a?(Array)
-            package_name = name.join(' ')
-          else
-            package_name = "#{name}"
-          end
+          package_name = [ name ].flatten.join(' ')
           Chef::Log.info("#{@new_resource} reconfiguring")
           run_noninteractive("dpkg-reconfigure #{package_name}")
         end
diff --git a/spec/unit/provider/package/apt_spec.rb b/spec/unit/provider/package/apt_spec.rb
index 8b1d40aab9..2ca48fd1ed 100644
--- a/spec/unit/provider/package/apt_spec.rb
+++ b/spec/unit/provider/package/apt_spec.rb
@@ -334,7 +334,7 @@ mpg123 1.12.1-0ubuntu1
           end
 
           it "should not run debconf-set-selections if the preseed file has not changed" do
-            allow(@provider).to receive(:check_package_state)
+            allow(@provider).to receive(:check_all_packages_state)
             @current_resource.version "0.8.11"
             @new_resource.response_file "/tmp/file"
             allow(@provider).to receive(:get_preseed_file).and_return(false)