fpm icon indicating copy to clipboard operation
fpm copied to clipboard

Fix handling of single-digit semver constraints (re:#755)

Open josephholsten opened this issue 6 years ago • 10 comments

rebased #755

josephholsten avatar Jul 21 '17 18:07 josephholsten

@clayne mentioned in #755

This section of code was confusing and repetitive when reading it at first to try and figure out what was going on. I vote this to be a cleaner approach:

# If attributes provided, use those, otherwise if rpm-use-file-permissions is false, use rpm defaults.
if !attrs[file].nil?
  return sprintf("%%attr(%s) %s\n", attrs[file], file)
elsif !attributes[:rpm_use_file_permissions?]
  return file
end

Then kill line 148 and let it fall through to the rpm-use-file-permissions true case. Since the attribute check is winning in all cases if it's non-nil, there's no reason to be checking it twice. It's precedence is higher in the overall logic regardless of rpm-use-file-permissions being true or not.

josephholsten avatar Jul 21 '17 19:07 josephholsten

or do we just want #931/#1382?

josephholsten avatar Jul 21 '17 19:07 josephholsten

Hello, I am running into this issue when packaging aws-sdk gems. Are there plans to get this merged & released soon?

zipkid avatar Oct 09 '17 08:10 zipkid

@zipkid is this patch working for you currently? Looks like the failing test is a straightforward fix.

@jordansissel any objection to my fixing the failing test and merging?

josephholsten avatar Oct 09 '17 23:10 josephholsten

@josephholsten i manually added line 653 in lib/fpm/package/deb.rb and it works like a charm. The fail is indeed an earlier test in the spec.rb that fails on the changes.

zipkid avatar Oct 10 '17 05:10 zipkid

The failing is coming from spec/fpm/package/deb_spec.rb:227:

      it "should have the correct dependencies" do
        original.dependencies.each do |dep|
          insist { input.dependencies }.include?(dep)
        end
      end

which is throwing:

Expected "semver ~> 3" in ["something > 10", "hello >= 20", "semver >= 3.0", "semver < 4.0"]

I'm not entirely sure how this should actually be changed.

josephholsten avatar Oct 10 '17 19:10 josephholsten

0001-This-way-of-checking-the-semver-no-longer-makes-sens.patch.txt

diff --git a/spec/fpm/package/deb_spec.rb b/spec/fpm/package/deb_spec.rb
index 0017849..dbca977 100644
--- a/spec/fpm/package/deb_spec.rb
+++ b/spec/fpm/package/deb_spec.rb
@@ -138,7 +138,6 @@ describe FPM::Package::Deb do
       original.architecture = "all"
       original.dependencies << "something > 10"
       original.dependencies << "hello >= 20"
-      original.dependencies << "semver ~> 3"
       original.provides << "#{original.name} = #{original.version}"
 
       # Test to cover PR#591 (fix provides names)

vStone avatar Oct 16 '17 08:10 vStone

@josephholsten Could you rebase this pr and include the patch I added to fix the tests? Maybe we can get @jordansissel to merge it then

vStone avatar Jan 22 '18 08:01 vStone

@vStone sorry for the delay, I've git am'd your patch and am waiting for build now!

josephholsten avatar May 06 '18 16:05 josephholsten

One related (I think) test failed in Travis:

  4) FPM::Package::Deb#output when read with dpkg should have the correct dependency list
     Failure/Error: insist { dpkg_field("Depends") } == "something (>> 10), hello (>= 20), semver (>= 3.0), semver (<< 4.0)"
     Insist::Failure:
       Expected "something (>> 10), hello (>= 20), semver (>= 3.0), semver (<< 4.0)", but got "something (>> 10), hello (>= 20)"
     # ./spec/fpm/package/deb_spec.rb:263:in `block (4 levels) in <top (required)>'

Can you check?

jordansissel avatar May 13 '18 04:05 jordansissel