Merge ~jslarraz/review-tools:strip-pkg-functionality into review-tools:master

[Jorge Sancho Larraz]

Strip snap related operations (unpack, extract file, list files, list binaries) from review class

Currently unpacking the snap, listing files and binaries and other operations are performed in the __init__function of the Review class. That means that those operations are repeated every time that the Review class or any of its subclasses (i.e. SnapReviewDeclaration, SnapReviewFunctional, SnapReviewLint, SnapReviewSecurity and potential future review classes) is instantiated.

This MR is intended to strip this functionality from the review class and define a self contained `container` class that handle this functionality. It will enable a followup MR where the `container` object is passed to review classes (instead of file name), thus removing the overhead caused by those duplicated operations.

Additionally pkg_filename, pkg_files and pkg_bin_files have been renamed to filename, files and bin_files as the pkg_prefix looks now redundant in the context of the `container`

[Alex Murray (alexmurray)]

LGTM - just a couple minor things to update.

review: Needs Fixing

[Jorge Sancho Larraz (jslarraz)]

Commentaries addressed in https://git.launchpad.net/~jslarraz/review-tools/commit/?id=c8dacfc49bc8f9fd3160277e6d690456ab44e710

[Alex Murray (alexmurray)]

LGTM - thanks @jslarraz!

review: Approve

diff --git a/reviewtools/common.py b/reviewtools/common.py
index 3e3ebdb..a61314a 100644
--- a/reviewtools/common.py
+++ b/reviewtools/common.py
@@ -296,8 +296,8 @@ class ReviewBase(object):
         self.review_type = name
 
 
-class Review(ReviewBase):
-    """Common review class"""
+class BaseContainer:
+    """Base container class"""
 
     magic_binary_file_descriptions = [
         "application/x-executable; charset=binary",
@@ -310,37 +310,32 @@ class Review(ReviewBase):
         "application/x-pie-executable",
     ]
 
-    pkg_files = None
-    pkg_bin_files = None
-
-    def __init__(self, fn, review_type, overrides=None):
-        ReviewBase.__init__(self, review_type, overrides)
+    files = None
+    bin_files = None
 
+    def __init__(self, fn):
         self.tmp_dir = mkdtemp(prefix=MKDTEMP_PREFIX, dir=MKDTEMP_DIR)
         if os.path.isfile(fn):
             if os.path.basename(fn).count(".") == 0:
                 error("container filename must include an extension")
-            self.pkg_filename = os.path.join(
+            self.filename = os.path.join(
                 self.tmp_dir,
                 os.path.basename(fn),
             )
-            shutil.copyfile(fn, self.pkg_filename)
+            shutil.copyfile(fn, self.filename)
         else:
             error("Could not find '%s'" % fn)
 
-        self.unpack_dir = ".".join(self.pkg_filename.split(".")[:-1])
+        self.unpack_dir = ".".join(self.filename.split(".")[:-1])
         unpack_pkg(fn, dest=self.unpack_dir)
 
-        # unpack_pkg() now only supports snap v2, so just hardcode these
-        self.pkgfmt = {"type": "snap", "version": "16.04"}
-
         # Get a list of all unpacked files
-        if self.pkg_files is None:
-            self.pkg_files = self._list_all_files()
+        if self.files is None:
+            self.files = self.get_files_list()
 
         # Setup what is needed to get a list of all unpacked compiled binaries
-        if self.pkg_bin_files is None:
-            self.pkg_bin_files = self._list_all_compiled_binaries()
+        if self.bin_files is None:
+            self.bin_files = self.get_compiled_binaries_list()
 
     def __del__(self):
         """
@@ -360,26 +355,20 @@ class Review(ReviewBase):
             error("Could not find '%s'" % rel)
         return open_file_read(fn)
 
-    def _list_all_files(self):
+    def get_files_list(self):
         """List all files included in this package."""
-        pkg_files = []
+        files = []
         for root, dirnames, filenames in os.walk(self.unpack_dir):
             for f in filenames:
-                pkg_files.append(os.path.join(root, f))
-        return pkg_files
-
-    def _check_if_message_catalog(self, fn):
-        """Check if file is a message catalog (.mo file)."""
-        if fn.endswith(".mo"):
-            return True
-        return False
+                files.append(os.path.join(root, f))
+        return files
 
-    def _list_all_compiled_binaries(self):
+    def get_compiled_binaries_list(self):
         """List all compiled binaries in this package."""
         self.mime = magic.open(magic.MAGIC_MIME)
         self.mime.load()
-        pkg_bin_files = []
-        for i in self.pkg_files:
+        bin_files = []
+        for i in self.files:
             try:
                 res = self.mime.file(i)
             except Exception:  # pragma: nocover
@@ -389,12 +378,12 @@ class Review(ReviewBase):
 
             if (
                 res in self.magic_binary_file_descriptions
-                and not self._check_if_message_catalog(i)
-                and i not in pkg_bin_files
+                and not i.endswith(".mo")  # check is not a message catalog
+                and i not in bin_files
             ):
-                pkg_bin_files.append(i)
+                bin_files.append(i)
 
-        return pkg_bin_files
+        return bin_files
 
 
 #
@@ -408,7 +397,7 @@ def error(out, exit_code=1, do_exit=True, output_type=None):
     global RESULT_TYPES
 
     if output_type is not None:
-        Review.set_report_type(None, output_type)
+        ReviewBase.set_report_type(None, output_type)
 
     try:
         if REPORT_OUTPUT == "json":
diff --git a/reviewtools/sr_common.py b/reviewtools/sr_common.py
index fbd0472..adb0a86 100644
--- a/reviewtools/sr_common.py
+++ b/reviewtools/sr_common.py
@@ -21,8 +21,9 @@ import yaml
 
 
 from reviewtools.common import (
-    Review,
+    ReviewBase,
     ReviewException,
+    BaseContainer,
     error,
     open_file_read,
     read_snapd_base_declaration,
@@ -40,7 +41,7 @@ class SnapReviewException(ReviewException):
     """This class represents SnapReview exceptions"""
 
 
-class SnapReview(Review):
+class SnapReview(ReviewBase):
     """This class represents snap reviews"""
 
     snappy_required = ["name", "version"]
@@ -414,7 +415,15 @@ class SnapReview(Review):
     def __init__(self, fn, review_type, overrides=None):
         if review_type is None:  # for using utility functions
             return
-        Review.__init__(self, fn, review_type, overrides=overrides)
+        ReviewBase.__init__(self, review_type, overrides=overrides)
+
+        self.snap = BaseContainer(fn)
+        # 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.tmp_dir = self.snap.tmp_dir
 
         snap_yaml = self._extract_snap_yaml()
         try:
diff --git a/reviewtools/sr_lint.py b/reviewtools/sr_lint.py
index 3ddaf3f..095acf7 100644
--- a/reviewtools/sr_lint.py
+++ b/reviewtools/sr_lint.py
@@ -2008,7 +2008,7 @@ class SnapReviewLint(SnapReview):
             else:
                 appnames.append("%s.%s" % (self.snap_yaml["name"], app))
 
-        fh = self._extract_file(fn)
+        fh = self.snap._extract_file(fn)
         lines = fh.readlines()
         fh.close()
 
diff --git a/reviewtools/sr_tests.py b/reviewtools/sr_tests.py
index 27b80ed..54c2a6f 100644
--- a/reviewtools/sr_tests.py
+++ b/reviewtools/sr_tests.py
@@ -22,7 +22,6 @@ import yaml
 from unittest.mock import patch
 from unittest import TestCase
 from reviewtools.common import check_results as common_check_results
-from reviewtools.common import ReviewBase
 
 # These should be set in the test cases
 TEST_SNAP_YAML = ""
@@ -43,13 +42,12 @@ def _mock_func(self):
     return
 
 
-class _Review:
-    def __init__(self, fn, review_type, overrides):
-        ReviewBase.__init__(self, review_type, overrides)
-        self.pkg_filename = fn
-        self.unpack_dir = ".".join(self.pkg_filename.split(".")[:-1])
-        self.pkg_files = []
-        self.pkg_bin_files = []
+class _BaseContainer:
+    def __init__(self, fn):
+        self.filename = fn
+        self.unpack_dir = ".".join(self.filename.split(".")[:-1])
+        self.files = []
+        self.bin_files = []
         self.tmp_dir = "/faketmpdir"
 
 
@@ -88,7 +86,9 @@ def create_patches():
     # Mock patching. Don't use decorators but instead patch in setUp() of the
     # child.
     patches = []
-    patches.append(patch("reviewtools.sr_common.Review.__init__", _Review.__init__))
+    patches.append(
+        patch("reviewtools.sr_common.BaseContainer.__init__", _BaseContainer.__init__)
+    )
     patches.append(
         patch("reviewtools.sr_common.SnapReview._extract_snap_yaml", _extract_snap_yaml)
     )