From 4624b5ecd7ef22dc3dbef916983ae4390c14c5f8 Mon Sep 17 00:00:00 2001 From: Cory Johns Date: Tue, 23 Jun 2020 09:52:26 -0400 Subject: [PATCH 1/5] 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. --- charmtools/build/builder.py | 10 +++++++- charmtools/build/tactics.py | 46 ++++++++++++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/charmtools/build/builder.py b/charmtools/build/builder.py index 25ba7112..53a9816c 100755 --- a/charmtools/build/builder.py +++ b/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,16 @@ 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="Use old per-layer wheelhouse processing") 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 +938,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: diff --git a/charmtools/build/tactics.py b/charmtools/build/tactics.py index a59397b3..fb355674 100644 --- a/charmtools/build/tactics.py +++ b/charmtools/build/tactics.py @@ -1036,11 +1036,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 +1053,27 @@ def __str__(self): def combine(self, existing): "" # suppress inherited doc self.previous = existing.previous + [existing] + existing.read() + self.read() + 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() + [''] + 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 @@ -1092,21 +1107,40 @@ 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:') + for line in self.lines: + log.debug(' ' + line.strip()) + wh_file = self.target.directory / 'wheelhouse.txt' + self.read() + wh_file.write_lines(self.lines) + self._add(wheelhouse, '-r', wh_file) 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 From 13f1a2d47108871205e7e6bc5ff72d351d47baaa Mon Sep 17 00:00:00 2001 From: Cory Johns Date: Tue, 23 Jun 2020 15:01:10 -0400 Subject: [PATCH 2/5] Improve error handling --- charmtools/build/tactics.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/charmtools/build/tactics.py b/charmtools/build/tactics.py index fb355674..fcacd2ee 100644 --- a/charmtools/build/tactics.py +++ b/charmtools/build/tactics.py @@ -1091,9 +1091,11 @@ 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) def _pip(self, *args): return self._run_in_venv('pip3', *args) @@ -1126,12 +1128,14 @@ def _process_per_layer(self, wheelhouse): def _process_combined(self, wheelhouse): log.debug('Processing wheelhouse:') + self.read() for line in self.lines: log.debug(' ' + line.strip()) - wh_file = self.target.directory / 'wheelhouse.txt' - self.read() - wh_file.write_lines(self.lines) - self._add(wheelhouse, '-r', wh_file) + 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 From 2e9a4e31135e9379f16703fccb9c70590125c299 Mon Sep 17 00:00:00 2001 From: Cory Johns Date: Tue, 23 Jun 2020 15:03:32 -0400 Subject: [PATCH 3/5] Improve help text on --wheelhouse-per-layer flag --- charmtools/build/builder.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/charmtools/build/builder.py b/charmtools/build/builder.py index 53a9816c..d76614c2 100755 --- a/charmtools/build/builder.py +++ b/charmtools/build/builder.py @@ -914,7 +914,8 @@ def main(args=None): help="Provide a wheelhouse.txt file with overrides " "for the built wheelhouse") parser.add_argument('-W', '--wheelhouse-per-layer', action="store_true", - help="Use old per-layer wheelhouse processing") + 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', From 09a2acfd3ee4ed925e0877a847ef84122a3b3884 Mon Sep 17 00:00:00 2001 From: Cory Johns Date: Tue, 23 Jun 2020 16:49:51 -0400 Subject: [PATCH 4/5] Comment out overridden lines in wheelhouse.txt This allows higher layers to override explicit requirements while still inheriting specs from lower layers. --- charmtools/build/tactics.py | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/charmtools/build/tactics.py b/charmtools/build/tactics.py index fcacd2ee..462d0097 100644 --- a/charmtools/build/tactics.py +++ b/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 @@ -1055,14 +1056,35 @@ def combine(self, existing): self.previous = existing.previous + [existing] existing.read() self.read() - self.lines = existing.lines + self.lines + 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() + [''] + self.lines = (['# ' + self.layer.url] + + src.lines(retain=False) + + ['']) else: self.lines = [] @@ -1096,6 +1118,7 @@ def _run_in_venv(self, *args): )))() if res.exit_code != 0: raise BuildError(res.output) + return res def _pip(self, *args): return self._run_in_venv('pip3', *args) From 54e93ff3325a630226983b302f66309204c1416a Mon Sep 17 00:00:00 2001 From: Cory Johns Date: Tue, 23 Jun 2020 16:56:43 -0400 Subject: [PATCH 5/5] Fix test --- tests/test_build.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_build.py b/tests/test_build.py index cfd5af8a..a59d0371 100644 --- a/tests/test_build.py +++ b/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]