dist/tools/ci or tests: `EXCLUDE` not assigned correctly?

PeterKietzmann commented 3 years ago

Description

Multiple static tests use changed_files.sh to exclude files from being tested, for example, codespell. If I understand it correctly, the exclude list is generated from a local EXCLUDE variable and the one in changed_files.

Is it on purpose to have two locations of exclusion?
The expansion of EXCLUDE does not work properly.

Steps to reproduce the issue

Edit a test script to print the list of files to be tested here by adding: echo "${FILES}"
Execute the script and grep for a file/folder that should be excluded, for example dist/tools/coccinelle/include: dist/tools/codespell/check.sh | grep dist/tools/coccinelle/include

Expected results

The list should not include files from dist/tools/coccinelle/include since it is excluded.

Actual results

The printed list contains dist/tools/coccinelle/include/riot-standard.h.

Versions

``` Operating System Environment ---------------------------- Operating System: "Ubuntu" "18.04.5 LTS (Bionic Beaver)" Kernel: Linux 4.15.0-126-generic x86_64 x86_64 System shell: /bin/dash (probably dash) make's shell: /bin/dash (probably dash) Installed compiler toolchains ----------------------------- native gcc: gcc (Ubuntu 7.5.0-3ubuntu1~18.04) 7.5.0 arm-none-eabi-gcc: arm-none-eabi-gcc (GNU Tools for Arm Embedded Processors 7-2018-q3-update) 7.3.1 20180622 (release) [ARM/embedded-7-branch revision 261907] avr-gcc: missing mips-mti-elf-gcc: missing msp430-elf-gcc: missing riscv-none-elf-gcc: missing riscv64-unknown-elf-gcc: missing riscv-none-embed-gcc: missing xtensa-esp32-elf-gcc: missing xtensa-esp8266-elf-gcc: missing clang: clang version 6.0.0-1ubuntu2 (tags/RELEASE_600/final) Installed compiler libs ----------------------- arm-none-eabi-newlib: "3.0.0" mips-mti-elf-newlib: missing msp430-elf-newlib: missing riscv-none-elf-newlib: missing riscv64-unknown-elf-newlib: missing riscv-none-embed-newlib: missing xtensa-esp32-elf-newlib: missing xtensa-esp8266-elf-newlib: missing avr-libc: missing (missing) Installed development tools --------------------------- ccache: ccache version 3.4.1 cmake: cmake version 3.10.2 cppcheck: Cppcheck 1.82 doxygen: 1.8.13 git: git version 2.17.1 make: GNU Make 4.1 openocd: Open On-Chip Debugger 0.10.0 python: Python 2.7.17 python2: Python 2.7.17 python3: Python 3.6.9 flake8: 3.6.0 (mccabe: 0.6.1, pycodestyle: 2.4.0, pyflakes: 2.0.0) CPython 3.6.9 on Linux coccinelle: missing ```

miri64 commented 3 years ago

Can't reproduce :-/. When on the current version of dist/enh/doccheck-annotate I get

```console $ git diff diff --git a/dist/tools/ci/static_tests.sh b/dist/tools/ci/static_tests.sh index cfaccd69e4..66ffc9b10e 100755 --- a/dist/tools/ci/static_tests.sh +++ b/dist/tools/ci/static_tests.sh @@ -69,7 +69,7 @@ run ./dist/tools/commit-msg/check.sh "${BASE_BRANCH}" run ./dist/tools/whitespacecheck/check.sh "${BASE_BRANCH}" DIFFFILTER="MR" ERROR_EXIT_CODE=0 run ./dist/tools/licenses/check.sh DIFFFILTER="AC" run ./dist/tools/licenses/check.sh -run ./dist/tools/doccheck/check.sh +# run ./dist/tools/doccheck/check.sh run ./dist/tools/externc/check.sh run ./dist/tools/cppcheck/check.sh run ./dist/tools/vera++/check.sh @@ -77,7 +77,7 @@ run ./dist/tools/pr_check/pr_check.sh "${BASE_BRANCH}" run ./dist/tools/coccinelle/check.sh run ./dist/tools/flake8/check.sh run ./dist/tools/headerguards/check.sh -run ./dist/tools/buildsystem_sanity_check/check.sh +# run ./dist/tools/buildsystem_sanity_check/check.sh run ./dist/tools/codespell/check.sh run ./dist/tools/uncrustify/uncrustify.sh --check diff --git a/dist/tools/codespell/check.sh b/dist/tools/codespell/check.sh index aeb088f90e..3b16f351ed 100755 --- a/dist/tools/codespell/check.sh +++ b/dist/tools/codespell/check.sh @@ -26,6 +26,8 @@ FILEREGEX='\.([CcHh]|[ch]pp|sh|py|md|txt)$' EXCLUDE='^(.+/vendor/)' FILES=$(FILEREGEX=${FILEREGEX} EXCLUDE=${EXCLUDE} changed_files) +echo "${FILES}" + if [ -z "${FILES}" ]; then exit 0 fi $ git diff --stat master .github/workflows/release-test.yml | 116 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------- dist/tools/ci/static_tests.sh | 4 ++-- dist/tools/codespell/check.sh | 2 ++ dist/tools/doccheck/check.sh | 54 +++++++++++++++++++++++++++++++++++++------------- sys/include/net/gnrc/ipv6.h | 6 +++--- tests/driver_aip31068/main.c | 92 ++++++++++++++++++++++++++++++++++++++++++------------------------------------------- 6 files changed, 182 insertions(+), 92 deletions(-) $ BASE_BRANCH=master dist/tools/codespell/check.sh dist/tools/ci/static_tests.sh dist/tools/codespell/check.sh dist/tools/doccheck/check.sh sys/include/net/gnrc/ipv6.h tests/driver_aip31068/main.c $ BASE_BRANCH=master dist/tools/ci/static_tests.sh Running "./dist/tools/commit-msg/check.sh master" ✓ Command output: Warning: Commit message is longer than 50 (but < 72) characters: "dist/tools/doccheck: annotate errors in Github Action" Running "./dist/tools/whitespacecheck/check.sh master" ✓ Running "./dist/tools/licenses/check.sh" ✓ Running "./dist/tools/licenses/check.sh" ✓ Running "./dist/tools/externc/check.sh" ✓ Running "./dist/tools/cppcheck/check.sh" x Command output: tests/driver_aip31068/main.c:366: style (shadowFunction): Local variable 'progress' shadows outer function Running "./dist/tools/vera++/check.sh" x Command output: ./dist/tools/vera++/check.sh: line 31: vera++: command not found ./dist/tools/vera++/check.sh: line 35: vera++: command not found Running "./dist/tools/pr_check/pr_check.sh master" x Command output: Pull request needs squashing: 74fd6fd983 REMOVE ME! Running "./dist/tools/coccinelle/check.sh" ✓ Command output: ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found ./dist/tools/coccinelle/check.sh: line 32: spatch: command not found Running "./dist/tools/flake8/check.sh" ✓ Running "./dist/tools/headerguards/check.sh" ✓ Running "./dist/tools/codespell/check.sh" ✓ Command output: dist/tools/ci/static_tests.sh dist/tools/codespell/check.sh dist/tools/doccheck/check.sh sys/include/net/gnrc/ipv6.h tests/driver_aip31068/main.c Running "./dist/tools/uncrustify/uncrustify.sh --check" ✓ Command output: All files are uncrustified! ```

No unexpected file in there

Is it on purpose to have two locations of exclusion?

I guess so you can check the vendor files for other static tests, but not for spelling errors.

PeterKietzmann commented 3 years ago

@miri64 thanks for testing. To 1.: Yes, I agree there might be reasons to do it in this way. At least in the case that not all static tests apply changed_files unconditionally (didn't check). Fine with me. To 2.: I wasn't precise enough in my test description above and adjusted it accordingly. The file that I was referring to does not actually have issues to be detected by codespell, however, it should be excluded since changed_files is applied to the exclude list.

Would you mind to have an other look? Eventually, I simply misunderstand the code...

miri64 commented 3 years ago

My last comment was incorrect. I deleted it.

miri64 commented 3 years ago

Ok, so contrary to me you did not set the BASE_BRANCH. Will try that out.

miri64 commented 3 years ago

Ah! Due to the internal EXCLUDE is overwritten by the provided EXCLUDE (that is what the : ${EXCLUDE:=...} construct is doing), dist/tools/coccinelle/include is of course not in the EXCLUDE list. Question is, was that intentional @aabadie? I could imagine you also want to check the spelling in the files that usually are excluded.

PeterKietzmann commented 3 years ago

In the end, I simply want to skip the static tests for one single file (in #15671). The same situation applies to vendor headers, I guess, but couldn't find a common blacklist. Manually adding the file to every test script seemed off...

miri64 commented 3 years ago

Manually adding the file to every test script seemed off...

Only the ones that change EXCLUDE, right? And then there is the question why you want to exclude it in the first place? pkg/mbedtls/include/riot_config.h seems very RIOT-specific to me, so I guess it is not an external import. If it is: since you are in a pkg, can't you import it via the pkg mechanism as well?

PeterKietzmann commented 3 years ago

Only the ones that change EXCLUDE, right?

Yes, but I think this is not an option. For the other question, I'd rather answer it in the other PR.

miri64 commented 3 years ago

Only the ones that change EXCLUDE, right?

Yes, but I think this is not an option. For the other question, I'd rather answer it in the other PR.

Yeah, I already commented on the file in question. See https://github.com/RIOT-OS/RIOT/pull/15671#discussion_r545320922

aabadie commented 3 years ago

was that intentional @aabadie?

Could be but not sure. I would bet on a misunderstanding of the changed_files script.

This pattern seems to be used in several checks:

$ git grep EXCLUDE= dist/
dist/tools/codespell/check.sh:EXCLUDE='^(.+/vendor/)'
dist/tools/codespell/check.sh:FILES=$(FILEREGEX=${FILEREGEX} EXCLUDE=${EXCLUDE} changed_files)
dist/tools/flake8/check.sh:EXCLUDE="^(.+/vendor/\
dist/tools/flake8/check.sh:FILES=$(FILEREGEX=${FILEREGEX} EXCLUDE=${EXCLUDE} changed_files)
dist/tools/vera++/check.sh:EXCLUDE='^(.+/vendor/)'
dist/tools/vera++/check.sh:FILES=$(FILEREGEX=${FILEREGEX} EXCLUDE=${EXCLUDE} changed_files)

And it's just to exclude the vendor directory, which is already excluded in changed_files.sh. So I guess this could be removed (except for flake8 which also excludes other paths with Python code). Will open a PR.

RIOT-OS / RIOT