koji

Clone

#997 Fix rpmdiff's ignoring of size
Merged 5 years ago by mikem. Opened 5 years ago by tkopecek.
tkopecek/koji issue994a  into  master

file modified
+4 -4 
@@ -81,12 +81,12 @@ 
 

          if self.ignore is None:
 

              self.ignore = []
 

  

-         FILEIDX = self.__FILEIDX
 

+         FILEIDX = [entry[:] for entry in self.__FILEIDX]
 

          for tag in self.ignore:
 

              for entry in FILEIDX:
 

                  if tag == entry[0]:
 

                      # store marked position for erasing data
 

-                     entry[1] = -entry[1]
 

+                     entry[1] = -entry[1] - 100
 

                      break
 

  

          old = self.__load_pkg(old)
 
@@ -140,8 +140,8 @@ 
 

                          diff = 1
 

                      elif entry[1] < 0:
 

                          # erase fields which are ignored
 

-                         old_file[-entry[1]] = None
 

-                         new_file[-entry[1]] = None
 

+                         old_file[-entry[1] - 100] = None
 

+                         new_file[-entry[1] - 100] = None
 

                          format = format + '.'
 

                      else:
 

                          format = format + '.'

file added
empty or binary file added
file added
empty or binary file added
file added
empty or binary file added
file added
empty or binary file added
file modified
+46 
@@ -1,6 +1,7 @@ 
 

  from __future__ import absolute_import
 

  import copy
 

  import mock
 

+ import os
 

  try:
 

      import unittest2 as unittest
 

  except ImportError:
 
@@ -37,6 +38,51 @@ 
 

          Rpmdiff.assert_called_once_with('basepath/12/1234/foo', 'basepath/13/1345/bar', ignore='S5TN')
 

          d.textdiff.assert_called_once_with()
 

  

+     def test_rpmdiff_real_target(self):
 

+         data_path = os.path.abspath("tests/test_hub/data/rpms")
 

+ 

+         # the only differences between rpm1 and rpm2 are 1) create time 2) file name
 

+         rpm1 = os.path.join(data_path, 'test-pkg-1.0.0-1.el7.noarch.rpm')
 

+         rpm2 = os.path.join(data_path, 'test-pkg-1.0.0-1.fc24.noarch.rpm')
 

+ 

+         diff_output = "..........T /usr/share/test-pkg/test-doc01.txt\n" + \
 

+                       "..........T /usr/share/test-pkg/test-doc02.txt\n" + \
 

+                       "..........T /usr/share/test-pkg/test-doc03.txt\n" + \
 

+                       "..........T /usr/share/test-pkg/test-doc04.txt"
 

+ 

+         # case 1. no ignore option, timestamp is different
 

+         # perform twice check to verify issue: #994
 

+         for _ in range(0, 2):
 

+             d = koji.rpmdiff.Rpmdiff(rpm1, rpm2)
 

+             self.assertEqual(d.textdiff(), diff_output)
 

+ 

+         # case 2. ignore timestamp, two rpms should be the same
 

+         # perform twice check to verify issue: #994
 

+         for r in range(0, 2):
 

+             d = koji.rpmdiff.Rpmdiff(rpm1, rpm2, ignore='S5TN')
 

+             self.assertEqual(d.textdiff(), '')
 

+ 

+     def test_rpmdiff_size(self):
 

+         data_path = os.path.abspath("tests/test_hub/data/rpms")
 

+ 

+         # the only differences between rpm1 and rpm2 are 1) create time 2) file name
 

+         rpm1 = os.path.join(data_path, 'different_size_a.noarch.rpm')
 

+         rpm2 = os.path.join(data_path, 'different_size_b.noarch.rpm')
 

+ 

+         diff_output = "S.5.......T /bin/test"
 

+ 

+         # case 1. no ignore option, timestamp is different
 

+         # perform twice check to verify issue: #994
 

+         for _ in range(0, 2):
 

+             d = koji.rpmdiff.Rpmdiff(rpm1, rpm2)
 

+             self.assertEqual(d.textdiff(), diff_output)
 

+ 

+         # case 2. ignore timestamp, two rpms should be the same
 

+         # perform twice check to verify issue: #994
 

+         for r in range(0, 2):
 

+             d = koji.rpmdiff.Rpmdiff(rpm1, rpm2, ignore='S5TN')
 

+             self.assertEqual(d.textdiff(), '')
 

+ 

  class TestCheckNoarchRpms(unittest.TestCase):
 

      @mock.patch('kojihub.rpmdiff')
 

      def test_check_noarch_rpms_empty_invocation(self, rpmdiff):

Related: #994

entry[1] = -entry[1] - 100

The reasons here are somewhat non-obvious and I think this at least deserves a comment.

I wonder if there is a better, clearer way to handle this. We set up FILEIDX early on, but only use it at the end.

I like mike's changes, actually I have done similar change in my PR: #995. I would like to use mike's solution since its variable name is more straightforward.
By the way I noticed that self.ignore is initialized but never use afterwards, should we keep it ?

Edited 5 years ago by franzh

I noticed that self.ignore is initialized but never use afterwards

My changes above remove it. I did this mainly to avoid confusion with the local ignore var. Probably a relic from the original code.

@tkopecek what do you think?

I wonder if I should make this a separate PR since my changes replace most of the previous ones (except for the new unit test).

Yes, we can drop this one. Look also to #995 solving same problem.

I'll just use this PR with the update changes. merging shortly
Edited 5 years ago by mikem

Commit 05e7913 fixes this pull-request

Pull-Request has been merged by mikem
5 years ago
Changes Summary 6
+4 -4
file changed
koji/rpmdiff.py
+0
file added
tests/test_hub/data/rpms/different_size_a.noarch.rpm
+0
file added
tests/test_hub/data/rpms/different_size_b.noarch.rpm
+0
file added
tests/test_hub/data/rpms/test-pkg-1.0.0-1.el7.noarch.rpm
+0
file added
tests/test_hub/data/rpms/test-pkg-1.0.0-1.fc24.noarch.rpm
+46 -0
file changed
tests/test_hub/test_rpmdiff.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.