From b80efbfd97f9669cdb4b0582aad1329f678bbf17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lubom=C3=ADr=20Sedl=C3=A1=C5=99?= Date: Wed, 27 Feb 2019 14:30:01 +0100 Subject: [PATCH] hybrid: Refactor handling debuginfo packages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavior before this change: debuginfo was added based on source RPMs. Pungi would get all debuginfo for all architectures that include at least one binary package. This had a consequence that if foo-x.x86_64 and foo-x.i686 from the same built were included, foo-y-debuginfo.i686 would be included despite foo-y only being present for x86_64. The patch changes this to work on binary package level: for each included package `x`, check if there is `x-debuginfo` or `x-debugsource`, and include them. Packages added in this way have to go through one iteration of the solver, to account for cases such as `x-libs-debuginfo` depending on `x-debuginfo` (with only `x` starting this chain). To make it slightly faster, the invocation of fus is changed to only process newly added package in each iteration. It should have no effect on the results, and subsequent iterations are much faster if they don't process the same stuff again and again. JIRA: COMPOSE-3247 Signed-off-by: Lubomír Sedlář --- pungi/phases/gather/methods/method_hybrid.py | 125 ++++++--- tests/test_gather_method_hybrid.py | 270 +++++++++++-------- 2 files changed, 248 insertions(+), 147 deletions(-) diff --git a/pungi/phases/gather/methods/method_hybrid.py b/pungi/phases/gather/methods/method_hybrid.py index fa38b8fe..a7b48396 100644 --- a/pungi/phases/gather/methods/method_hybrid.py +++ b/pungi/phases/gather/methods/method_hybrid.py @@ -79,6 +79,12 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): self.added_langpacks = set() # Set of NEVRAs of modular packages self.modular_packages = set() + # Arch -> pkg name -> set of pkg object + self.debuginfo = defaultdict(lambda: defaultdict(set)) + + # caches for processed packages + self.processed_multilib = set() + self.processed_debuginfo = set() def _get_pkg_map(self, arch): """Create a mapping from NEVRA to actual package object. This will be @@ -111,6 +117,20 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): self._prepare_packages() return self.packages[nevra] + def _prepare_debuginfo(self): + """Prepare cache of debuginfo packages for easy access. The cache is + indexed by package architecture and then by package name. There can be + more than one debuginfo package with the same name. + """ + for pkg_arch in self.package_sets[self.arch].rpms_by_arch: + for pkg in self.package_sets[self.arch].rpms_by_arch[pkg_arch]: + self.debuginfo[pkg.arch][pkg.name].add(pkg) + + def _get_debuginfo(self, name, arch): + if not self.debuginfo: + self._prepare_debuginfo() + return self.debuginfo.get(arch, {}).get(name, set()) + def expand_list(self, patterns): """Given a list of globs, create a list of package names matching any of the pattern. @@ -206,6 +226,8 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): def run_solver(self, variant, arch, packages, platform, filter_packages): repos = [self.compose.paths.work.arch_repo(arch=arch)] + results = set() + result_modules = set() modules = [] if variant.arch_mmds.get(arch): @@ -219,8 +241,6 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): step = 0 - old_multilib = set() - while True: step += 1 conf_file = self.compose.paths.work.fus_conf(arch, variant, step) @@ -245,31 +265,45 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): run(cmd, logfile=logfile, show_cmd=True, env=env) output, out_modules = fus.parse_output(logfile) self.compose.log_debug("[DONE ] Running fus") - new_multilib = self.add_multilib(variant, arch, output, old_multilib) - old_multilib = new_multilib - if new_multilib: - input_packages.extend( - _fmt_pkg(pkg_name, pkg_arch) - for pkg_name, pkg_arch in sorted(new_multilib) - ) - continue + # No need to resolve modules again. They are not going to change. + modules = [] + # Reset input packages as well to only solve newly added things. + input_packages = [] + # Preserve the results from this iteration. + results.update(output) + result_modules.update(out_modules) + + new_multilib = self.add_multilib(variant, arch, output) + input_packages.extend( + _fmt_pkg(pkg_name, pkg_arch) + for pkg_name, pkg_arch in sorted(new_multilib) + ) + + new_debuginfo = self.add_debuginfo(arch, output) + input_packages.extend( + _fmt_pkg(pkg_name, pkg_arch) + for pkg_name, pkg_arch in sorted(new_debuginfo) + ) new_langpacks = self.add_langpacks(output) - if new_langpacks: - input_packages.extend(new_langpacks) - continue + input_packages.extend(new_langpacks) - # Nothing new was added, we can stop now. - break + if not input_packages: + # Nothing new was added, we can stop now. + break - return output, out_modules + return results, result_modules - def add_multilib(self, variant, arch, nvrs, old_multilib): + def add_multilib(self, variant, arch, nvrs): added = set() if not self.multilib_methods: return [] for nvr, pkg_arch, flags in nvrs: + if (nvr, pkg_arch) in self.processed_multilib: + continue + self.processed_multilib.add((nvr, pkg_arch)) + if "modular" in flags: continue @@ -289,7 +323,34 @@ class GatherMethodHybrid(pungi.phases.gather.method.GatherMethodBase): if self.multilib.is_multilib(multilib_candidate): added.add((nevr["name"], add_arch)) - return added - old_multilib + return added + + def add_debuginfo(self, arch, nvrs): + added = set() + + for nvr, pkg_arch, flags in nvrs: + if (nvr, pkg_arch) in self.processed_debuginfo: + continue + self.processed_debuginfo.add((nvr, pkg_arch)) + + if "modular" in flags: + continue + + pkg = self._get_package("%s.%s" % (nvr, pkg_arch)) + + # There are two ways how the debuginfo package can be named. We + # want to get them all. + for pattern in ["%s-debuginfo", "%s-debugsource"]: + debuginfo_name = pattern % pkg.name + debuginfo = self._get_debuginfo(debuginfo_name, pkg_arch) + for dbg in debuginfo: + # For each debuginfo package that matches on name and + # architecture, we also need to check if it comes from the + # same build. + if dbg.sourcerpm == pkg.rpm_sourcerpm: + added.add((dbg.name, dbg.arch)) + + return added def add_langpacks(self, nvrs): if not self.langpacks: @@ -477,8 +538,8 @@ def _make_result(paths): def expand_packages(nevra_to_pkg, variant_modules, lookasides, nvrs, filter_packages): - """For each package add source RPM and possibly also debuginfo.""" - # This will server as the final result. We collect sets of paths to the + """For each package add source RPM.""" + # This will serve as the final result. We collect sets of paths to the # packages. rpms = set() srpms = set() @@ -500,12 +561,8 @@ def expand_packages(nevra_to_pkg, variant_modules, lookasides, nvrs, filter_pack # Strip file:// prefix lookaside_packages.add(url[7:]) - # This is used to figure out which debuginfo packages to include. We keep - # track of package architectures from each SRPM. - srpm_arches = defaultdict(set) - - for nvr, arch, flags in nvrs: - pkg = nevra_to_pkg["%s.%s" % (nvr, arch)] + for nvr, pkg_arch, flags in nvrs: + pkg = nevra_to_pkg["%s.%s" % (nvr, pkg_arch)] if pkg.file_path in lookaside_packages: # Package is in lookaside, don't add it and ignore sources and # debuginfo too. @@ -518,11 +575,6 @@ def expand_packages(nevra_to_pkg, variant_modules, lookasides, nvrs, filter_pack try: srpm_nevra = _get_srpm_nevra(pkg) srpm = nevra_to_pkg[srpm_nevra] - if "modular" not in flags: - # Only mark the arch for sources of non-modular packages. The - # debuginfo is explicitly listed in the output, and we don't - # want anything more. - srpm_arches[srpm_nevra].add(arch) if (srpm.name, "src") in filters: # Filtered package, skipping continue @@ -532,17 +584,6 @@ def expand_packages(nevra_to_pkg, variant_modules, lookasides, nvrs, filter_pack # Didn't find source RPM.. this should be logged pass - # Get all debuginfo packages from all included sources. We iterate over all - # available packages and if we see a debug package from included SRPM built - # for architecture that has at least one binary package, we include it too. - for pkg in nevra_to_pkg.values(): - if pkg_is_debug(pkg) and pkg.arch in srpm_arches[_get_srpm_nevra(pkg)]: - if set([(pkg.name, pkg.arch), (pkg.name, None)]) & filters: - # Filtered package, skipping - continue - if pkg.file_path not in lookaside_packages: - debuginfo.add(pkg.file_path) - return _mk_pkg_map(_make_result(rpms), _make_result(srpms), _make_result(debuginfo)) diff --git a/tests/test_gather_method_hybrid.py b/tests/test_gather_method_hybrid.py index 458b0570..d973e99f 100644 --- a/tests/test_gather_method_hybrid.py +++ b/tests/test_gather_method_hybrid.py @@ -87,7 +87,6 @@ class TestMethodHybrid(helpers.PungiTestCase): eg.call_args_list, [mock.call(compose, arch, variant, ["standard"], set_pkg_arch=False)], ) - print(CW.mock_calls) self.assertEqual( CW.mock_calls, [ @@ -328,6 +327,7 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): self.compose = helpers.DummyCompose(self.topdir, {}) self.phase = hybrid.GatherMethodHybrid(self.compose) self.phase.multilib_methods = [] + self.phase.arch = "x86_64" self.logfile1 = os.path.join( self.compose.topdir, "logs/x86_64/hybrid-depsolver-Server-iter-1.x86_64.log" ) @@ -359,7 +359,7 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): peek_context=mock.Mock(return_value="ctx"), ) ] - po.return_value = (mock.Mock(), mock.Mock()) + po.return_value = ([], ["m1"]) res = self.phase.run_solver( self.compose.variants["Server"], @@ -369,7 +369,8 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): filter_packages=[("foo", None)], ) - self.assertEqual(res, po.return_value) + self.assertItemsEqual(res[0], []) + self.assertItemsEqual(res[1], ["m1"]) self.assertEqual(po.call_args_list, [mock.call(self.logfile1)]) self.assertEqual( run.call_args_list, @@ -412,7 +413,9 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): peek_context=mock.Mock(return_value="ctx"), ), } - po.return_value = (mock.Mock(), mock.Mock()) + po.return_value = ([("p-1-1", "x86_64", frozenset())], ["m1"]) + self.phase.packages = {"p-1-1.x86_64": mock.Mock()} + self.phase.package_sets = {"x86_64": mock.Mock(rpms_by_arch={"x86_64": []})} res = self.phase.run_solver( self.compose.variants["Server"], @@ -422,7 +425,7 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): filter_packages=["foo"], ) - self.assertEqual(res, po.return_value) + self.assertEqual(res, (set([("p-1-1", "x86_64", frozenset())]), set(["m1"]))) self.assertEqual(po.call_args_list, [mock.call(self.logfile1)]) self.assertEqual( run.call_args_list, @@ -451,7 +454,9 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): ) def test_with_comps(self, run, gc, po, wc): - po.return_value = (mock.Mock(), mock.Mock()) + self.phase.packages = {"pkg-1.0-1.x86_64": mock.Mock()} + self.phase.debuginfo = {"x86_64": {}} + po.return_value = ([("pkg-1.0-1", "x86_64", frozenset())], []) res = self.phase.run_solver( self.compose.variants["Server"], "x86_64", @@ -460,7 +465,8 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): filter_packages=[], ) - self.assertEqual(res, po.return_value) + self.assertItemsEqual(res[0], po.return_value[0]) + self.assertItemsEqual(res[1], []) self.assertEqual(po.call_args_list, [mock.call(self.logfile1)]) self.assertEqual( run.call_args_list, @@ -487,20 +493,41 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): ], ) - def test_with_langpacks(self, run, gc, po, wc): - self.phase.langpacks = {"pkg": set(["pkg-en"])} - final = [("pkg-1.0-1", "x86_64", []), ("pkg-en-1.0-1", "noarch", [])] - po.side_effect = [([("pkg-1.0-1", "x86_64", [])], set()), (final, [])] - + def test_with_comps_with_debuginfo(self, run, gc, po, wc): + dbg1 = NamedMock(name="pkg-debuginfo", arch="x86_64", sourcerpm="pkg.src.rpm") + dbg2 = NamedMock(name="pkg-debuginfo", arch="x86_64", sourcerpm="x.src.rpm") + self.phase.packages = { + "pkg-1.0-1.x86_64": NamedMock( + name="pkg", arch="x86_64", rpm_sourcerpm="pkg.src.rpm" + ), + "pkg-debuginfo-1.0-1.x86_64": dbg1, + "pkg-debuginfo-1.0-2.x86_64": dbg2, + } + self.phase.debuginfo = { + "x86_64": { + "pkg-debuginfo": [dbg1, dbg2], + }, + } + po.side_effect = [ + ([("pkg-1.0-1", "x86_64", frozenset())], []), + ([("pkg-debuginfo-1.0-1", "x86_64", frozenset())], []), + ] res = self.phase.run_solver( self.compose.variants["Server"], "x86_64", [("pkg", None)], platform=None, - filter_packages=["foo"], + filter_packages=[], ) - self.assertEqual(res, (final, [])) + self.assertItemsEqual( + res[0], + [ + ("pkg-1.0-1", "x86_64", frozenset()), + ("pkg-debuginfo-1.0-1", "x86_64", frozenset()), + ], + ) + self.assertItemsEqual(res[1], []) self.assertEqual( po.call_args_list, [mock.call(self.logfile1), mock.call(self.logfile2)] ) @@ -519,7 +546,73 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): wc.call_args_list, [ mock.call(self.config1, [], ["pkg"]), - mock.call(self.config2, [], ["pkg", "pkg-en"]), + mock.call(self.config2, [], ["pkg-debuginfo.x86_64"]), + ], + ) + self.assertEqual( + gc.call_args_list, + [ + mock.call( + self.config1, + "x86_64", + [self._repo("repo")], + [], + platform=None, + filter_packages=[], + ), + mock.call( + self.config2, + "x86_64", + [self._repo("repo")], + [], + platform=None, + filter_packages=[], + ), + ], + ) + + def test_with_langpacks(self, run, gc, po, wc): + self.phase.langpacks = {"pkg": set(["pkg-en"])} + final = [ + ("pkg-1.0-1", "x86_64", frozenset()), + ("pkg-en-1.0-1", "noarch", frozenset()), + ] + po.side_effect = [([("pkg-1.0-1", "x86_64", frozenset())], []), (final, [])] + self.phase.packages = { + "pkg-1.0-1.x86_64": mock.Mock(), + "pkg-en-1.0-1.noarch": mock.Mock(), + } + self.phase.package_sets = {"x86_64": mock.Mock(rpms_by_arch={"x86_64": []})} + + res = self.phase.run_solver( + self.compose.variants["Server"], + "x86_64", + [("pkg", None)], + platform=None, + filter_packages=["foo"], + ) + + self.assertItemsEqual(res[0], final) + self.assertItemsEqual(res[1], []) + self.assertEqual( + po.call_args_list, [mock.call(self.logfile1), mock.call(self.logfile2)] + ) + self.assertEqual( + run.call_args_list, + [ + mock.call( + gc.return_value, logfile=self.logfile1, show_cmd=True, env=mock.ANY + ), + mock.call( + gc.return_value, logfile=self.logfile2, show_cmd=True, env=mock.ANY + ), + ], + ) + self.assertEqual( + wc.call_args_list, + [ + mock.call(self.config1, [], ["pkg"]), + mock.call(self.config2, [], ["pkg-en"]), ], ) self.assertEqual( @@ -562,14 +655,20 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): } } self.phase.packages = self.phase.package_maps["x86_64"] - final = [ - ("pkg-devel-1.0-1", "x86_64", []), - ("foo-1.0-1", "x86_64", []), - ("pkg-devel-1.0-1", "i686", []), - ] + self.phase.debuginfo = {"x86_64": {}} po.side_effect = [ - ([("pkg-devel-1.0-1", "x86_64", []), ("foo-1.0-1", "x86_64", [])], []), - (final, []), + ( + [ + ("pkg-devel-1.0-1", "x86_64", frozenset()), + ("foo-1.0-1", "x86_64", frozenset()) + ], + frozenset()), + ( + [ + ("pkg-devel-1.0-1", "i686", frozenset()), + ], + [], + ), ] res = self.phase.run_solver( @@ -580,7 +679,15 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): filter_packages=[], ) - self.assertEqual(res, (final, [])) + self.assertItemsEqual( + res[0], + [ + ("pkg-devel-1.0-1", "x86_64", frozenset()), + ("foo-1.0-1", "x86_64", frozenset()), + ("pkg-devel-1.0-1", "i686", frozenset()), + ] + ) + self.assertItemsEqual(res[1], []) self.assertEqual( po.call_args_list, [mock.call(self.logfile1), mock.call(self.logfile2)] ) @@ -599,7 +706,7 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): wc.call_args_list, [ mock.call(self.config1, [], ["foo", "pkg-devel"]), - mock.call(self.config2, [], ["foo", "pkg-devel", "pkg-devel.i686"]), + mock.call(self.config2, [], ["pkg-devel.i686"]), ], ) self.assertEqual( @@ -668,14 +775,21 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): "foo-1.0-1.i686": mock.Mock(), } } - final = [ - ("pkg-devel-1.0-1", "x86_64", []), - ("foo-1.0-1", "x86_64", []), - ("foo-1.0-1", "i686", []), - ] + self.phase.debuginfo = {"x86_64": {}} po.side_effect = [ - ([("pkg-devel-1.0-1", "x86_64", []), ("foo-1.0-1", "x86_64", [])], []), - (final, []), + ( + [ + ("pkg-devel-1.0-1", "x86_64", frozenset()), + ("foo-1.0-1", "x86_64", frozenset()) + ], + [], + ), + ( + [ + ("foo-1.0-1", "i686", frozenset()), + ], + [], + ), ] res = self.phase.run_solver( @@ -686,7 +800,15 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): filter_packages=[], ) - self.assertEqual(res, (final, [])) + self.assertItemsEqual( + res[0], + [ + ("pkg-devel-1.0-1", "x86_64", frozenset()), + ("foo-1.0-1", "x86_64", frozenset()), + ("foo-1.0-1", "i686", frozenset()), + ], + ) + self.assertItemsEqual(res[1], []) self.assertEqual( po.call_args_list, [mock.call(self.logfile1), mock.call(self.logfile2)] ) @@ -705,7 +827,7 @@ class TestRunSolver(HelperMixin, helpers.PungiTestCase): wc.call_args_list, [ mock.call(self.config1, [], ["foo", "pkg-devel"]), - mock.call(self.config2, [], ["foo", "foo.i686", "pkg-devel"]), + mock.call(self.config2, [], ["foo.i686"]), ], ) self.assertEqual( @@ -771,8 +893,8 @@ class TestExpandPackages(helpers.PungiTestCase): }, ) - def test_include_src_and_debuginfo(self): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") + def test_include_src(self): + nevra_to_pkg = self._mk_packages(src=True) res = hybrid.expand_packages( nevra_to_pkg, {}, [], [("pkg-3:1-2", "x86_64", [])], [] @@ -783,19 +905,19 @@ class TestExpandPackages(helpers.PungiTestCase): { "rpm": [{"path": "/tmp/pkg.rpm", "flags": []}], "srpm": [{"path": "/tmp/pkg.src.rpm", "flags": []}], - "debuginfo": [{"path": "/tmp/pkg-debuginfo.x86_64.rpm", "flags": []}], + "debuginfo": [], }, ) - def test_filter_src_and_debuginfo(self): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") + def test_filter_src(self): + nevra_to_pkg = self._mk_packages(src=True) res = hybrid.expand_packages( nevra_to_pkg, {}, [], [("pkg-3:1-2", "x86_64", [])], - filter_packages=[("pkg-debuginfo", "x86_64"), ("pkg", "src")], + filter_packages=[("pkg", "src")], ) self.assertEqual( @@ -807,48 +929,8 @@ class TestExpandPackages(helpers.PungiTestCase): }, ) - def test_filter_debuginfo_missing_arch(self): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") - - res = hybrid.expand_packages( - nevra_to_pkg, - {}, - [], - [("pkg-3:1-2", "x86_64", [])], - filter_packages=[("pkg-debuginfo", None)], - ) - - self.assertEqual( - res, - { - "rpm": [{"path": "/tmp/pkg.rpm", "flags": []}], - "srpm": [{"path": "/tmp/pkg.src.rpm", "flags": []}], - "debuginfo": [], - }, - ) - - def test_filter_debuginfo_different_arch(self): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") - - res = hybrid.expand_packages( - nevra_to_pkg, - {}, - [], - [("pkg-3:1-2", "x86_64", [])], - filter_packages=[("pkg-debuginfo", "aarch64")], - ) - - self.assertEqual( - res, - { - "rpm": [{"path": "/tmp/pkg.rpm", "flags": []}], - "srpm": [{"path": "/tmp/pkg.src.rpm", "flags": []}], - "debuginfo": [{"path": "/tmp/pkg-debuginfo.x86_64.rpm", "flags": []}], - }, - ) - - def test_modular_include_src_but_not_debuginfo(self): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") + def test_modular_include_src(self): + nevra_to_pkg = self._mk_packages(src=True) res = hybrid.expand_packages( nevra_to_pkg, {}, [], [("pkg-3:1-2", "x86_64", ["modular"])], [] @@ -879,25 +961,9 @@ class TestExpandPackages(helpers.PungiTestCase): }, ) - def test_skip_debuginfo_for_different_arch(self): - nevra_to_pkg = self._mk_packages(debug_arch="i686") - - res = hybrid.expand_packages( - nevra_to_pkg, {}, [], [("pkg-3:1-2", "x86_64", [])], [] - ) - - self.assertEqual( - res, - { - "rpm": [{"path": "/tmp/pkg.rpm", "flags": []}], - "srpm": [{"path": "/tmp/pkg.src.rpm", "flags": []}], - "debuginfo": [], - }, - ) - @mock.patch("pungi.phases.gather.methods.method_hybrid.cr") - def test_skip_lookaside_source_and_debuginfo(self, cr): - nevra_to_pkg = self._mk_packages(debug_arch="x86_64") + def test_skip_lookaside_source(self, cr): + nevra_to_pkg = self._mk_packages(src=True) lookasides = [mock.Mock()] repo = { "abc": NamedMock( @@ -906,12 +972,6 @@ class TestExpandPackages(helpers.PungiTestCase): location_base="file:///tmp/", location_href="pkg.src.rpm", ), - "def": NamedMock( - name="pkg-debuginfo", - arch="x86_64", - location_base="file:///tmp/", - location_href="pkg-debuginfo.x86_64.rpm", - ), } cr.Metadata.return_value.keys.return_value = repo.keys() cr.Metadata.return_value.get.side_effect = lambda key: repo[key]