From f4be5e8985ea48920cb266b18453bf2dfff47801 Mon Sep 17 00:00:00 2001 From: Sam James Date: Tue, 25 Nov 2025 21:24:22 +0000 Subject: [PATCH 1/2] eclass: add ShadowedEclassPhase for multiple-eclass defined phases Detect the basic case of an ignored eclass phase where two eclasses are inherited, both defining the same phase, and the ebuild does not define a custom implementation of that phase at all. This means one of the exported phases from the eclasses is being ignored. Ignore some eclasses with a blacklist where they are known to vary their API by EAPI or eclass variables, at least for now, because the eclass cache we have accessible here isn't keyed by EAPI or the context of the sourcing ebuild. Bug: https://bugs.gentoo.org/516014 Bug: https://bugs.gentoo.org/795006 Closes: https://github.com/pkgcore/pkgcheck/issues/377 Signed-off-by: Sam James --- src/pkgcheck/checks/eclass.py | 77 +++++++++++++++++++ .../ShadowedEclassPhase/expected.json | 1 + .../ShadowedEclassPhase/fix.patch | 12 +++ .../ShadowedEclassPhase-0.ebuild | 6 ++ 4 files changed, 96 insertions(+) create mode 100644 testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/expected.json create mode 100644 testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/fix.patch create mode 100644 testdata/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild diff --git a/src/pkgcheck/checks/eclass.py b/src/pkgcheck/checks/eclass.py index 5a6b6430e..8e4c7e27e 100644 --- a/src/pkgcheck/checks/eclass.py +++ b/src/pkgcheck/checks/eclass.py @@ -129,6 +129,28 @@ def desc(self): return f"line {self.lineno}: redundant eclass inherit {self.line!r}, provided by {self.provider!r}" +class ShadowedEclassPhase(results.VersionResult, results.Style): + """Ebuild does not define a phase when inheriting multiple eclasses + exporting that phase. + + When inheriting multiple eclasses exporting the same phase, a custom + phase must usually be defined in the ebuild to call the phase exported + from each eclass. + + Exceptions exist where the functionality isn't needed, but it should + be a deliberate choice, not an accidental omission. + """ + + def __init__(self, phase, providers, **kwargs): + super().__init__(**kwargs) + self.phase = phase + self.providers = tuple(providers) + + @property + def desc(self): + return f"missing custom phase for {self.phase!r}, provided by eclasses: {', '.join(self.providers)}" + + class EclassUsageCheck(Check): """Scan packages for various eclass-related issues.""" @@ -142,6 +164,7 @@ class EclassUsageCheck(Check): EclassUserVariableUsage, MisplacedEclassVar, ProvidedEclassInherit, + ShadowedEclassPhase, } ) required_addons = (addons.eclass.EclassAddon,) @@ -254,6 +277,59 @@ def check_provided_eclasses(self, pkg, inherits: list[tuple[list[str], int]]): for provided, (eclass, lineno) in provided_eclasses.items(): yield ProvidedEclassInherit(eclass, pkg=pkg, line=provided, lineno=lineno) + def check_exported_eclass_phase( + self, pkg: bash.ParseTree, inherits: list[tuple[list[str], int]] + ): + """Check for eclasses exporting the same phase where the ebuild does not + call such phases manually.""" + latest_eapi = EAPI.known_eapis[sorted(EAPI.known_eapis)[-1]] + # all known build phases, e.g. src_configure + known_phases = list(latest_eapi.phases_rev) + exported_phases = {phase: [] for phase in known_phases} + + # Create a dict of known phases => eclasses exporting them: + # we're interested in the cases where the RHS list has > 1 element. + for eclasses, _ in inherits: + for eclass in eclasses: + for func in self.eclass_cache[eclass].functions: + phase = func.name.removeprefix(f"{eclass}_") + if phase in known_phases: + exported_phases[phase].append(eclass) + + if not exported_phases.keys(): + return + + defined_phases = [] + for node in bash.func_query.captures(pkg.tree.root_node).get("func", ()): + func_name = pkg.node_str(node.child_by_field_name("name")) + if func_name in known_phases: + defined_phases.append(func_name) + + # XXX: Some eclasses vary their API based on the EAPI, usually to + # 'unexport' a phase. self.eclass_cache is generated once per eclass, + # not (eclass, EAPI), so it can't handle this. Ditto phases which are only + # exported if a variable is set. Blacklist such eclasses here as it + # doesn't happen often. + # + # We could maybe make this more finely-grained for phases we know + # are conditionally exported if this list is impacting coverage + # severely. + blacklisted_eclasses = ["pypi", "vala", "xdg"] + exported_phases = { + phase: set(eclass) - set(blacklisted_eclasses) + for phase, eclass in exported_phases.items() + } + + # Strip out phases we already define (even if inside of those, we don't + # actually call exported phases from all eclasses inherited). Assume that + # a custom phase in the ebuild is intentionally omitting them. + missing_custom_phases = set( + phase for phase, eclass in exported_phases.items() if len(eclass) > 1 + ) - set(defined_phases) + + for missing in missing_custom_phases: + yield ShadowedEclassPhase(missing, sorted(exported_phases[missing]), pkg=pkg) + def feed(self, pkg): if pkg.inherit: inherited: set[str] = set() @@ -283,6 +359,7 @@ def feed(self, pkg): # verify @DEPRECATED variables or functions yield from self.check_deprecated_variables(pkg, inherits) yield from self.check_deprecated_functions(pkg, inherits) + yield from self.check_exported_eclass_phase(pkg, inherits) for eclass in pkg.inherit.intersection(self.deprecated_eclasses): replacement = self.deprecated_eclasses[eclass] diff --git a/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/expected.json b/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/expected.json new file mode 100644 index 000000000..71892994d --- /dev/null +++ b/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/expected.json @@ -0,0 +1 @@ +{"__class__": "ShadowedEclassPhase", "category": "EclassUsageCheck", "package": "ShadowedEclassPhase", "version": "0", "phase": "src_prepare", "providers": ["another-src_prepare", "export-funcs-before-inherit"]} diff --git a/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/fix.patch b/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/fix.patch new file mode 100644 index 000000000..8e5f65e04 --- /dev/null +++ b/testdata/data/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/fix.patch @@ -0,0 +1,12 @@ +--- eclass/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild ++++ fixed/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild +@@ -4,3 +4,9 @@ DESCRIPTION="Ebuild" + HOMEPAGE="https://github.com/pkgcore/pkgcheck" + LICENSE="BSD" + SLOT="0" ++ ++src_prepare() { ++ default ++ another-src_prepare_src_prepare ++ export-funcs-before-inherit_src_prepare ++} diff --git a/testdata/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild b/testdata/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild new file mode 100644 index 000000000..ffdc64167 --- /dev/null +++ b/testdata/repos/eclass/EclassUsageCheck/ShadowedEclassPhase/ShadowedEclassPhase-0.ebuild @@ -0,0 +1,6 @@ +EAPI=7 +inherit another-src_prepare export-funcs-before-inherit +DESCRIPTION="Ebuild" +HOMEPAGE="https://github.com/pkgcore/pkgcheck" +LICENSE="BSD" +SLOT="0" From 10489c0dc83ccc9c396b4af8de21680cc4ac523a Mon Sep 17 00:00:00 2001 From: Sam James Date: Tue, 25 Nov 2025 22:07:16 +0000 Subject: [PATCH 2/2] eclass: avoid a level of indentation in EclassUsageCheck Signed-off-by: Sam James --- src/pkgcheck/checks/eclass.py | 72 ++++++++++++++++++----------------- 1 file changed, 37 insertions(+), 35 deletions(-) diff --git a/src/pkgcheck/checks/eclass.py b/src/pkgcheck/checks/eclass.py index 8e4c7e27e..1e1b70e55 100644 --- a/src/pkgcheck/checks/eclass.py +++ b/src/pkgcheck/checks/eclass.py @@ -331,41 +331,43 @@ def check_exported_eclass_phase( yield ShadowedEclassPhase(missing, sorted(exported_phases[missing]), pkg=pkg) def feed(self, pkg): - if pkg.inherit: - inherited: set[str] = set() - inherits: list[tuple[list[str], int]] = [] - for node in bash.cmd_query.captures(pkg.tree.root_node).get("call", ()): - name = pkg.node_str(node.child_by_field_name("name")) - if name == "inherit": - call = pkg.node_str(node) - # filter out line continuations and conditional inherits - if eclasses := [x for x in call.split()[1:] if x in pkg.inherit]: - lineno, _colno = node.start_point - if not inherited and eclasses[0] == pkg.inherit[0]: - inherits.append((eclasses, lineno)) - - for eclass in eclasses: - if eclass not in inherited: - inherited.add(eclass) - else: - yield DuplicateEclassInherit( - eclass, line=call, lineno=lineno + 1, pkg=pkg - ) - - yield from self.check_provided_eclasses(pkg, inherits) - yield from self.check_user_variables(pkg, inherits) - # verify @PRE_INHERIT variable placement - yield from self.check_pre_inherits(pkg, inherits) - # verify @DEPRECATED variables or functions - yield from self.check_deprecated_variables(pkg, inherits) - yield from self.check_deprecated_functions(pkg, inherits) - yield from self.check_exported_eclass_phase(pkg, inherits) - - for eclass in pkg.inherit.intersection(self.deprecated_eclasses): - replacement = self.deprecated_eclasses[eclass] - if not isinstance(replacement, str): - replacement = None - yield DeprecatedEclass(eclass, replacement, pkg=pkg) + if not pkg.inherit: + return + + inherited: set[str] = set() + inherits: list[tuple[list[str], int]] = [] + for node in bash.cmd_query.captures(pkg.tree.root_node).get("call", ()): + name = pkg.node_str(node.child_by_field_name("name")) + if name == "inherit": + call = pkg.node_str(node) + # filter out line continuations and conditional inherits + if eclasses := [x for x in call.split()[1:] if x in pkg.inherit]: + lineno, _colno = node.start_point + if not inherited and eclasses[0] == pkg.inherit[0]: + inherits.append((eclasses, lineno)) + + for eclass in eclasses: + if eclass not in inherited: + inherited.add(eclass) + else: + yield DuplicateEclassInherit( + eclass, line=call, lineno=lineno + 1, pkg=pkg + ) + + yield from self.check_provided_eclasses(pkg, inherits) + yield from self.check_user_variables(pkg, inherits) + # verify @PRE_INHERIT variable placement + yield from self.check_pre_inherits(pkg, inherits) + # verify @DEPRECATED variables or functions + yield from self.check_deprecated_variables(pkg, inherits) + yield from self.check_deprecated_functions(pkg, inherits) + yield from self.check_exported_eclass_phase(pkg, inherits) + + for eclass in pkg.inherit.intersection(self.deprecated_eclasses): + replacement = self.deprecated_eclasses[eclass] + if not isinstance(replacement, str): + replacement = None + yield DeprecatedEclass(eclass, replacement, pkg=pkg) class EclassVariableScope(VariableScope, results.EclassResult):