Process wheelhouse.txt holistically rather than per-layer (#569)

* Process wheelhouse.txt wholistically rather than per-layer

Processing the wheelhouse.txt file from each layer in isolation means
that pip can't resolve conflicts, duplicates, or dependency version
pinning which spans across layers.  This can lead to duplicate versions
of libraries being installed which violate constraints to avoid certain
upstream bugs.

This came up in both of:

* https://bugs.launchpad.net/charm-flannel/+bug/1884598
* https://bugs.launchpad.net/charm-octavia/+bug/1884164

And in both cases trying to resolve it on a per-layer or per-charm basis
results in a whack-a-mole process of catching all of the layers which
indirectly reintroduce the unpinned version of a dependency pinned in a
base layer.

* Improve error handling

* Improve help text on --wheelhouse-per-layer flag

* Comment out overridden lines in wheelhouse.txt

This allows higher layers to override explicit requirements while still
inheriting specs from lower layers.

* Fix test

Author: johnsca

charmtools/build/builder.py

@@ -148,6 +148,7 @@ def __init__(self):
         self._top_layer = None
         self.hide_metrics = os.environ.get('CHARM_HIDE_METRICS', False)
         self.wheelhouse_overrides = None
+        self.wheelhouse_per_layer = False
         self._warned_home = False
 
     @property
@@ -904,11 +905,17 @@ def main(args=None):
                         "from the interface service.")
     parser.add_argument('-n', '--name',
                         help="Build a charm of 'name' from 'charm'")
-    parser.add_argument('-r', '--report', action="store_true",
+    parser.add_argument('-r', '--report', action="store_true", default=True,
                         help="Show post-build report of changes")
+    parser.add_argument('-R', '--no-report', action="store_false",
+                        dest='report', default=True,
+                        help="Don't show post-build report of changes")
     parser.add_argument('-w', '--wheelhouse-overrides', type=path,
                         help="Provide a wheelhouse.txt file with overrides "
                              "for the built wheelhouse")
+    parser.add_argument('-W', '--wheelhouse-per-layer', action="store_true",
+                        help="Deprecated: Use original wheelhouse processing "
+                             "method (see PR juju/charm-tools#569)")
     parser.add_argument('-v', '--verbose', action='store_true', default=False,
                         help="Increase output (same as -l DEBUG)")
     parser.add_argument('--debug', action='store_true',
@@ -932,6 +939,8 @@ def main(args=None):
                                    build.layer_index)
     LayerFetcher.NO_LOCAL_LAYERS = build.no_local_layers
 
+    WheelhouseTactic.per_layer = build.wheelhouse_per_layer
+
     configLogging(build)
 
     try:

charmtools/build/tactics.py

@@ -7,6 +7,7 @@
 from inspect import getargspec
 
 from path import Path as path
+from pkg_resources import Requirement
 from ruamel import yaml
 from charmtools import utils
 from charmtools.build.errors import BuildError
@@ -1036,11 +1037,13 @@ class WheelhouseTactic(ExactMatch, Tactic):
     kind = "dynamic"
     FILENAME = 'wheelhouse.txt'
     removed = []  # has to be class level to affect all tactics during signing
+    per_layer = False
 
     def __init__(self, *args, **kwargs):
         super(WheelhouseTactic, self).__init__(*args, **kwargs)
         self.tracked = []
         self.previous = []
+        self.lines = None
         self._venv = None
         self.purge_wheels = False
 
@@ -1051,14 +1054,48 @@ def __str__(self):
     def combine(self, existing):
         ""  # suppress inherited doc
         self.previous = existing.previous + [existing]
+        existing.read()
+        self.read()
+        new_pkgs = set()
+        for line in self.lines:
+            try:
+                req = Requirement.parse(line)
+                new_pkgs.add(req.project_name)
+            except ValueError:
+                pass  # ignore comments, blank lines, etc
+        existing_lines = []
+        for line in existing.lines:
+            try:
+                req = Requirement.parse(line)
+                # new explicit reqs will override existing ones
+                if req.project_name not in new_pkgs:
+                    existing_lines.append(line)
+                else:
+                    existing_lines.append('# {}  # overridden by {}'.format(
+                        line, self.layer.url))
+            except ValueError:
+                existing_lines.append(line)  # ignore comments, blank lines, &c
+        self.lines = existing_lines + self.lines
         return self
 
+    def read(self):
+        if self.lines is None:
+            src = path(self.entity)
+            if src.exists():
+                self.lines = (['# ' + self.layer.url] +
+                              src.lines(retain=False) +
+                              [''])
+            else:
+                self.lines = []
+
     def _add(self, wheelhouse, *reqs):
         with utils.tempdir(chdir=False) as temp_dir:
             # put in a temp dir first to ensure we track all of the files
             self._pip('download', '--no-binary', ':all:', '-d', temp_dir,
                       *reqs)
+            log.debug('Copying wheels:')
             for wheel in temp_dir.files():
+                log.debug('  ' + wheel.name)
                 dest = wheelhouse / wheel.basename()
                 if dest in self.tracked:
                     return
@@ -1076,9 +1113,12 @@ def _add(self, wheelhouse, *reqs):
     def _run_in_venv(self, *args):
         assert self._venv is not None
         # have to use bash to activate the venv properly first
-        return utils.Process(('bash', '-c', ' '.join(
+        res = utils.Process(('bash', '-c', ' '.join(
             ('.', self._venv / 'bin' / 'activate', ';') + args
-        ))).exit_on_error()()
+        )))()
+        if res.exit_code != 0:
+            raise BuildError(res.output)
+        return res
 
     def _pip(self, *args):
         return self._run_in_venv('pip3', *args)
@@ -1092,21 +1132,42 @@ def __call__(self):
             utils.Process(
                 ('virtualenv', '--python', 'python3', self._venv)
             ).exit_on_error()()
+        if self.per_layer:
+            self._process_per_layer(wheelhouse)
+        else:
+            self._process_combined(wheelhouse)
+        # clean up
+        if create_venv:
+            self._venv.rmtree_p()
+            self._venv = None
+
+    def _process_per_layer(self, wheelhouse):
         # we are the top layer; process all lower layers first
         for tactic in self.previous:
             tactic()
         # process this layer
+        log.debug('Processing wheelhouse for {}'.format(self.layer.url))
         self._add(wheelhouse, '-r', self.entity)
-        # clean up
-        if create_venv:
-            self._venv.rmtree_p()
-            self._venv = None
+
+    def _process_combined(self, wheelhouse):
+        log.debug('Processing wheelhouse:')
+        self.read()
+        for line in self.lines:
+            log.debug('  ' + line.strip())
+        with utils.tempdir(chdir=False) as temp_dir:
+            wh_file = temp_dir / 'wheelhouse.txt'
+            wh_file.write_lines(self.lines)
+            self._add(wheelhouse, '-r', wh_file)
+            wh_file.move(self.target.directory / 'wheelhouse.txt')
 
     def sign(self):
         ""  # suppress inherited doc
         sigs = {}
-        for tactic in self.previous:
-            sigs.update(tactic.sign())
+        if self.per_layer:
+            for tactic in self.previous:
+                sigs.update(tactic.sign())
+        else:
+            sigs.update(super().sign())
         for d in self.tracked:
             if d in self.removed:
                 continue

tests/test_build.py

@@ -434,6 +434,7 @@ def test_wheelhouse(self, Process, mkdtemp, rmtree_p, ph, pi, pv):
         bu.hide_metrics = True
         bu.report = False
         bu.wheelhouse_overrides = self.dirname / 'wh-over.txt'
+        Process.return_value.return_value.exit_code = 0
 
         # remove the sign phase
         bu.PHASES = bu.PHASES[:-2]