Re: [PATCH 6/8] tools: make sure that test groups are described in the documentation

Amir Goldstein
2021-09-03T03:38:53Z

> diff --git a/include/buildgrouplist b/include/buildgrouplist
> index d898efa3..489de965 100644
> --- a/include/buildgrouplist
> +++ b/include/buildgrouplist
> @@ -6,3 +6,4 @@
> group.list:
> @echo " [GROUP] $$PWD/$@"
> $(Q)$(TOPDIR)/tools/mkgroupfile $@
> + $(Q)$(TOPDIR)/tools/check-groups $(TOPDIR)/doc/group-names.txt $@

I would like to argue against checking groups post mkgroupfile
and for checking groups during mkgroupfile

> diff --git a/tools/check-groups b/tools/check-groups
> new file mode 100755
> index 00000000..0d193615
> --- /dev/null
> +++ b/tools/check-groups
> @@ -0,0 +1,35 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2021 Oracle. All Rights Reserved.
> +#
> +# Make sure that all groups listed in a group.list file are mentioned in the
> +# group description file.
> +
> +if [ -z "$1" ] || [ "$1" = "--help" ]; then
> + echo "Usage: $0 path_to_group_names [group.list files...]"
> + exit 1
> +fi
> +
> +groups_doc_file="$1"
> +shift
> +
> +get_group_list() {
> + for file in "$@"; do
> + while read testname groups; do
> + test -z "${testname}" && continue
> + test "${testname:0:1}" = "#" && continue
> +
> + echo "${groups}" | tr ' ' '\n'
> + done < "${file}"
> + done | sort | uniq
> +}
> +
> +ret=0
> +while read group; do
> + if ! grep -q "^${group}[[:space:]]" "${groups_doc_file}"; then
> + echo "${group}: group not mentioned in documentation." 1>&2

This message would have been more informative with the offending
test file.

Now after you crunched all the test files into group.list files and
all the group.list files into a unique group set, this is too late.
But this same check during generate_groupfile() would have
been trivial and would allow reporting the offending test.

While we are on the subject of generate_groupfile(), can you please
explain the rationale behind the method of extracting the test file
groups by executing the test with GENERATE_GROUPS=yes?
As opposed to just getting the list of groups on the stop from the file
using grep?

Thanks,
Amir.