[gentoo-commits] proj/pkgcore/pkgcheck:master commit in: testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/, ...

[gentoo-commits] proj/pkgcore/pkgcheck:master commit in: testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/, ...

[Arthur Zamarin]

commit:     189883497abaee3708e9348df93800cc4c24fb07
Author:     Arthur Zamarin <arthurzam <AT> gentoo <DOT> org>
AuthorDate: Wed Oct  5 11:43:57 2022 +0000
Commit:     Arthur Zamarin <arthurzam <AT> gentoo <DOT> org>
CommitDate: Wed Oct  5 11:43:57 2022 +0000
URL:        https://gitweb.gentoo.org/proj/pkgcore/pkgcheck.git/commit/?id=18988349

EbuildReservedCheck: catch declaration of phase hooks

Phase hooks (`{pre,post}_${phase}`) are used by portage to run user's
hooks defined at `bashrc` file. Defining them in ebuilds might cause
unexpected behavior, so we should warn about it.

Signed-off-by: Arthur Zamarin <arthurzam <AT> gentoo.org>

 src/pkgcheck/checks/reserved.py                    | 36 ++++++++++++++++------
 .../EbuildReservedName/expected.json               | 12 +++++---
 .../EbuildReservedName/fix.patch                   | 15 +++++++--
 .../EbuildReservedName/EbuildReservedName-0.ebuild |  8 +++++
 4 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/src/pkgcheck/checks/reserved.py b/src/pkgcheck/checks/reserved.py
index 72b5300f..8448179a 100644
--- a/src/pkgcheck/checks/reserved.py
+++ b/src/pkgcheck/checks/reserved.py
@@ -1,4 +1,7 @@
 import re
+
+from pkgcore.ebuild.eapi import EAPI
+
 from .. import addons, bash, results, sources
 from . import Check
 
@@ -15,19 +18,18 @@ class _ReservedNameCheck(Check):
     variables_usage_whitelist = {"EBUILD_PHASE", "EBUILD_PHASE_FUNC"}
 
     def _check(self, used_type: str, used_names):
-        for used_name, node_start in used_names.items():
+        for used_name, (lineno, _) in used_names.items():
             if used_name in self.special_whitelist:
                 continue
             test_name = used_name.lower()
-            lineno, _ = node_start
             for reserved in self.reserved_prefixes:
                 if test_name.startswith(reserved):
-                    yield used_name, used_type, reserved, 'prefix', lineno
+                    yield used_name, used_type, reserved, 'prefix', lineno+1
             for reserved in self.reserved_substrings:
                 if reserved in test_name:
-                    yield used_name, used_type, reserved, 'substring', lineno
+                    yield used_name, used_type, reserved, 'substring', lineno+1
             if self.reserved_ebuild_regex.match(test_name):
-                yield used_name, used_type, 'ebuild', 'substring', lineno
+                yield used_name, used_type, 'ebuild', 'substring', lineno+1
 
     def _feed(self, item):
         yield from self._check('function', {
@@ -78,16 +80,15 @@ class EclassReservedCheck(_ReservedNameCheck):
 class EbuildReservedName(results.LineResult, results.Warning):
     """Ebuild uses reserved variable or function name for package manager."""
 
-    def __init__(self, used_name: str, used_type: str, reserved_word: str, reserved_type: str, **kwargs):
+    def __init__(self, used_type: str, reserved_word: str, reserved_type: str, **kwargs):
         super().__init__(**kwargs)
-        self.used_name = used_name
         self.used_type = used_type
         self.reserved_word = reserved_word
         self.reserved_type = reserved_type
 
     @property
     def desc(self):
-        return f'line {self.lineno}: {self.used_type} name "{self.used_name}" is disallowed because "{self.reserved_word}" is a reserved {self.reserved_type}'
+        return f'line {self.lineno}: {self.used_type} name "{self.line}" is disallowed because "{self.reserved_word}" is a reserved {self.reserved_type}'
 
 
 class EbuildReservedCheck(_ReservedNameCheck):
@@ -96,6 +97,21 @@ class EbuildReservedCheck(_ReservedNameCheck):
     _source = sources.EbuildParseRepoSource
     known_results = frozenset([EbuildReservedName])
 
+    def __init__(self, options, **kwargs):
+        super().__init__(options, **kwargs)
+        self.phases_hooks = {
+            eapi_name: {
+                f'{prefix}_{phase}' for phase in eapi.phases.values() for prefix in ('pre', 'post')
+            }
+            for eapi_name, eapi in EAPI.known_eapis.items()
+        }
+
     def feed(self, pkg):
-        for *args, lineno in self._feed(pkg):
-            yield EbuildReservedName(*args, lineno=lineno, line='', pkg=pkg)
+        for used_name, *args, lineno in self._feed(pkg):
+            yield EbuildReservedName(*args, lineno=lineno, line=used_name, pkg=pkg)
+
+        for node, _ in bash.func_query.captures(pkg.tree.root_node):
+            used_name = pkg.node_str(node.child_by_field_name('name'))
+            if used_name in self.phases_hooks[str(pkg.eapi)]:
+                lineno, _ = node.start_point
+                yield EbuildReservedName('function', used_name, 'phase hook', lineno=lineno+1, line=used_name, pkg=pkg)

diff --git a/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/expected.json b/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/expected.json
index 1dd73cac..9fbd6727 100644
--- a/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/expected.json
+++ b/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/expected.json
@@ -1,5 +1,7 @@
-{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "used_name": "prepare_locale", "used_type": "function", "reserved_word": "prep", "reserved_type": "prefix", "lineno": 5, "line": ""}
-{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "used_name": "DYNAMIC_DEPS", "used_type": "variable", "reserved_word": "dyn", "reserved_type": "prefix", "lineno": 6, "line": ""}
-{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "used_name": "_hook_prepare", "used_type": "variable", "reserved_word": "hook", "reserved_type": "substring", "lineno": 7, "line": ""}
-{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "used_name": "__ORIG_CC", "used_type": "variable", "reserved_word": "__", "reserved_type": "prefix", "lineno": 10, "line": ""}
-{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "used_name": "EBUILD_TEST", "used_type": "variable", "reserved_word": "ebuild", "reserved_type": "substring", "lineno": 12, "line": ""}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "prepare_locale", "lineno": 6, "used_type": "function", "reserved_word": "prep", "reserved_type": "prefix"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "DYNAMIC_DEPS", "lineno": 7, "used_type": "variable", "reserved_word": "dyn", "reserved_type": "prefix"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "_hook_prepare", "lineno": 8, "used_type": "variable", "reserved_word": "hook", "reserved_type": "substring"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "__ORIG_CC", "lineno": 11, "used_type": "variable", "reserved_word": "__", "reserved_type": "prefix"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "EBUILD_TEST", "lineno": 13, "used_type": "variable", "reserved_word": "ebuild", "reserved_type": "substring"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "post_src_unpack", "lineno": 16, "used_type": "function", "reserved_word": "post_src_unpack", "reserved_type": "phase hook"}
+{"__class__": "EbuildReservedName", "category": "EbuildReservedCheck", "package": "EbuildReservedName", "version": "0", "line": "pre_src_test", "lineno": 20, "used_type": "function", "reserved_word": "pre_src_test", "reserved_type": "phase hook"}

diff --git a/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/fix.patch b/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/fix.patch
index 146e025e..3ce2b36a 100644
--- a/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/fix.patch
+++ b/testdata/data/repos/standalone/EbuildReservedCheck/EbuildReservedName/fix.patch
@@ -1,6 +1,7 @@
+diff -Naur standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild fixed/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
 --- standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
-+++ standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
-@@ -3,12 +3,11 @@ HOMEPAGE="https://github.com/pkgcore/pkgcheck"
++++ fixed/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
+@@ -3,20 +3,19 @@ HOMEPAGE="https://github.com/pkgcore/pkgcheck"
  SLOT="0"
  LICENSE="BSD"
 
@@ -17,3 +18,13 @@
  EBUILD_SUCCESS_HOOKS="true"
 -EBUILD_TEST="1"
  REBUILD_ALL="1"
+
+-post_src_unpack() {
++my_post_src_unpack() {
+ 	echo "Larry was here"
+ }
+
+-pre_src_test() {
++my_pre_src_test() {
+ 	echo "Larry was even here"
+ }

diff --git a/testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild b/testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
index 420f4dac..a0c8e201 100644
--- a/testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
+++ b/testdata/repos/standalone/EbuildReservedCheck/EbuildReservedName/EbuildReservedName-0.ebuild
@@ -12,3 +12,11 @@ __ORIG_CC="STUB"
 EBUILD_SUCCESS_HOOKS="true"
 EBUILD_TEST="1"
 REBUILD_ALL="1"
+
+post_src_unpack() {
+	echo "Larry was here"
+}
+
+pre_src_test() {
+	echo "Larry was even here"
+}