commit: 15cbe87076b512b318fac1729ec94e6d6674a95a Author: Zac Medico <zmedico <AT> gentoo <DOT> org> AuthorDate: Sat Mar 27 23:15:16 2021 +0000 Commit: Zac Medico <zmedico <AT> gentoo <DOT> org> CommitDate: Mon Mar 29 04:46:41 2021 +0000 URL: https://gitweb.gentoo.org/proj/portage.git/commit/?id=15cbe870
repoman: add variable.phase check like pkgcheck VariableScopeCheck (bug 608664) The variable.phase check is inspired by pkgcheck's VariableScopeCheck, and uses essentially the same PMS data to drive the check. References: - https://projects.gentoo.org/pms/7/pms.html#x1-10900011.1 - https://pkgcore.github.io/pkgcheck/_modules/pkgcheck/checks/codingstyle.html#VariableScopeCheck - https://bugs.gentoo.org/775191 Bug: https://bugs.gentoo.org/608664 Signed-off-by: Zac Medico <zmedico <AT> gentoo.org> repoman/cnf/qa_data/qa_data.yaml | 1 + repoman/cnf/repository/qa_data.yaml | 1 + repoman/cnf/repository/repository.yaml | 1 + .../repoman/modules/linechecks/phases/__init__.py | 6 + .../lib/repoman/modules/linechecks/phases/phase.py | 132 +++++++++++++++++++-- repoman/lib/repoman/tests/simple/test_simple.py | 28 ++++- repoman/man/repoman.1 | 5 +- 7 files changed, 162 insertions(+), 12 deletions(-) diff --git a/repoman/cnf/qa_data/qa_data.yaml b/repoman/cnf/qa_data/qa_data.yaml index 29a3d6e9f..530c8c806 100644 --- a/repoman/cnf/qa_data/qa_data.yaml +++ b/repoman/cnf/qa_data/qa_data.yaml @@ -129,6 +129,7 @@ qahelp: obsolete: "The ebuild makes use of an obsolete construct" variable: invalidchar: "A variable contains an invalid character that is not part of the ASCII character set" + phase: "Variable referenced found within scope of incorrect ebuild phase as specified by PMS" readonly: "Assigning a readonly variable" usedwithhelpers: "Ebuild uses D, ROOT, BROOT, ED, EROOT or EPREFIX with helpers" virtual: diff --git a/repoman/cnf/repository/qa_data.yaml b/repoman/cnf/repository/qa_data.yaml index 3fe6b53d5..2249000c3 100644 --- a/repoman/cnf/repository/qa_data.yaml +++ b/repoman/cnf/repository/qa_data.yaml @@ -80,6 +80,7 @@ qawarnings: - usage.obsolete - upstream.workaround - uri.https + - variable.phase - virtual.suspect - wxwidgets.eclassnotused diff --git a/repoman/cnf/repository/repository.yaml b/repoman/cnf/repository/repository.yaml index ad00d18c1..dbc1decaa 100644 --- a/repoman/cnf/repository/repository.yaml +++ b/repoman/cnf/repository/repository.yaml @@ -61,6 +61,7 @@ linechecks_modules: emakeparallel srccompileeconf srcunpackpatches + pmsvariablerefphasescope portageinternal portageinternalvariableassignment quote diff --git a/repoman/lib/repoman/modules/linechecks/phases/__init__.py b/repoman/lib/repoman/modules/linechecks/phases/__init__.py index 686c675d2..e166b31a3 100644 --- a/repoman/lib/repoman/modules/linechecks/phases/__init__.py +++ b/repoman/lib/repoman/modules/linechecks/phases/__init__.py @@ -29,6 +29,12 @@ module_spec = { 'class': "SrcUnpackPatches", 'description': doc, }, + 'pmsvariablerefphasescope-check': { + 'name': "pmsvariablerefphasescope", + 'sourcefile': "phase", + 'class': "PMSVariableReference", + 'description': doc, + }, }, 'version': 1, } diff --git a/repoman/lib/repoman/modules/linechecks/phases/phase.py b/repoman/lib/repoman/modules/linechecks/phases/phase.py index 74cf4608f..433e93601 100644 --- a/repoman/lib/repoman/modules/linechecks/phases/phase.py +++ b/repoman/lib/repoman/modules/linechecks/phases/phase.py @@ -1,7 +1,19 @@ +import fnmatch import re - -from portage.eapi import eapi_has_src_prepare_and_src_configure +import types + +from portage.eapi import ( + eapi_has_broot, + eapi_has_sysroot, + eapi_has_src_prepare_and_src_configure, + eapi_exports_AA, + eapi_exports_replace_vars, + eapi_exports_ECLASSDIR, + eapi_exports_PORTDIR, + eapi_supports_prefix, + eapi_exports_merge_type, +) from repoman.modules.linechecks.base import LineCheck @@ -9,11 +21,22 @@ class PhaseCheck(LineCheck): """ basic class for function detection """ func_end_re = re.compile(r'^\}$') - phases_re = re.compile('(%s)' % '|'.join(( - 'pkg_pretend', 'pkg_setup', 'src_unpack', 'src_prepare', - 'src_configure', 'src_compile', 'src_test', 'src_install', - 'pkg_preinst', 'pkg_postinst', 'pkg_prerm', 'pkg_postrm', - 'pkg_config'))) + phase_funcs = ( + 'pkg_pretend', + 'pkg_setup', + 'src_unpack', + 'src_prepare', + 'src_configure', + 'src_compile', + 'src_test', + 'src_install', + 'pkg_preinst', + 'pkg_postinst', + 'pkg_prerm', + 'pkg_postrm', + 'pkg_config', + ) + phases_re = re.compile('(%s)' % '|'.join(phase_funcs)) in_phase = '' def check(self, num, line): @@ -69,3 +92,98 @@ class SrcUnpackPatches(PhaseCheck): if m is not None: return ("'%s'" % m.group(1)) + \ " call should be moved to src_prepare" + +# Refererences +# - https://projects.gentoo.org/pms/7/pms.html#x1-10900011.1 +# - https://pkgcore.github.io/pkgcheck/_modules/pkgcheck/checks/codingstyle.html#VariableScopeCheck +_pms_vars = ( + ("A", None, ("src_*", "pkg_nofetch")), + ("AA", eapi_exports_AA, ("src_*", "pkg_nofetch")), + ("FILESDIR", None, ("src_*",)), + ("DISTDIR", None, ("src_*",)), + ("WORKDIR", None, ("src_*",)), + ("S", None, ("src_*",)), + ("PORTDIR", eapi_exports_PORTDIR, ("src_*",)), + ("ECLASSDIR", eapi_exports_ECLASSDIR, ("src_*",)), + ("ROOT", None, ("pkg_*",)), + ("EROOT", eapi_supports_prefix, ("pkg_*",)), + ("SYSROOT", eapi_has_sysroot, ("src_*", "pkg_setup")), + ("ESYSROOT", eapi_has_sysroot, ("src_*", "pkg_setup")), + ("BROOT", eapi_has_broot, ("src_*", "pkg_setup")), + ("D", None, ("src_install", "pkg_preinst", "pkg_postint")), + ("ED", eapi_supports_prefix, ("src_install", "pkg_preinst", "pkg_postint")), + ("DESTTREE", None, ("src_install",)), + ("INSDESTTREE", None, ("src_install",)), + ("MERGE_TYPE", eapi_exports_merge_type, ("pkg_*",)), + ("REPLACING_VERSIONS", eapi_exports_replace_vars, ("pkg_*",)), + ("REPLACED_BY_VERSION", eapi_exports_replace_vars, ("pkg_prerm", "pkg_postrm")), +) + + +def _compile_phases(): + phase_vars = {} + for phase_func in PhaseCheck.phase_funcs: + for variable, eapi_filter, allowed_scopes in _pms_vars: + allowed = False + for scope in allowed_scopes: + if fnmatch.fnmatch(phase_func, scope): + allowed = True + break + + if not allowed: + phase_vars.setdefault(phase_func, []).append((variable, eapi_filter)) + + phase_info = {} + for phase_func, prohibited_vars in phase_vars.items(): + phase_func_vars = [] + for variable, eapi_filter in prohibited_vars: + phase_func_vars.append(variable) + phase_obj = phase_info[phase_func] = types.SimpleNamespace() + phase_obj.prohibited_vars = dict(prohibited_vars) + phase_obj.var_names = "(%s)" % "|".join( + variable for variable, eapi_filter in prohibited_vars + ) + phase_obj.var_reference = re.compile( + r"\$(\{|)%s(\}|\W)" % (phase_obj.var_names,) + ) + + return phase_info + + +class PMSVariableReference(PhaseCheck): + """Check phase scope for references to variables specified by PMS""" + + repoman_check_name = "variable.phase" + phase_info = _compile_phases() + + def new(self, pkg): + self._eapi = pkg.eapi + + def end(self): + self._eapi = None + + def phase_check(self, num, line): + try: + phase_info = self.phase_info[self.in_phase] + except KeyError: + return + + eapi = self._eapi + issues = [] + for m in phase_info.var_reference.finditer(line): + open_brace = m.group(1) + var_name = m.group(2) + close_brace = m.group(3) + # discard \W if matched by (\}|\W) + close_brace = close_brace if close_brace == "}" else "" + if bool(open_brace) != bool(close_brace): + continue + var_name = m.group(2) + eapi_filter = phase_info.prohibited_vars[var_name] + if eapi_filter is not None and not eapi_filter(eapi): + continue + issues.append( + "phase %s: EAPI %s: variable %s: Forbidden reference to variable specified by PMS" + % (self.in_phase, eapi, var_name) + ) + return issues diff --git a/repoman/lib/repoman/tests/simple/test_simple.py b/repoman/lib/repoman/tests/simple/test_simple.py index e64ba4f07..3a699a708 100644 --- a/repoman/lib/repoman/tests/simple/test_simple.py +++ b/repoman/lib/repoman/tests/simple/test_simple.py @@ -3,6 +3,7 @@ import collections import subprocess +import sys import time import types @@ -141,6 +142,12 @@ class SimpleRepomanTestCase(TestCase): """ % time.gmtime().tm_year + pkg_preinst_references_forbidden_var = """ +pkg_preinst() { + echo "This ${A} reference is not allowed. Neither is this $BROOT reference." +} +""" + repo_configs = { "test_repo": { "layout.conf": @@ -194,13 +201,14 @@ class SimpleRepomanTestCase(TestCase): "dev-libs/C-0": { "COPYRIGHT_HEADER" : copyright_header, "DESCRIPTION" : "Desc goes here", - "EAPI" : "4", + "EAPI" : "7", "HOMEPAGE" : "https://example.com";, "IUSE" : "flag", # must be unstable, since dev-libs/A[flag] is stable masked "KEYWORDS": "~x86", "LICENSE": "GPL-2", "RDEPEND": "flag? ( dev-libs/A[flag] )", + "MISC_CONTENT": pkg_preinst_references_forbidden_var, }, } licenses = ["GPL-2"] @@ -292,7 +300,15 @@ class SimpleRepomanTestCase(TestCase): committer_name = "Gentoo Dev" committer_email = "[email protected]" - expected_warnings = {"returncode": 0} + expected_warnings = { + "returncode": 0, + "warns": { + "variable.phase": [ + "dev-libs/C/C-0.ebuild: line 15: phase pkg_preinst: EAPI 7: variable A: Forbidden reference to variable specified by PMS", + "dev-libs/C/C-0.ebuild: line 15: phase pkg_preinst: EAPI 7: variable BROOT: Forbidden reference to variable specified by PMS", + ] + }, + } git_test = ( ("", RepomanRun(args=["manifest"])), @@ -380,7 +396,7 @@ class SimpleRepomanTestCase(TestCase): # triggered by python -Wd will be visible. stdout = subprocess.PIPE - for cwd in ("", "dev-libs", "dev-libs/A", "dev-libs/B"): + for cwd in ("", "dev-libs", "dev-libs/A", "dev-libs/B", "dev-libs/C"): abs_cwd = os.path.join(test_repo_symlink, cwd) proc = await asyncio.create_subprocess_exec( @@ -413,7 +429,11 @@ class SimpleRepomanTestCase(TestCase): await cmd.run() if cmd.result["result"] != cmd.expected and cmd.result.get("stdio"): portage.writemsg(cmd.result["stdio"]) - self.assertEqual(cmd.result["result"], cmd.expected) + try: + self.assertEqual(cmd.result["result"], cmd.expected) + except Exception: + print(cmd.result["result"], file=sys.stderr, flush=True) + raise continue proc = await asyncio.create_subprocess_exec( diff --git a/repoman/man/repoman.1 b/repoman/man/repoman.1 index 0926e806c..5dbc41560 100644 --- a/repoman/man/repoman.1 +++ b/repoman/man/repoman.1 @@ -1,4 +1,4 @@ -.TH "REPOMAN" "1" "Aug 2020" "Repoman VERSION" "Repoman" +.TH "REPOMAN" "1" "March 2021" "Repoman VERSION" "Repoman" .SH NAME repoman \- Gentoo's program to enforce a minimal level of quality assurance in packages added to the ebuild repository @@ -445,6 +445,9 @@ The ebuild makes use of an obsolete construct A variable contains an invalid character that is not part of the ASCII character set. .TP +.B variable.phase +Variable referenced found within scope of incorrect ebuild phase as specified by PMS. +.TP .B variable.readonly Assigning a readonly variable .TP
