Avoid the need to parse nil versions in the package provider

If we don't have a version installed yet we shouldn't go do a version
comparison with nil. No matter what that's a waste of time and in
dpkg/apt it's going to require shelling out now. Instead just do the
install.

This also adds the same version comparison method to the dpkg resource.
At the moment it doesn't look like this is going to get called since our
upgrade action there is a bit broken (doesn't actually compare
versions), but that's going to get fixed next.

This also cleans up the version comparison to make sure we're
always dealing with strings

Signed-off-by: Tim Smith <tsmith@chef.io>

4 files changed, 61 insertions, 4 deletions

diff --git a/lib/chef/provider/package.rb b/lib/chef/provider/package.rb
index 95d16dd666..7116bc9045 100644
--- a/lib/chef/provider/package.rb
+++ b/lib/chef/provider/package.rb
@@ -477,6 +477,9 @@ class Chef
                 elsif candidate_version.nil?
                   Chef::Log.debug("#{new_resource} #{package_name} has no candidate_version to upgrade to")
                   target_version_array.push(nil)
+                elsif current_version.nil?
+                  Chef::Log.debug("#{new_resource} has no existing installed version. Installing install #{candidate_version}")
+                  target_version_array.push(candidate_version)
                 elsif version_compare(current_version, candidate_version) == 1 && !new_resource.allow_downgrade
                   Chef::Log.debug("#{new_resource} #{package_name} has installed version #{current_version}, which is newer than available version #{candidate_version}. Skipping...)")
                   target_version_array.push(nil)
diff --git a/lib/chef/provider/package/apt.rb b/lib/chef/provider/package/apt.rb
index 605bbc70eb..3f32f9d380 100644
--- a/lib/chef/provider/package/apt.rb
+++ b/lib/chef/provider/package/apt.rb
@@ -127,12 +127,16 @@ class Chef
 
         private
 
+        # compare 2 versions to each other to see which is newer.
+        # this differs from the standard package method because we
+        # need to be able to parse debian version strings which contain
+        # tildes which Gem cannot properly parse
+        #
+        # @return [Integer] 1 if v1 > v2. 0 if they're equal. -1 if v1 < v2
         def version_compare(v1, v2)
-          dpkg_v1 = v1 || '0'
-          dpkg_v2 = v2 || '0'
-          if shell_out_compact_timeout("dpkg", "--compare-versions", dpkg_v1, "gt", dpkg_v2).status.success?
+          if !shell_out_compact_timeout("dpkg", "--compare-versions", v1.to_s, "gt", v2.to_s).error?
             1
-          elsif shell_out_compact_timeout("dpkg", "--compare-versions", dpkg_v1, "eq", dpkg_v2).status.success?
+          elsif !shell_out_compact_timeout("dpkg", "--compare-versions", v1.to_s, "eq", v2.to_s).error?
             0
           else
             -1
diff --git a/lib/chef/provider/package/dpkg.rb b/lib/chef/provider/package/dpkg.rb
index 89a57affac..cf92e6d3e7 100644
--- a/lib/chef/provider/package/dpkg.rb
+++ b/lib/chef/provider/package/dpkg.rb
@@ -106,6 +106,22 @@ class Chef
 
         private
 
+        # compare 2 versions to each other to see which is newer.
+        # this differs from the standard package method because we
+        # need to be able to parse debian version strings which contain
+        # tildes which Gem cannot properly parse
+        #
+        # @return [Integer] 1 if v1 > v2. 0 if they're equal. -1 if v1 < v2
+        def version_compare(v1, v2)
+          if !shell_out_compact_timeout("dpkg", "--compare-versions", v1.to_s, "gt", v2.to_s).error?
+            1
+          elsif !shell_out_compact_timeout("dpkg", "--compare-versions", v1.to_s, "eq", v2.to_s).error?
+            0
+          else
+            -1
+          end
+        end
+
         def read_current_version_of_package(package_name)
           Chef::Log.debug("#{new_resource} checking install state of #{package_name}")
           status = shell_out_compact_timeout!("dpkg", "-s", package_name, returns: [0, 1])
diff --git a/spec/unit/provider/package/apt_spec.rb b/spec/unit/provider/package/apt_spec.rb
index 46ae7fcadc..ff14edcffd 100644
--- a/spec/unit/provider/package/apt_spec.rb
+++ b/spec/unit/provider/package/apt_spec.rb
@@ -475,6 +475,40 @@ mpg123 1.12.1-0ubuntu1
           end
         end
 
+        describe "#action_install" do
+          it "should run dpkg to compare versions if an existing version is installed" do
+            allow(@provider).to receive(:get_current_versions).and_return("1.4.0")
+            allow(@new_resource).to receive(:allow_downgrade).and_return(false)
+            expect(@provider).to receive(:shell_out_compact_timeout).with(
+              "dpkg", "--compare-versions", "1.4.0", "gt", "0.8.12-7"
+            ).and_return(double(error?: false))
+            @provider.run_action(:upgrade)
+          end
+
+          it "should install the package if the installed version is older" do
+            allow(@provider).to receive(:get_current_versions).and_return("0.4.0")
+            allow(@new_resource).to receive(:allow_downgrade).and_return(false)
+            expect(@provider).to receive(:version_compare).and_return(-1)
+            expect(@provider).to receive(:shell_out!).with(
+              "apt-get", "-q", "-y", "install", "irssi=0.8.12-7",
+              :env => { "DEBIAN_FRONTEND" => "noninteractive" },
+              :timeout => @timeout
+            )
+            @provider.run_action(:upgrade)
+          end
+
+          it "should not compare versions if an existing version is not installed" do
+            allow(@provider).to receive(:get_current_versions).and_return(nil)
+            allow(@new_resource).to receive(:allow_downgrade).and_return(false)
+            expect(@provider).not_to receive(:version_compare)
+            expect(@provider).to receive(:shell_out!).with(
+              "apt-get", "-q", "-y", "install", "irssi=0.8.12-7",
+              :env => { "DEBIAN_FRONTEND" => "noninteractive" },
+              :timeout => @timeout
+            )
+            @provider.run_action(:upgrade)
+          end
+        end
       end
     end
   end