Merge lp:~mvo/click-reviewers-tools/snappy into lp:click-reviewers-tools

[Michael Vogt]

Tiny branch to allow unpacking of squashfs based snaps. This will be used by the store and its the first step toward more snappy specific support for c-r-t.

[lp:~mvo/click-reviewers-tools/snappy]

534. By Michael Vogt on 2015-10-20

address review comments from Ricardo (many thanks!)

[Michael Vogt (mvo)]

Thanks Ricardo, excellent feedback and you are absolutely right, your suggestion makes it much nicer.

[Ricardo Kirkner (ricardokirkner)]

LGTM

review: Approve

[Jamie Strandboge (jdstrand)]

If we accept this, won't that mean that the store will accept squashfs snaps and clicks? Are we ready to do this now?

review: Needs Information

[Michael Vogt (mvo)]

Thanks for the review Jamie. We want this for os and kernel snaps (that are manual review currently now anyway). This will unblock the all-snap work. Of course we need the verification of the squashfs (i.e. repacking it and compare) before this is ready for non-manual review snaps.

[James Westby (james-w)]

Jamie Strandboge <email address hidden> writes:

> Review: Needs Information
>
> If we accept this, won't that mean that the store will accept squashfs snaps and clicks? Are we ready to do this now?

We will accept them if they pass all the rest of the checks. We probably
don't want that, but if a check fails it will be held for manual review.

Is there going to be a base framework update for the new capability?

Thanks,

James

[lp:~mvo/click-reviewers-tools/snappy]

535. By Michael Vogt on 2015-10-26

clickreviews/cr_common.py: s/unsquashfs/unpacking/ thanks to Ricardo Kirkner

[Michael Vogt (mvo)]

Updated to fix typo as suggested by Ricardo.

[Jamie Strandboge (jdstrand)]

Ok, we discussed this on irc. The code in this branch is ok, but a check needs to be added to make sure that if a squashfs is used, it triggers a manual review. Once that is in place, it would be safe for the store to pull as is and then people can build on top of that in future iterations.

review: Needs Fixing

[lp:~mvo/click-reviewers-tools/snappy]

536. By Michael Vogt on 2015-10-27

add cr_lint check for squashfs

537. By Michael Vogt on 2015-10-27

pep8 fixes

[Michael Vogt (mvo)]

Thanks Jamie! I added a check to cr_lint and a test. Please let me know if that is sufficient. I ran the "tests-{pep8,pyflakes,tests}" and they all pass.

[lp:~mvo/click-reviewers-tools/snappy]

538. By Michael Vogt on 2015-10-27

merged trunk and resolved conflicts

[Daniel Holbach (dholbach)]

Looks good to me.

The only (less important) bit I was unsure about was the question if squashfs-tools should be a Recommends or a Depends. It looks like c-r-t error out just fine if the tools can't be found, but as the squashfs snaps are going to be the future of everything, we could as well make it a depends. No super strong opinion though. :-)

review: Approve

[Jamie Strandboge (jdstrand)]

Thanks for the updates. +1

review: Approve

=== modified file 'clickreviews/cr_common.py'
--- clickreviews/cr_common.py	2015-10-16 04:47:48 +0000
+++ clickreviews/cr_common.py	2015-10-27 08:51:15 +0000
@@ -639,6 +639,38 @@
     return [sp2.returncode, out]
 
 
+def _unpack_cmd(cmd_args, d, dest):
+    '''Low level unpack helper'''
+    curdir = os.getcwd()
+    os.chdir(d)
+
+    (rc, out) = cmd(cmd_args)
+    os.chdir(curdir)
+
+    if rc != 0:
+        if os.path.isdir(d):
+            recursive_rm(d)
+        error("unpacking failed with '%d':\n%s" % (rc, out))
+
+    if dest is None:
+        dest = d
+    else:
+        shutil.move(d, dest)
+
+    return dest
+
+
+def _unpack_snap_squashfs(snap_pkg, dest):
+    '''Unpack a squashfs based snap package to dest'''
+    d = tempfile.mkdtemp(prefix='clickreview-')
+    return _unpack_cmd(['unsquashfs', '-f', '-d', d, snap_pkg], d, dest)
+
+
+def _unpack_click_deb(click_pkg, dest):
+    d = tempfile.mkdtemp(prefix='clickreview-')
+    return _unpack_cmd(['dpkg-deb', '-R', click_pkg, d], d, dest)
+
+
 def unpack_click(fn, dest=None):
     '''Unpack click package'''
     if not os.path.isfile(fn):
@@ -650,24 +682,18 @@
     if dest is not None and os.path.exists(dest):
         error("'%s' exists. Aborting." % dest)
 
-    d = tempfile.mkdtemp(prefix='clickreview-')
-
-    curdir = os.getcwd()
-    os.chdir(d)
-    (rc, out) = cmd(['dpkg-deb', '-R', click_pkg, d])
-    os.chdir(curdir)
-
-    if rc != 0:
-        if os.path.isdir(d):
-            recursive_rm(d)
-        error("dpkg-deb -R failed with '%d':\n%s" % (rc, out))
-
-    if dest is None:
-        dest = d
-    else:
-        shutil.move(d, dest)
-
-    return dest
+    # check if its a squashfs based snap
+    if is_squashfs(click_pkg):
+        return _unpack_snap_squashfs(fn, dest)
+
+    return _unpack_click_deb(fn, dest)
+
+
+def is_squashfs(filename):
+    '''Return true if the given filename as a squashfs header'''
+    with open(filename, 'rb') as f:
+        header = f.read(10)
+    return header.startswith(b"hsqs")
 
 
 def raw_unpack_pkg(fn, dest=None):

=== modified file 'clickreviews/cr_lint.py'
--- clickreviews/cr_lint.py	2015-10-21 21:48:50 +0000
+++ clickreviews/cr_lint.py	2015-10-27 08:51:15 +0000
@@ -24,7 +24,13 @@
 import yaml
 
 from clickreviews.frameworks import Frameworks
-from clickreviews.cr_common import ClickReview, open_file_read, cmd, error
+from clickreviews.cr_common import (
+    ClickReview,
+    open_file_read,
+    cmd,
+    error,
+    is_squashfs,
+)
 
 CONTROL_FILE_NAMES = ["control", "manifest", "preinst"]
 MINIMUM_CLICK_FRAMEWORK_VERSION = "0.4"
@@ -1206,6 +1212,15 @@
                         break
                 self._add_result(t, n, s)
 
+    def check_is_squashfs(self):
+        '''Check snapfs'''
+        if is_squashfs(self.click_package):
+            t = 'error'
+            n = self._get_check_name('is_squashfs')
+            s = "(MANUAL REVIEW) squashfs pkg"
+            manual_review = True
+            self._add_result(t, n, s, manual_review=manual_review)
+
     def check_snappy_hashes(self):
         '''Check snappy hashes.yaml'''
         if not self.is_snap:

=== modified file 'clickreviews/tests/test_cr_lint.py'
--- clickreviews/tests/test_cr_lint.py	2015-10-16 05:27:08 +0000
+++ clickreviews/tests/test_cr_lint.py	2015-10-27 08:51:15 +0000
@@ -1864,6 +1864,17 @@
         expected_counts = {'info': None, 'warn': 0, 'error': 1}
         self.check_results(r, expected_counts)
 
+    def test_squashfs_errors(self):
+        '''Test that squashfs snaps are marked for manual review'''
+        c = ClickReviewLint(self.test_name)
+        c.unpack_dir = "/nonexistent.nonexec"
+        with patch("clickreviews.cr_lint.is_squashfs") as mock_is_squashfs:
+            mock_is_squashfs.return_value = True
+            c.check_is_squashfs()
+            r = c.click_report
+            expected_counts = {'info': None, 'warn': 0, 'error': 1}
+            self.check_results(r, expected_counts)
+
 
 class ClickReviewLintTestCase(TestCase):
     """Tests without mocks where they are not needed."""

=== modified file 'debian/control'
--- debian/control	2015-06-09 13:02:53 +0000
+++ debian/control	2015-10-27 08:51:15 +0000
@@ -32,6 +32,7 @@
          python3-yaml,
          ${misc:Depends},
          ${python3:Depends}
+Recommends: squashfs-tools
 Description: tools to review click packages
  These scripts can be used to review click packages both manually and in a
  programmatic fashion.