| Message ID | 20220731193521.1217825-15-ricardo.martincoski@gmail.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | Preventing style regressions using check-package v2 | expand |
Ricardo, All, On 2022-07-31 16:35 -0300, Ricardo Martincoski spake thusly: > Add a pre-commit hook that can be used by any buildroot developer to > prevent local commits that introduce warnings that check-package would > catch. The thing is, most users will not add that hook before comitting and submitting patches. > It can also be used by maintainers. In fact, as maintainers, what we are more insterested in, is a post-applypatch hook, not a pre-commit one. However, your script does not work as a post-applypatch hook. Furthermore, maintainers already have their hooks. For example, I have been maintaining and publishing mines: https://git.buildroot.org/~ymorin/git/buildroot.git-hooks Even if the history only dates back to 2021-11-14, I had those hooks even before, and they are an aggregation of what Arnout and Peter already had before and shared with me. My hooks are a bit slow to run, as they run on all commits, even during a rebase, or when applying a series of patches (which I do with a helper around patchwork API). I am OK with the extra time it takes, but that is not necessarily the case for other maintainers. > This hook checks only the files added/changed by the commit, so it runs > fast. > It also skips the check when working in branches that do not have this > infra. > But when there is a change to check-package itself in the commit being > performed, the shole tree is checked, since new warnings in the tree can > occur. Ah, this last part is nice, indeed. But as a maintainer, when I see changes to, say, check-package, I do run it to validate it, so changes in the ignore list, or new issues, or similar, would get caught. And if check-package et al. add new checks, either the issues it reports were previously fixed (so we can run that new check unconditionally), or the offenders have been added to the ignore list (or will be in the next patch). So, I am not sure this is important. > At same time, add an option -T to utils/docker-run so docker can run > without a tty, otherwise calling it from inside the commit hook would > lead to this error: > the input device is not a TTY I already applied a better patch, that automatically detects if a tty is needed or not, see 3d8212c4b29d (utils/docker-run: allow running without a tty). In the end, I am not convinced we should have this hook in the tree... Regards, Yann E. MORIN.
diff --git a/DEVELOPERS b/DEVELOPERS index 88e0d02c69..5908ca7f33 100644 --- a/DEVELOPERS +++ b/DEVELOPERS @@ -2499,6 +2499,7 @@ F: support/testing/tests/utils/test_check_package.py F: utils/check-package F: utils/checkpackagelib/ F: utils/docker-run +F: utils/git_hooks/ N: Richard Braun <rbraun@sceen.net> F: package/curlftpfs/ diff --git a/utils/docker-run b/utils/docker-run index f42dc33d0e..bcddcd5a1b 100755 --- a/utils/docker-run +++ b/utils/docker-run @@ -1,12 +1,23 @@ #!/usr/bin/env bash set -o errexit -o pipefail + +tty="-t" +while getopts "T" OPT; do + case "${OPT}" in + T) tty=;; + *) exit 1;; + esac +done +shift $((OPTIND-1)) + DIR=$(dirname "${0}") MAIN_DIR=$(readlink -f "${DIR}/..") # shellcheck disable=SC2016 IMAGE=$(grep ^image: "${MAIN_DIR}/.gitlab-ci.yml" | \ sed -e 's,^image: ,,g' | sed -e 's,\$CI_REGISTRY,registry.gitlab.com,g') -exec docker run -it --rm \ +# shellcheck disable=SC2086 +exec docker run -i ${tty} --rm \ --user "$(id -u)":"$(id -g)" \ --mount "type=bind,src=${MAIN_DIR},dst=${MAIN_DIR}" \ --workdir "${MAIN_DIR}" \ diff --git a/utils/git_hooks/pre-commit b/utils/git_hooks/pre-commit new file mode 100644 index 0000000000..c8764c4d57 --- /dev/null +++ b/utils/git_hooks/pre-commit @@ -0,0 +1,46 @@ +#!/bin/sh +# In order to prevent commits with check-package warnings, do the following: +# cp utils/git_hooks/pre-commit .git/hooks/pre-commit +# chmod +x .git/hooks/pre-commit + +# shellcheck disable=SC1091 +. git-sh-setup +cd_to_toplevel + +full_tree=0 +if git diff --cached --name-only HEAD | grep -q '^utils/\(checkpackagelib\|check-package\)'; then + full_tree=1 +else + removed_typechanged=$(git diff --cached --name-only --diff-filter=acmr HEAD | wc -l) + if [ "$removed_typechanged" != 0 ]; then + full_tree=1 + fi +fi + +if [ $full_tree = 1 ]; then + echo "check-package will run in the full tree..." + # shellcheck disable=SC2046 disable=SC2006 + utils/docker-run -T utils/check-package -q `git ls-files` + result=$? +else + added_changed_renamed=$(git diff --cached --name-only --diff-filter=ACMR HEAD | wc -l) + if [ "$added_changed_renamed" = 0 ]; then + echo "No files in commit to be passed to check-package." + exit 0 + fi + git diff --cached --name-only --diff-filter=ACMR -z HEAD | \ + xargs -0 utils/docker-run -T utils/check-package -q + result=$? +fi + +if [ $result = 0 ]; then + echo "Commit successfully checked with check-package." + exit 0 +fi +if utils/docker-run -T true; then + echo "check-package FAILED for commit." + exit $result +else + # probably in a branch without 'utils/docker-run -T' + echo "check-package SKIPPED for commit." +fi
Add a pre-commit hook that can be used by any buildroot developer to prevent local commits that introduce warnings that check-package would catch. It can also be used by maintainers. This hook checks only the files added/changed by the commit, so it runs fast. It also skips the check when working in branches that do not have this infra. But when there is a change to check-package itself in the commit being performed, the shole tree is checked, since new warnings in the tree can occur. At same time, add an option -T to utils/docker-run so docker can run without a tty, otherwise calling it from inside the commit hook would lead to this error: the input device is not a TTY Signed-off-by: Ricardo Martincoski <ricardo.martincoski@gmail.com> --- Changes v1 -> v2: - new patch, only a RFC! Please someone do a thorough review on this patch. Last time I wrote a pre-commit hook was more than 10 years ago. :) Output from local tests: $ git checkout origin/2022.05.x $ echo >> utils/check-package $ git add -u $ git commit -m 'another branch' check-package will run in the full tree... the input device is not a TTY the input device is not a TTY check-package SKIPPED for commit. [detached HEAD aaf6ab603a] another branch 1 file changed, 1 insertion(+) $ $ git checkout make-check-package-standardize-flake8 $ echo >> utils/check-package $ git add -u $ git commit -m 'check full tree' check-package will run in the full tree... utils/check-package:0: run 'flake8' and fix the warnings check-package FAILED for commit. $ $ git checkout make-check-package-standardize-flake8 $ git rm support/libtool/buildroot-libtool-v2.4.patch $ git commit -m 'check full tree' check-package will run in the full tree... Commit successfully checked with check-package. [make-check-package-standardize-flake8 b761eccc68] check full tree 1 file changed, 89 deletions(-) delete mode 100644 support/libtool/buildroot-libtool-v2.4.patch $ # notice the entry on .checkpackageignore remains for deleted files # but it can be removed from time to time by using: # $ utils/docker-run make .checkpackageignore $ git checkout make-check-package-standardize-flake8 $ sed -e 's, , ,g' -i package/atop/atop.hash $ git add -u $ git commit -m 'only changed' package/atop/atop.hash:2: separation does not match expectation (http://nightly.buildroot.org/#adding-packages-hash) package/atop/atop.hash:5: separation does not match expectation (http://nightly.buildroot.org/#adding-packages-hash) check-package FAILED for commit. $ $ git checkout make-check-package-standardize-flake8 $ cp package/busybox/S02sysctl package/busybox/S02sysctl2 $ git add package/busybox/S02sysctl2 $ git commit -m 'only added' package/busybox/S02sysctl2:0: DAEMON variable not defined (http://nightly.buildroot.org/#adding-packages-start-script) check-package FAILED for commit. $ --- DEVELOPERS | 1 + utils/docker-run | 13 ++++++++++- utils/git_hooks/pre-commit | 46 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 59 insertions(+), 1 deletion(-) create mode 100644 utils/git_hooks/pre-commit