| Message ID | 20231027152941.232042-1-ross.burton@arm.com |
|---|---|
| State | Accepted, archived |
| Commit | a3a868519beab1b9cac94fefd7dbeffb09d047e9 |
| Headers | show |
| Series | [1/3] scripts/patchreview: rework patch detection | expand |
Hi Ross, On 10/27/23 17:29, Ross Burton via lists.openembedded.org wrote: > From: Ross Burton <ross.burton@arm.com> > > A previous patch[1] added the ability to allow the search pattern for > patches to be changed, so that patchreview can be used across the entire > meta-oe repository by changing the patterns. > > However, this means the caller needs to write long patterns when calling > patchreview. > > Instead, we can see if the specified directory contains a layer by > checking if conf/layer.conf exists. If it does, then search for patches > inside this directory. If it doesn't, assume that the specified > directory is a repository that contains sublayers (such as > meta-openembedded) and look through each of the directories that match > the pattern meta-*. > > This means patchreview can both scan either a single layer (eg > .../poky/meta) or a repository of sublayers (eg .../meta-openembedded). > > [1] oe-core 599046ea9302af0cf856d3fcd827f6a2be75b7e1 > > Signed-off-by: Ross Burton <ross.burton@arm.com> > --- > scripts/contrib/patchreview.py | 36 +++++++++++++++++++++++++--------- > 1 file changed, 27 insertions(+), 9 deletions(-) > > diff --git a/scripts/contrib/patchreview.py b/scripts/contrib/patchreview.py > index 43de105adc2..af66e32e02e 100755 > --- a/scripts/contrib/patchreview.py > +++ b/scripts/contrib/patchreview.py > @@ -41,7 +41,7 @@ def blame_patch(patch): > "--format=%s (%aN <%aE>)", > "--", patch)).decode("utf-8").splitlines() > > -def patchreview(path, patches): > +def patchreview(patches): > import re, os.path > > # General pattern: start of line, optional whitespace, tag with optional > @@ -56,11 +56,10 @@ def patchreview(path, patches): > > for patch in patches: > > - fullpath = os.path.join(path, patch) > result = Result() > - results[fullpath] = result > + results[patch] = result > > - content = open(fullpath, encoding='ascii', errors='ignore').read() > + content = open(patch, encoding='ascii', errors='ignore').read() > > # Find the Signed-off-by tag > match = sob_re.search(content) > @@ -198,21 +197,40 @@ def histogram(results): > for k in bars: > print("%-20s %s (%d)" % (k.capitalize() if k else "No status", bars[k], counts[k])) > > +def gather_patches(candidate): > + # candidate can either be the path to a layer directly (eg meta-intel), or a > + # repository that contains other layers (meta-arm). We can determine what by > + # looking for a conf/layer.conf file. If that file exists then it's a layer, > + # otherwise its a repository of layers and we can assume they're called > + # meta-*. > + Maybe mention that this expects a pathlib.Path object? > + if (candidate / "conf" / "layer.conf").exists(): > + print(f"{candidate} is a layer") > + scan = [candidate] > + else: > + print(f"{candidate} is not a layer, checking for sub-layers") > + scan = [d for d in candidate.iterdir() if d.is_dir() and (d.name == "meta" or d.name.startswith("meta-"))] > + print(f"Found layers {' '.join((d.name for d in scan))}") > + What about just looking for all layer.conf files recursively instead? e.g.: scan = [l.parents[1].name for l in candidate.glob('**/conf/layer.conf') This assumes a layer root directory is the parent of `conf` directory in which a layer.conf file is, which I think is a good assumption. But this does not make any assumption that the layer should be prefixed with meta- or be meta, as opposed to the current implementation. > + patches = [] > + for directory in scan: > + filenames = subprocess.check_output(("git", "-C", directory, "ls-files", "recipes-*/**/*.patch", "recipes-*/**/*.diff"), universal_newlines=True).split() FWIW, recipes- prefix is NOT enforced, it is derived from BBFILES variable in each layer.conf. (I have used layers where a typo was there, with recipe- or sometimes just recipes/). I am not saying we need to support those, but just merely raising this as a potential shortcoming. Cheers, Quentin
diff --git a/scripts/contrib/patchreview.py b/scripts/contrib/patchreview.py index 43de105adc2..af66e32e02e 100755 --- a/scripts/contrib/patchreview.py +++ b/scripts/contrib/patchreview.py @@ -41,7 +41,7 @@ def blame_patch(patch): "--format=%s (%aN <%aE>)", "--", patch)).decode("utf-8").splitlines() -def patchreview(path, patches): +def patchreview(patches): import re, os.path # General pattern: start of line, optional whitespace, tag with optional @@ -56,11 +56,10 @@ def patchreview(path, patches): for patch in patches: - fullpath = os.path.join(path, patch) result = Result() - results[fullpath] = result + results[patch] = result - content = open(fullpath, encoding='ascii', errors='ignore').read() + content = open(patch, encoding='ascii', errors='ignore').read() # Find the Signed-off-by tag match = sob_re.search(content) @@ -198,21 +197,40 @@ def histogram(results): for k in bars: print("%-20s %s (%d)" % (k.capitalize() if k else "No status", bars[k], counts[k])) +def gather_patches(candidate): + # candidate can either be the path to a layer directly (eg meta-intel), or a + # repository that contains other layers (meta-arm). We can determine what by + # looking for a conf/layer.conf file. If that file exists then it's a layer, + # otherwise its a repository of layers and we can assume they're called + # meta-*. + + if (candidate / "conf" / "layer.conf").exists(): + print(f"{candidate} is a layer") + scan = [candidate] + else: + print(f"{candidate} is not a layer, checking for sub-layers") + scan = [d for d in candidate.iterdir() if d.is_dir() and (d.name == "meta" or d.name.startswith("meta-"))] + print(f"Found layers {' '.join((d.name for d in scan))}") + + patches = [] + for directory in scan: + filenames = subprocess.check_output(("git", "-C", directory, "ls-files", "recipes-*/**/*.patch", "recipes-*/**/*.diff"), universal_newlines=True).split() + patches += [os.path.join(directory, f) for f in filenames] + return patches if __name__ == "__main__": - import argparse, subprocess, os + import argparse, subprocess, os, pathlib args = argparse.ArgumentParser(description="Patch Review Tool") args.add_argument("-b", "--blame", action="store_true", help="show blame for malformed patches") args.add_argument("-v", "--verbose", action="store_true", help="show per-patch results") args.add_argument("-g", "--histogram", action="store_true", help="show patch histogram") args.add_argument("-j", "--json", help="update JSON") - args.add_argument("-p", "--pattern", nargs=1, action="extend", default=["recipes-*/**/*.patch", "recipes-*/**/*.diff"], help="pattern to search recipes patch") - args.add_argument("directory", help="directory to scan") + args.add_argument("directory", type=pathlib.Path, metavar="DIRECTORY", help="directory to scan (layer, or repository of layers)") args = args.parse_args() - patches = subprocess.check_output(("git", "-C", args.directory, "ls-files") + tuple(args.pattern)).decode("utf-8").split() - results = patchreview(args.directory, patches) + patches = gather_patches(args.directory) + results = patchreview(patches) analyse(results, want_blame=args.blame, verbose=args.verbose) if args.json: