From d7601f7eed8af06c44d2163d30b1dc3c572e0240 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 7 Feb 2024 12:16:22 +0000 Subject: [PATCH 1/3] Incorporate some lint checks into `meson test` This will make it easier and more obvious for developers to run them locally: I'm sure I'm not the only developer who had assumed that `.gitlab-ci/` is private to the CI environment and inappropriate (or perhaps even destructive) to run on a developer/user system. The lint checks are automatically skipped (with TAP SKIP syntax) if we are not in a git checkout, or if git or the lint tool is missing. They can also be disabled explicitly with `meson test --no-suite=lint`, which downstream distributions will probably want to do. By default, most lint checks are reported as an "expected failure" (with TAP TODO syntax) rather than a hard failure, because they do not indicate a functional problem with GLib and there is a tendency for lint tools to introduce additional checks or become more strict over time. Developers can override this by configuring with `-Dwerror=true` (which also makes compiler warnings into fatal errors), or by running the test suite like `LINT_WARNINGS_ARE_ERRORS=1 meson test --suite=lint`. One exception to this is tests/check-missing-install-tag.py, which is checking a functionally significant feature of our build system, and seems like it is unlikely to have false positives: if that one fails, it is reported as a hard failure. run-style-check-diff.sh and run-check-todos.sh are not currently given this treatment, because they require search-common-ancestor.sh, which uses Gitlab-CI-specific information to find out which commits are in-scope for checking. Signed-off-by: Simon McVittie --- .gitlab-ci.yml | 12 ++-- .gitlab-ci/run-black.sh | 6 -- .gitlab-ci/run-flake8.sh | 9 --- .gitlab-ci/run-shellcheck.sh | 7 --- .gitlab-ci/run-tests.sh | 2 - .gitlab-ci/test-msvc.bat | 1 - meson.build | 5 ++ tests/black.sh | 23 +++++++ .../check-missing-install-tag.py | 26 ++++++-- tests/flake8.sh | 28 +++++++++ tests/lint-common.sh | 46 ++++++++++++++ tests/meson.build | 27 ++++++++ .gitlab-ci/run-reuse.sh => tests/reuse.sh | 61 ++++++++++++++----- tests/shellcheck.sh | 23 +++++++ 14 files changed, 225 insertions(+), 51 deletions(-) delete mode 100755 .gitlab-ci/run-black.sh delete mode 100755 .gitlab-ci/run-flake8.sh delete mode 100755 .gitlab-ci/run-shellcheck.sh create mode 100755 tests/black.sh rename {.gitlab-ci => tests}/check-missing-install-tag.py (52%) create mode 100755 tests/flake8.sh create mode 100644 tests/lint-common.sh create mode 100644 tests/meson.build rename .gitlab-ci/run-reuse.sh => tests/reuse.sh (69%) create mode 100755 tests/shellcheck.sh diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 112140412..87d93aed6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -137,9 +137,11 @@ sh-and-py-check: allow_failure: false script: - git config --global --add safe.directory "${PWD}" - - .gitlab-ci/run-shellcheck.sh - - .gitlab-ci/run-black.sh - - .gitlab-ci/run-flake8.sh + - tests/shellcheck.sh + - tests/black.sh + - tests/flake8.sh + variables: + LINT_WARNINGS_ARE_ERRORS: '1' only: changes: - "**/*.py" @@ -151,7 +153,9 @@ style-check-mandatory: stage: style-check allow_failure: false script: - - .gitlab-ci/run-reuse.sh + - tests/reuse.sh + variables: + LINT_WARNINGS_ARE_ERRORS: '1' fedora-x86_64: extends: diff --git a/.gitlab-ci/run-black.sh b/.gitlab-ci/run-black.sh deleted file mode 100755 index fdeaf1684..000000000 --- a/.gitlab-ci/run-black.sh +++ /dev/null @@ -1,6 +0,0 @@ -#!/bin/bash - -set -e - -# shellcheck disable=SC2046 -black --diff --check $(git ls-files '*.py') diff --git a/.gitlab-ci/run-flake8.sh b/.gitlab-ci/run-flake8.sh deleted file mode 100755 index 5675a01ef..000000000 --- a/.gitlab-ci/run-flake8.sh +++ /dev/null @@ -1,9 +0,0 @@ -#!/bin/bash - -set -e - -# Disable formatting warnings in flake8, as we use `black` to handle that. -formatting_warnings=E101,E111,E114,E115,E116,E117,E12,E13,E2,E3,E401,E5,E70,W1,W2,W3,W5 - -# shellcheck disable=SC2046 -flake8 --max-line-length=88 --ignore="$formatting_warnings" $(git ls-files '*.py') diff --git a/.gitlab-ci/run-shellcheck.sh b/.gitlab-ci/run-shellcheck.sh deleted file mode 100755 index abf2e5ef3..000000000 --- a/.gitlab-ci/run-shellcheck.sh +++ /dev/null @@ -1,7 +0,0 @@ -#!/bin/bash - -set -e - -# Ignoring third-party directories that we don't want to parse -# shellcheck disable=SC2046 -shellcheck $(git ls-files '*.sh' | grep -Ev "glib/libcharset|glib/dirent") diff --git a/.gitlab-ci/run-tests.sh b/.gitlab-ci/run-tests.sh index 1189493ba..629385066 100755 --- a/.gitlab-ci/run-tests.sh +++ b/.gitlab-ci/run-tests.sh @@ -2,8 +2,6 @@ set -ex -./.gitlab-ci/check-missing-install-tag.py _build - meson test -v \ -C _build \ --timeout-multiplier "${MESON_TEST_TIMEOUT_MULTIPLIER}" \ diff --git a/.gitlab-ci/test-msvc.bat b/.gitlab-ci/test-msvc.bat index 73f972a3c..09ba13c48 100644 --- a/.gitlab-ci/test-msvc.bat +++ b/.gitlab-ci/test-msvc.bat @@ -14,7 +14,6 @@ set args=%args:~1% :: FIXME: make warnings fatal pip3 install --upgrade --user meson==1.2.3 packaging==23.2 || goto :error meson setup %args% _build || goto :error -python .gitlab-ci/check-missing-install-tag.py _build || goto :error meson compile -C _build || goto :error meson test -v -C _build --timeout-multiplier %MESON_TEST_TIMEOUT_MULTIPLIER% || goto :error diff --git a/meson.build b/meson.build index c14b7f5f1..85856008c 100644 --- a/meson.build +++ b/meson.build @@ -171,6 +171,10 @@ common_test_env = [ 'MALLOC_CHECK_=2', ] +if get_option('werror') + common_test_env += 'LINT_WARNINGS_ARE_ERRORS=1' +endif + # Note: this may cause the tests output not to be printed when running in # verbose mode, see https://github.com/mesonbuild/meson/issues/11185 # Can be changed it to 'exitcode' if required during development. @@ -2545,6 +2549,7 @@ subdir('gmodule') subdir('gio') subdir('girepository') subdir('fuzzing') +subdir('tests') # xgettext is optional (on Windows for instance) if find_program('xgettext', required : get_option('nls')).found() diff --git a/tests/black.sh b/tests/black.sh new file mode 100755 index 000000000..37473bef5 --- /dev/null +++ b/tests/black.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# Copyright 2020 Frederic Martinsons +# Copyright 2020 Endless OS Foundation, LLC +# Copyright 2024 Collabora Ltd. +# SPDX-License-Identifier: LGPL-2.1-or-later + +set -eu + +if [ -z "${G_TEST_SRCDIR-}" ]; then + me="$(readlink -f "$0")" + G_TEST_SRCDIR="${me%/*}" +fi + +export TEST_NAME=black +export TEST_REQUIRES_TOOLS="black git" + +run_lint () { + # shellcheck disable=SC2046 + black --diff --check $(git ls-files '*.py') +} + +# shellcheck source=tests/lint-common.sh +. "$G_TEST_SRCDIR/lint-common.sh" diff --git a/.gitlab-ci/check-missing-install-tag.py b/tests/check-missing-install-tag.py similarity index 52% rename from .gitlab-ci/check-missing-install-tag.py rename to tests/check-missing-install-tag.py index ab2380421..71e96eb37 100755 --- a/.gitlab-ci/check-missing-install-tag.py +++ b/tests/check-missing-install-tag.py @@ -1,6 +1,6 @@ #!/usr/bin/env python3 # -# Copyright © 2022 Collabora, Ltd. +# Copyright © 2022-2024 Collabora, Ltd. # # SPDX-License-Identifier: LGPL-2.1-or-later # @@ -18,19 +18,33 @@ from pathlib import Path def main(): parser = argparse.ArgumentParser() - parser.add_argument("builddir", type=Path) + parser.add_argument("builddir", type=Path, nargs="?", default=".") args = parser.parse_args() - success = True + print("# TAP version 13") + + count = 0 + bad = 0 path = args.builddir / "meson-info" / "intro-install_plan.json" with path.open(encoding="utf-8") as f: install_plan = json.load(f) for target in install_plan.values(): for info in target.values(): + count += 1 + if not info["tag"]: - print("Missing install_tag for", info["destination"]) - success = False - return 0 if success else 1 + bad += 1 + dest = info["destination"] + print(f"not ok {bad} - Missing install_tag for {dest}") + + if bad == 0: + print(f"ok 1 - All {count} installed files have install_tag") + print("1..1") + return 0 + else: + print(f"# {bad}/{count} installed files do not have install_tag") + print(f"1..{bad}") + return 1 if __name__ == "__main__": diff --git a/tests/flake8.sh b/tests/flake8.sh new file mode 100755 index 000000000..10a0a6ffd --- /dev/null +++ b/tests/flake8.sh @@ -0,0 +1,28 @@ +#!/usr/bin/env bash +# Copyright 2020 Frederic Martinsons +# Copyright 2020 Endless OS Foundation, LLC +# Copyright 2024 Collabora Ltd. +# SPDX-License-Identifier: LGPL-2.1-or-later + +set -eu + +if [ -z "${G_TEST_SRCDIR-}" ]; then + me="$(readlink -f "$0")" + G_TEST_SRCDIR="${me%/*}" +fi + +export TEST_NAME=flake8 +export TEST_REQUIRES_TOOLS="flake8 git" + +run_lint () { + # Disable formatting warnings in flake8, as we use `black` to handle that. + local formatting_warnings=E101,E111,E114,E115,E116,E117,E12,E13,E2,E3,E401,E5,E70,W1,W2,W3,W5 + + # shellcheck disable=SC2046 + flake8 \ + --max-line-length=88 --ignore="$formatting_warnings" \ + $(git ls-files '*.py') +} + +# shellcheck source=tests/lint-common.sh +. "$G_TEST_SRCDIR/lint-common.sh" diff --git a/tests/lint-common.sh b/tests/lint-common.sh new file mode 100644 index 000000000..1d1c3d427 --- /dev/null +++ b/tests/lint-common.sh @@ -0,0 +1,46 @@ +#!/usr/bin/env bash +# Copyright 2016-2018 Simon McVittie +# Copyright 2018-2024 Collabora Ltd. +# SPDX-License-Identifier: LGPL-2.1-or-later + +set -eu + +skip_all () { + echo "1..0 # SKIP $*" + exit 0 +} + +main () { + local need_git= + local tool + + cd "$G_TEST_SRCDIR/.." + echo "TAP version 13" + + # shellcheck disable=SC2046 + for tool in ${TEST_REQUIRES_TOOLS-}; do + command -v "$tool" >/dev/null || skip_all "$tool not found" + if [ "$tool" = git ]; then + need_git=1 + fi + done + + if [ -n "${need_git-}" ] && ! test -e .git; then + skip_all "not a git checkout" + fi + + echo "1..1" + + if run_lint >&2; then + echo "ok 1" + exit 0 + elif [ -n "${LINT_WARNINGS_ARE_ERRORS-}" ]; then + echo "not ok 1 - warnings from ${TEST_NAME-"lint tool"}" + exit 1 + else + echo "not ok 1 # TO""DO warnings from ${TEST_NAME-"lint tool"}" + exit 0 + fi +} + +main diff --git a/tests/meson.build b/tests/meson.build new file mode 100644 index 000000000..ca739c458 --- /dev/null +++ b/tests/meson.build @@ -0,0 +1,27 @@ +# Copyright 2024 Collabora Ltd. +# SPDX-License-Identifier: LGPL-2.1-or-later + +lint_scripts = [ + 'black.sh', + 'flake8.sh', + 'reuse.sh', + 'shellcheck.sh', +] + +foreach test_name : lint_scripts + test( + test_name, files(test_name), + env : common_test_env, + suite : 'lint', + protocol : 'tap', + ) +endforeach + +test( + 'check-missing-install-tag.py', + python, + args : ['-B', files('check-missing-install-tag.py')], + env : common_test_env, + suite : 'lint', + protocol : 'tap', +) \ No newline at end of file diff --git a/.gitlab-ci/run-reuse.sh b/tests/reuse.sh similarity index 69% rename from .gitlab-ci/run-reuse.sh rename to tests/reuse.sh index a45b4dd3f..e9d9e0641 100755 --- a/.gitlab-ci/run-reuse.sh +++ b/tests/reuse.sh @@ -1,23 +1,43 @@ -#!/bin/bash +#!/usr/bin/env bash # # Copyright 2022 Endless OS Foundation, LLC +# Copyright 2024 Collabora Ltd. # # SPDX-License-Identifier: LGPL-2.1-or-later # # Original author: Philip Withnall -set -e +set -eu + +if [ -z "${G_TEST_SRCDIR-}" ]; then + me="$(readlink -f "$0")" + G_TEST_SRCDIR="${me%/*}" +fi + +skip_all () { + echo "1..0 # SKIP $*" + exit 0 +} + +cd "$G_TEST_SRCDIR/.." +echo "TAP version 13" + +command -v git >/dev/null || skip_all "git not found" +command -v reuse >/dev/null || skip_all "reuse not found" +test -e .git || skip_all "not a git checkout" + +echo "1..1" # We need to make sure the submodules are up to date, or `reuse lint` will fail # when it tries to run `git status` internally -git submodule update --init +git submodule update --init >&2 # Run `reuse lint` on the code base and see if the number of files without # suitable copyright/licensing information has increased from a baseline # FIXME: Eventually this script can check whether *any* files are missing # information. But for now, let’s slowly improve the baseline. -files_without_copyright_information_max=346 -files_without_license_information_max=417 +files_without_copyright_information_max=343 +files_without_license_information_max=414 # The || true is because `reuse lint` will exit with status 1 if the project is not compliant # FIXME: Once https://github.com/fsfe/reuse-tool/issues/512 or @@ -36,27 +56,36 @@ files_without_license_information="$(( total_files - files_with_license_informat if [ "${files_without_copyright_information}" -gt "${files_without_copyright_information_max}" ] || \ [ "${files_without_license_information}" -gt "${files_without_license_information_max}" ]; then - echo "${lint_output}" + echo "${lint_output}" >&2 fi if [ "${files_without_copyright_information}" -gt "${files_without_copyright_information_max}" ]; then - echo "" - echo "Error: New files added without REUSE-compliant copyright information" - echo "Please make sure that all files added in this branch/merge request have correct copyright information" + echo "" >&2 + echo "Error: New files added without REUSE-compliant copyright information" >&2 + echo "Please make sure that all files added in this branch/merge request have correct copyright information" >&2 error=1 fi if [ "${files_without_license_information}" -gt "${files_without_license_information_max}" ]; then - echo "" - echo "Error: New files added without REUSE-compliant licensing information" - echo "Please make sure that all files added in this branch/merge request have correct license information" + echo "" >&2 + echo "Error: New files added without REUSE-compliant licensing information" >&2 + echo "Please make sure that all files added in this branch/merge request have correct license information" >&2 error=1 fi if [ "${error}" -eq "1" ]; then - echo "" - echo "See https://reuse.software/tutorial/#step-2 for information on how to add REUSE information" - echo "Also see https://gitlab.gnome.org/GNOME/glib/-/issues/1415" + echo "" >&2 + echo "See https://reuse.software/tutorial/#step-2 for information on how to add REUSE information" >&2 + echo "Also see https://gitlab.gnome.org/GNOME/glib/-/issues/1415" >&2 fi -exit "${error}" \ No newline at end of file +if [ "${error}" -eq 0 ]; then + echo "ok 1" + exit 0 +elif [ -n "${LINT_WARNINGS_ARE_ERRORS-}" ]; then + echo "not ok 1 - warnings from reuse" + exit "${error}" +else + echo "not ok 1 # TO""DO warnings from reuse" + exit 0 +fi diff --git a/tests/shellcheck.sh b/tests/shellcheck.sh new file mode 100755 index 000000000..4f5ce6209 --- /dev/null +++ b/tests/shellcheck.sh @@ -0,0 +1,23 @@ +#!/usr/bin/env bash +# Copyright 2020 Frederic Martinsons +# Copyright 2024 Collabora Ltd. +# SPDX-License-Identifier: LGPL-2.1-or-later + +set -eu + +if [ -z "${G_TEST_SRCDIR-}" ]; then + me="$(readlink -f "$0")" + G_TEST_SRCDIR="${me%/*}" +fi + +export TEST_NAME=shellcheck +export TEST_REQUIRES_TOOLS="git shellcheck" + +run_lint () { + # Ignoring third-party directories that we don't want to parse + # shellcheck disable=SC2046 + shellcheck $(git ls-files '*.sh' | grep -Ev "glib/libcharset|glib/dirent") +} + +# shellcheck source=tests/lint-common.sh +. "$G_TEST_SRCDIR/lint-common.sh" From a7702505e02ed361c053f5fbfdf8694a94d04893 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 7 Feb 2024 13:46:11 +0000 Subject: [PATCH 2/3] CI: Extend submodule workaround to most jobs that run the test suite Signed-off-by: Simon McVittie --- .gitlab-ci.yml | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 87d93aed6..51c7dd1b6 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -111,8 +111,7 @@ variables: - cp -r $HOME/subprojects/* subprojects/ # FIXME: Work around https://gitlab.com/gitlab-org/gitlab/-/issues/391756 -.only-default-with-git: - extends: .only-default +.with-git: before_script: - rm -rf subprojects/gvdb - git config --global --add safe.directory "${PWD}" @@ -122,7 +121,9 @@ variables: GIT_SUBMODULE_DEPTH: 1 style-check-advisory: - extends: .only-default-with-git + extends: + - .only-default + - .with-git image: $DEBIAN_IMAGE stage: style-check allow_failure: true @@ -131,12 +132,13 @@ style-check-advisory: - .gitlab-ci/run-check-todos.sh sh-and-py-check: - extends: .only-default + extends: + - .only-default + - .with-git image: $DEBIAN_IMAGE stage: style-check allow_failure: false script: - - git config --global --add safe.directory "${PWD}" - tests/shellcheck.sh - tests/black.sh - tests/flake8.sh @@ -148,7 +150,9 @@ sh-and-py-check: - "**/*.sh" style-check-mandatory: - extends: .only-default-with-git + extends: + - .only-default + - .with-git image: $DEBIAN_IMAGE stage: style-check allow_failure: false @@ -161,6 +165,7 @@ fedora-x86_64: extends: - .build-linux - .only-default-and-merges + - .with-git image: $FEDORA_IMAGE stage: build needs: [] @@ -211,6 +216,7 @@ debian-stable-x86_64: extends: - .build-linux - .only-default + - .with-git image: $DEBIAN_IMAGE stage: build needs: [] @@ -242,6 +248,7 @@ debian-stable-x86_64: hurd-i386: extends: - .only-schedules-or-manual + - .with-git stage: build tags: - hurd @@ -278,6 +285,7 @@ muslc-alpine-x86_64: extends: - .build-linux - .only-schedules-or-manual + - .with-git image: $ALPINE_IMAGE stage: build needs: [] @@ -347,6 +355,7 @@ G_DISABLE_ASSERT: extends: - .build-linux - .only-schedules-or-manual + - .with-git image: $FEDORA_IMAGE stage: build needs: [] @@ -381,6 +390,7 @@ valgrind: extends: - .build-linux - .only-schedules-or-manual + - .with-git image: $FEDORA_IMAGE stage: analysis needs: [] From e87db7dbf2cc0b260543a83ff8bc8adead1d4320 Mon Sep 17 00:00:00 2001 From: Simon McVittie Date: Wed, 7 Feb 2024 13:00:45 +0000 Subject: [PATCH 3/3] CI: Run all style/lint checks before failing Even if we get warnings from the first lint check, we probably want to see the warnings from later lint checks too, to reduce the number of round-trips. Signed-off-by: Simon McVittie --- .gitlab-ci.yml | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 51c7dd1b6..1f60b8681 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -128,8 +128,10 @@ style-check-advisory: stage: style-check allow_failure: true script: - - .gitlab-ci/run-style-check-diff.sh - - .gitlab-ci/run-check-todos.sh + - failed= + - .gitlab-ci/run-style-check-diff.sh || failed=1 + - .gitlab-ci/run-check-todos.sh || failed=1 + - test -z "$failed" sh-and-py-check: extends: @@ -139,9 +141,11 @@ sh-and-py-check: stage: style-check allow_failure: false script: - - tests/shellcheck.sh - - tests/black.sh - - tests/flake8.sh + - failed= + - tests/shellcheck.sh || failed=1 + - tests/black.sh || failed=1 + - tests/flake8.sh || failed=1 + - test -z "$failed" variables: LINT_WARNINGS_ARE_ERRORS: '1' only: