From bd00920c6269e8468f2d67fbade7db2a08d08e31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 9 Nov 2016 10:20:55 +0100 Subject: [PATCH 1/2] compose: Search all nested variants MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The code to search for install tree and repo for image-build and live-media was only looking at top-level variants. Addons, optional or integrated layered products could not have been found. This would lead to error messages such as "There is no variant Server-optional to get repo from when building live image for Client" even though the variant exists. Various tests are updated to exercise this edge case. Signed-off-by: Lubomír Sedlář --- pungi/compose.py | 8 ++++++++ pungi/phases/image_build.py | 4 ++-- pungi/phases/live_images.py | 4 ++-- pungi/phases/livemedia_phase.py | 4 ++-- pungi/phases/ostree.py | 2 +- pungi/phases/ostree_installer.py | 2 +- tests/helpers.py | 5 +++++ tests/test_imagebuildphase.py | 7 ++++--- tests/test_liveimagesphase.py | 5 +++-- tests/test_livemediaphase.py | 7 ++++--- 10 files changed, 32 insertions(+), 16 deletions(-) diff --git a/pungi/compose.py b/pungi/compose.py index e62b9b9a..48a47a10 100644 --- a/pungi/compose.py +++ b/pungi/compose.py @@ -97,7 +97,11 @@ class Compose(kobo.log.LoggingBase): kobo.log.LoggingBase.__init__(self, logger) # TODO: check if minimal conf values are set self.conf = conf + # This is a dict mapping UID to Variant objects. It only contains top + # level variants. self.variants = {} + # This is a similar mapping, but contains even nested variants. + self.all_variants = {} self.topdir = os.path.abspath(topdir) self.skip_phases = skip_phases or [] self.just_phases = just_phases or [] @@ -215,6 +219,10 @@ class Compose(kobo.log.LoggingBase): parser = VariantsXmlParser(file_obj, tree_arches, tree_variants, logger=self._logger) self.variants = parser.parse() + self.all_variants = {} + for variant in self.get_variants(): + self.all_variants[variant.uid] = variant + # populate ci_base with variants - needed for layered-products (compose_id) ####FIXME - compose_to_composeinfo is no longer needed and has been #### removed, but I'm not entirely sure what this is needed for diff --git a/pungi/phases/image_build.py b/pungi/phases/image_build.py index d869a5ad..0f8f1604 100644 --- a/pungi/phases/image_build.py +++ b/pungi/phases/image_build.py @@ -31,7 +31,7 @@ class ImageBuildPhase(base.ImageConfigMixin, base.ConfigGuardedPhase): install_tree_from = image_conf.pop('install_tree_from', variant.uid) if '://' in install_tree_from: return install_tree_from - install_tree_source = self.compose.variants.get(install_tree_from) + install_tree_source = self.compose.all_variants.get(install_tree_from) if not install_tree_source: raise RuntimeError( 'There is no variant %s to get install tree from when building image for %s.' @@ -56,7 +56,7 @@ class ImageBuildPhase(base.ImageConfigMixin, base.ConfigGuardedPhase): extras.append(variant.uid) for extra in extras: - v = self.compose.variants.get(extra) + v = self.compose.all_variants.get(extra) if not v: raise RuntimeError( 'There is no variant %s to get repo from when building image for %s.' diff --git a/pungi/phases/live_images.py b/pungi/phases/live_images.py index e2b403df..6ae4fd1f 100644 --- a/pungi/phases/live_images.py +++ b/pungi/phases/live_images.py @@ -47,7 +47,7 @@ class LiveImagesPhase(base.ImageConfigMixin, base.ConfigGuardedPhase): def _get_extra_repos(self, arch, variant, extras): repo = [] for extra in extras: - v = self.compose.variants.get(extra) + v = self.compose.all_variants.get(extra) if not v: raise RuntimeError( 'There is no variant %s to get repo from when building live image for %s.' @@ -72,7 +72,7 @@ class LiveImagesPhase(base.ImageConfigMixin, base.ConfigGuardedPhase): symlink_isos_to = self.compose.conf.get("symlink_isos_to") commands = [] - for variant in self.compose.variants.values(): + for variant in self.compose.all_variants.values(): for arch in variant.arches + ["src"]: for data in get_arch_variant_data(self.compose.conf, self.name, arch, variant): subvariant = data.get('subvariant', variant.uid) diff --git a/pungi/phases/livemedia_phase.py b/pungi/phases/livemedia_phase.py index 35a5894c..0d98dadd 100644 --- a/pungi/phases/livemedia_phase.py +++ b/pungi/phases/livemedia_phase.py @@ -36,7 +36,7 @@ class LiveMediaPhase(ImageConfigMixin, ConfigGuardedPhase): extras.append(variant.uid) for extra in extras: - v = self.compose.variants.get(extra) + v = self.compose.all_variants.get(extra) if not v: raise RuntimeError( 'There is no variant %s to get repo from when building live media for %s.' @@ -56,7 +56,7 @@ class LiveMediaPhase(ImageConfigMixin, ConfigGuardedPhase): if 'install_tree_from' in image_conf: variant_uid = image_conf['install_tree_from'] try: - variant = self.compose.variants[variant_uid] + variant = self.compose.all_variants[variant_uid] except KeyError: raise RuntimeError( 'There is no variant %s to get repo from when building live media for %s.' diff --git a/pungi/phases/ostree.py b/pungi/phases/ostree.py index 4a52f2ff..cac2d4d9 100644 --- a/pungi/phases/ostree.py +++ b/pungi/phases/ostree.py @@ -45,7 +45,7 @@ class OSTreeThread(WorkerThread): (arch, variant.uid, self.num)) repodir = os.path.join(workdir, 'config_repo') - source_variant = compose.variants[config['source_repo_from']] + source_variant = compose.all_variants[config['source_repo_from']] source_repo = translate_path(compose, compose.paths.compose.repository('$basearch', source_variant, diff --git a/pungi/phases/ostree_installer.py b/pungi/phases/ostree_installer.py index 521a442b..bb1f9f65 100644 --- a/pungi/phases/ostree_installer.py +++ b/pungi/phases/ostree_installer.py @@ -69,7 +69,7 @@ class OstreeInstallerThread(WorkerThread): """ if '://' in source: return source.replace('$arch', arch) - source_variant = compose.variants[source] + source_variant = compose.all_variants[source] return translate_path( compose, compose.paths.compose.repository(arch, source_variant, create_dir=False)) diff --git a/tests/helpers.py b/tests/helpers.py index 7fa2d6f8..a05373f5 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -59,6 +59,11 @@ class DummyCompose(object): 'Everything': mock.Mock(uid='Everything', arches=['x86_64', 'amd64'], type='variant', is_empty=False), } + self.all_variants = self.variants.copy() + self.all_variants['Server-optional'] = mock.Mock( + uid='Server-optional', arches=['x86_64'], type='optional', is_empty=False, + parent=self.variants['Server']) + self.variants['Server'].variants = {'optional': self.all_variants['Server-optional']} self.log_info = mock.Mock() self.log_error = mock.Mock() self.log_debug = mock.Mock() diff --git a/tests/test_imagebuildphase.py b/tests/test_imagebuildphase.py index 2407cf1c..08c3ca5f 100644 --- a/tests/test_imagebuildphase.py +++ b/tests/test_imagebuildphase.py @@ -277,7 +277,7 @@ class TestImageBuildPhase(PungiTestCase): 'distro': 'Fedora-20', 'disk_size': 3, 'arches': ['x86_64'], - 'install_tree_from': 'Everything', + 'install_tree_from': 'Server-optional', } } ] @@ -302,7 +302,7 @@ class TestImageBuildPhase(PungiTestCase): "format": [('docker', 'tar.xz')], "image_conf": { 'image-build': { - 'install_tree': self.topdir + '/compose/Everything/$arch/os', + 'install_tree': self.topdir + '/compose/Server-optional/$arch/os', 'kickstart': 'fedora-docker-base.ks', 'format': 'docker', 'repo': self.topdir + '/compose/Server/$arch/os', @@ -340,7 +340,7 @@ class TestImageBuildPhase(PungiTestCase): 'distro': 'Fedora-20', 'disk_size': 3, 'arches': ['x86_64'], - 'repo_from': 'Everything', + 'repo_from': ['Everything', 'Server-optional'], } } ] @@ -369,6 +369,7 @@ class TestImageBuildPhase(PungiTestCase): 'kickstart': 'fedora-docker-base.ks', 'format': 'docker', 'repo': ','.join([self.topdir + '/compose/Everything/$arch/os', + self.topdir + '/compose/Server-optional/$arch/os', self.topdir + '/compose/Server/$arch/os']), 'variant': compose.variants['Server'], 'target': 'f24', diff --git a/tests/test_liveimagesphase.py b/tests/test_liveimagesphase.py index ac01d13b..f3bacdb7 100644 --- a/tests/test_liveimagesphase.py +++ b/tests/test_liveimagesphase.py @@ -25,7 +25,7 @@ class TestLiveImagesPhase(PungiTestCase): 'amd64': { 'kickstart': 'test.ks', 'additional_repos': ['http://example.com/repo/'], - 'repo_from': ['Everything'], + 'repo_from': ['Everything', 'Server-optional'], 'release': None, } }) @@ -49,7 +49,8 @@ class TestLiveImagesPhase(PungiTestCase): 'scratch': False, 'repos': [self.topdir + '/compose/Client/amd64/os', 'http://example.com/repo/', - self.topdir + '/compose/Everything/amd64/os'], + self.topdir + '/compose/Everything/amd64/os', + self.topdir + '/compose/Server-optional/amd64/os'], 'label': '', 'name': None, 'filename': 'image-name', diff --git a/tests/test_livemediaphase.py b/tests/test_livemediaphase.py index acca0389..92b078fe 100644 --- a/tests/test_livemediaphase.py +++ b/tests/test_livemediaphase.py @@ -326,12 +326,12 @@ class TestLiveMediaPhase(PungiTestCase): 'scratch': True, 'skip_tag': True, 'title': 'Custom Title', - 'repo_from': ['Everything'], + 'repo_from': ['Everything', 'Server-optional'], 'repo': ['http://example.com/extra_repo'], 'arches': ['x86_64'], 'ksversion': '24', 'release': None, - 'install_tree_from': 'Everything', + 'install_tree_from': 'Server-optional', 'subvariant': 'Something', 'failable': ['*'], } @@ -359,12 +359,13 @@ class TestLiveMediaPhase(PungiTestCase): 'release': '20151203.t.0', 'repo': ['http://example.com/extra_repo', self.topdir + '/compose/Everything/$basearch/os', + self.topdir + '/compose/Server-optional/$basearch/os', self.topdir + '/compose/Server/$basearch/os'], 'scratch': True, 'skip_tag': True, 'target': 'f24', 'title': 'Custom Title', - 'install_tree': self.topdir + '/compose/Everything/$basearch/os', + 'install_tree': self.topdir + '/compose/Server-optional/$basearch/os', 'version': '25', 'subvariant': 'Something', 'failable_arches': ['*'], From 13871b64fb664d375a59f606cfbacf65ae734538 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 9 Nov 2016 13:39:01 +0100 Subject: [PATCH 2/2] compose: Drop unused argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `get_variants()` method had a `recursive` argument with default value of `False. However, this argument had no effect and the method always returned all variants recursively. We can just drop the argument. All callers are updated to not supply the argument. Should any need for getting the top-level variants only arise, they can be accessed as the `variants` attribute directly on the Compose object. Signed-off-by: Lubomír Sedlář --- bin/pungi-koji | 2 +- pungi/compose.py | 4 ++-- pungi/phases/createiso.py | 2 +- pungi/phases/gather/__init__.py | 2 +- tests/helpers.py | 14 ++++++++------ tests/test_buildinstall.py | 1 + tests/test_imagebuildphase.py | 2 ++ tests/test_liveimagesphase.py | 1 + tests/test_livemediaphase.py | 1 + 9 files changed, 18 insertions(+), 11 deletions(-) diff --git a/bin/pungi-koji b/bin/pungi-koji index f77808ae..1b149225 100755 --- a/bin/pungi-koji +++ b/bin/pungi-koji @@ -366,7 +366,7 @@ def run_compose(compose, create_latest_link=True): pungi.metadata.write_tree_info(compose, arch, variant) # write .discinfo and media.repo before ISOs are created - for variant in compose.get_variants(recursive=True): + for variant in compose.get_variants(): if variant.type == "addon" or variant.is_empty: continue for arch in variant.arches + ["src"]: diff --git a/pungi/compose.py b/pungi/compose.py index 48a47a10..b3a967a2 100644 --- a/pungi/compose.py +++ b/pungi/compose.py @@ -229,13 +229,13 @@ class Compose(kobo.log.LoggingBase): #### or if it is at all self.ci_base = compose_to_composeinfo(self) - def get_variants(self, types=None, arch=None, recursive=False): + def get_variants(self, types=None, arch=None): result = [] types = types or ["variant", "optional", "addon", "layered-product"] for i in self.variants.itervalues(): if i.type in types and (not arch or arch in i.arches): result.append(i) - result.extend(i.get_variants(types=types, arch=arch, recursive=recursive)) + result.extend(i.get_variants(types=types, arch=arch)) return sorted(set(result)) def get_arches(self): diff --git a/pungi/phases/createiso.py b/pungi/phases/createiso.py index d90b45f8..0a7f0ff1 100644 --- a/pungi/phases/createiso.py +++ b/pungi/phases/createiso.py @@ -65,7 +65,7 @@ class CreateisoPhase(PhaseBase): deliverables = [] commands = [] - for variant in self.compose.get_variants(types=["variant", "layered-product", "optional"], recursive=True): + for variant in self.compose.get_variants(types=["variant", "layered-product", "optional"]): for arch in variant.arches + ["src"]: skip_iso = get_arch_variant_data(self.compose.conf, "createiso_skip", arch, variant) if skip_iso == [True]: diff --git a/pungi/phases/gather/__init__.py b/pungi/phases/gather/__init__.py index 71a7b243..4eab85db 100644 --- a/pungi/phases/gather/__init__.py +++ b/pungi/phases/gather/__init__.py @@ -307,7 +307,7 @@ def gather_wrapper(compose, package_sets, path_prefix): # write packages (package lists) for all variants for arch in compose.get_arches(): - for variant in compose.get_variants(arch=arch, recursive=True): + for variant in compose.get_variants(arch=arch): pkg_map = result[arch][variant.uid] write_packages(compose, arch, variant, pkg_map, path_prefix=path_prefix) diff --git a/tests/helpers.py b/tests/helpers.py index a05373f5..53828a7f 100644 --- a/tests/helpers.py +++ b/tests/helpers.py @@ -60,10 +60,6 @@ class DummyCompose(object): type='variant', is_empty=False), } self.all_variants = self.variants.copy() - self.all_variants['Server-optional'] = mock.Mock( - uid='Server-optional', arches=['x86_64'], type='optional', is_empty=False, - parent=self.variants['Server']) - self.variants['Server'].variants = {'optional': self.all_variants['Server-optional']} self.log_info = mock.Mock() self.log_error = mock.Mock() self.log_debug = mock.Mock() @@ -78,8 +74,14 @@ class DummyCompose(object): self.fail_deliverable = mock.Mock() self.require_deliverable = mock.Mock() - def get_variants(self, arch=None, types=None, recursive=None): - return [v for v in self.variants.values() if not arch or arch in v.arches] + def setup_optional(self): + self.all_variants['Server-optional'] = mock.Mock( + uid='Server-optional', arches=['x86_64'], type='optional', is_empty=False, + parent=self.variants['Server']) + self.variants['Server'].variants = {'optional': self.all_variants['Server-optional']} + + def get_variants(self, arch=None, types=None): + return [v for v in self.all_variants.values() if not arch or arch in v.arches] def can_fail(self, variant, arch, deliverable): failable = get_arch_variant_data(self.conf, 'failable_deliverables', arch, variant) diff --git a/tests/test_buildinstall.py b/tests/test_buildinstall.py index 15491c56..7144685e 100644 --- a/tests/test_buildinstall.py +++ b/tests/test_buildinstall.py @@ -28,6 +28,7 @@ class BuildInstallCompose(DummyCompose): type='variant', buildinstallpackages=[], is_empty=False), } + self.all_variants = self.variants.copy() class TestBuildinstallPhase(PungiTestCase): diff --git a/tests/test_imagebuildphase.py b/tests/test_imagebuildphase.py index 08c3ca5f..2fad381f 100644 --- a/tests/test_imagebuildphase.py +++ b/tests/test_imagebuildphase.py @@ -284,6 +284,7 @@ class TestImageBuildPhase(PungiTestCase): }, 'koji_profile': 'koji', }) + compose.setup_optional() self.assertEqual(validate(compose.conf), []) @@ -347,6 +348,7 @@ class TestImageBuildPhase(PungiTestCase): }, 'koji_profile': 'koji', }) + compose.setup_optional() self.assertEqual(validate(compose.conf), []) diff --git a/tests/test_liveimagesphase.py b/tests/test_liveimagesphase.py index f3bacdb7..6f383164 100644 --- a/tests/test_liveimagesphase.py +++ b/tests/test_liveimagesphase.py @@ -31,6 +31,7 @@ class TestLiveImagesPhase(PungiTestCase): }) ], }) + compose.setup_optional() self.assertEqual(validate(compose.conf), []) diff --git a/tests/test_livemediaphase.py b/tests/test_livemediaphase.py index 92b078fe..6d577de0 100644 --- a/tests/test_livemediaphase.py +++ b/tests/test_livemediaphase.py @@ -338,6 +338,7 @@ class TestLiveMediaPhase(PungiTestCase): ] } }) + compose.setup_optional() self.assertEqual(validate(compose.conf), [])