Merge ~jslarraz/review-tools:rework-get-files-list into review-tools:master

[Jorge Sancho Larraz]

many: rework get_files_list and get_compiled_binaries_list

[Alex Murray (alexmurray)]

LGTM!

review: Approve

diff --git a/reviewtools/common.py b/reviewtools/common.py
index 4b50dc5..e2e80ff 100644
--- a/reviewtools/common.py
+++ b/reviewtools/common.py
@@ -86,12 +86,6 @@ MAX_COMPRESSED_SIZE = 5
 # 90% of disk but not larger than this
 MAX_UNCOMPRESSED_SIZE = 25
 
-# cache the expensive magic calls
-PKG_BIN_FILES = None
-
-# cache gathering all the files
-PKG_FILES = None
-
 # os release map
 OS_RELEASE_MAP = {
     "ubuntu": {
diff --git a/reviewtools/containers/base_container.py b/reviewtools/containers/base_container.py
index 3cd069f..9e6f104 100644
--- a/reviewtools/containers/base_container.py
+++ b/reviewtools/containers/base_container.py
@@ -125,32 +125,56 @@ class BaseContainer:
         with open(os.path.join(self.unpack_dir, fn), "rb") as fd:
             return fd.read()
 
-    def get_files_list(self):
-        """List all files included in this package."""
+    def get_files_list(self, abs: bool = False) -> list:
+        """
+        Returns a list of files present in the container.
+
+        Args:
+            abs (bool): if ``True``, absolute paths are used, otherwise paths are
+                relative to unpack_dir directory. Defaults to ``False``
+
+        Returns:
+            list(str): list of files present in the container
+        """
         files = []
         for root, dirnames, filenames in os.walk(self.unpack_dir):
             for f in filenames:
-                files.append(os.path.join(root, f))
+                file = os.path.join(root, f)
+                if not abs:
+                    file = os.path.relpath(file, self.unpack_dir)
+                files.append(file)
         return files
 
-    def get_compiled_binaries_list(self):
-        """List all compiled binaries in this package."""
-        self.mime = magic.open(magic.MAGIC_MIME)
-        self.mime.load()
+    def get_compiled_binaries_list(self, abs: bool = False) -> list:
+        """
+        Returns a list of compiled binaries present in the container.
+
+        Args:
+            abs (bool): if ``True``, absolute paths are used, otherwise paths are
+                relative to unpack_dir directory. Defaults to ``False``
+
+        Returns:
+            list(str): list of files present in the container
+        """
+        mime = magic.open(magic.MAGIC_MIME)
+        mime.load()
         bin_files = []
-        for i in self.files:
+        for file in self.get_files_list(abs=True):
             try:
-                res = self.mime.file(i)
+                res = mime.file(file)
             except Exception:  # pragma: nocover
                 # workaround for zesty python3-magic
-                debug("could not detemine mime type of '%s'" % i)
+                debug("could not detemine mime type of '%s'" % file)
                 continue
 
+            if not abs:
+                file = os.path.relpath(file, self.unpack_dir)
+
             if (
                 res in self.magic_binary_file_descriptions
-                and not i.endswith(".mo")  # check is not a message catalog
-                and i not in bin_files
+                and not file.endswith(".mo")  # check is not a message catalog
+                and file not in bin_files
             ):
-                bin_files.append(i)
+                bin_files.append(file)
 
         return bin_files
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index 5ded6d8..e9f18d4 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -413,8 +413,8 @@ class SnapReview(ReviewBase):
         # TODO: update code to directly use self.snap.xxx instead of assigning here
         self.pkg_filename = self.snap.filename
         self.unpack_dir = self.snap.unpack_dir
-        self.pkg_files = self.snap.files
-        self.pkg_bin_files = self.snap.bin_files
+        self.pkg_files = self.snap.get_files_list(abs=True)
+        self.pkg_bin_files = self.snap.get_compiled_binaries_list(abs=True)
         self.tmp_dir = self.snap.tmp_dir
 
         snap_yaml = self._extract_snap_yaml()
diff --git a/reviewtools/tests/containers/test_base_container.py b/reviewtools/tests/containers/test_base_container.py
index 976c74a..740e942 100644
--- a/reviewtools/tests/containers/test_base_container.py
+++ b/reviewtools/tests/containers/test_base_container.py
@@ -9,10 +9,17 @@ from reviewtools.tests.utils import make_snap2
 class TestBaseContainer(unittest.TestCase):
     """Tests for reviewtools.containers.base_container.BaseContainer"""
 
+    expected_files = ["meta/snap.yaml", "meta/icon.png", "nonexecstack.bin"]
+    expected_binaries = ["nonexecstack.bin"]
+
     @classmethod
     def setUpClass(cls):
         cls.tmp_dir = mkdtemp()
-        cls.fn = make_snap2(output_dir=cls.tmp_dir, yaml="Test")
+        cls.fn = make_snap2(
+            output_dir=cls.tmp_dir,
+            yaml="Test",
+            extra_files=["/bin/ls:nonexecstack.bin"],
+        )
 
     @classmethod
     def tearDownClass(cls):
@@ -50,3 +57,73 @@ class TestBaseContainer(unittest.TestCase):
             container = BaseContainer(self.fn)
             container.get_file("non/existent/file")
             self.assertTrue("Could not find" in context.exception.value)
+
+    # Test get files list
+    def test_get_files_list__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(
+                container.get_files_list(),
+                self.expected_files,
+            )
+        except ContainerException:
+            self.fail("An unexpected error occurred while getting files list")
+
+    def test_get_files_list_abs__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(
+                container.get_files_list(abs=True),
+                [
+                    os.path.join(container.unpack_dir, file)
+                    for file in self.expected_files
+                ],
+            )
+        except ContainerException:
+            self.fail("An unexpected error occurred while getting files list")
+
+    def test_files__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(
+                container.files,
+                self.expected_files,
+            )
+        except ContainerException:
+            self.fail("An unexpected error occurred while getting files list")
+
+    # Test get binaries list
+    def test_get_compiled_binaries_list__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(
+                container.get_compiled_binaries_list(), self.expected_binaries
+            )
+        except ContainerException:
+            self.fail(
+                "An unexpected error occurred while getting compiled binaries list"
+            )
+
+    def test_get_compiled_binaries_list_abs__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(
+                container.get_compiled_binaries_list(abs=True),
+                [
+                    os.path.join(container.unpack_dir, file)
+                    for file in self.expected_binaries
+                ],
+            )
+        except ContainerException:
+            self.fail(
+                "An unexpected error occurred while getting compiled binaries list"
+            )
+
+    def test_bin_files__happy(self):
+        try:
+            container = BaseContainer(self.fn)
+            self.assertCountEqual(container.bin_files, self.expected_binaries)
+        except ContainerException:
+            self.fail(
+                "An unexpected error occurred while getting compiled binaries list"
+            )