Skip to content
This repository has been archived by the owner on Oct 15, 2024. It is now read-only.

checkbashism prints "strange results" #3834

Closed
markus2330 opened this issue May 10, 2021 · 10 comments
Closed

checkbashism prints "strange results" #3834

markus2330 opened this issue May 10, 2021 · 10 comments
Labels
1p one point continuous integration floss2022w lang/shell stale testing errors in tests without implication on real usage

Comments

@markus2330
Copy link
Contributor

markus2330 commented May 10, 2021

from https://build.libelektra.org/blue/organizations/jenkins/libelektra/detail/PR-3830/6/pipeline/397

160/263 Test #171: testscr_check_bashisms ........................***Failed    9.45 sec
script scripts/api_review/generate_review_files.py does not appear to be a /bin/sh script; skipping
could not find any possible bashisms in bash script scripts/check-third-party-license-file-paths
script scripts/dev/.shfmtignore does not appear to have a #! interpreter line;
you may get strange results
script scripts/keynames.py does not appear to be a /bin/sh script; skipping
script scripts/packaging/debian/changelog does not appear to have a #! interpreter line;
you may get strange results
error: scripts/packaging/debian/changelog: Unterminated quoted string found, EOF reached. Wanted: <'>, opened in line 272
script scripts/packaging/debian/doc-base/elektra-doc does not appear to have a #! interpreter line;
you may get strange results
script scripts/packaging/fedora/changelog does not appear to have a #! interpreter line;
you may get strange results
fatal (no cleanup necessary): Possible bashisms detected, please check.

Please fix all the problems by excluding the files in ./tests/shell/check_bashisms.sh

@mpranj did you already fix (part) of this problem?

@markus2330 markus2330 added 3p three points cm2022s for university course continuous integration 1p one point and removed 3p three points labels May 10, 2021
@mpranj
Copy link
Member

mpranj commented May 10, 2021

I did not touch the checkbashisms scripts yet. I left a comment in the PR, in this specific case the fault is in the PR and not in the check_bashisms script.

Regarding the other files: yes we should probably exclude them to avoid strange results. 😄

@flo91
Copy link
Collaborator

flo91 commented Mar 31, 2022

[CM T0]
Is this issue still relevant?
The current master builds run successfully through all CI-steps as can be seen here.

On the other hand the mentioned file /tests/shell/check_bashisms.sh has the last commit on 11.11.2019.

Am I missing something or did the problem get fixed some other way?

@flo91 flo91 changed the title checkbashism [CM T0] checkbashism Mar 31, 2022
@kodebach
Copy link
Member

Is this issue still relevant?

Hard to say, but I'd say the root issue is still relevant. AFAICT we run checkbashisms against all files in scripts except for the ones that have been explicitly excluded. This is probably not a good idea, since there are lots of files in scripts that are not shell scripts.

Am I missing something or did the problem get fixed some other way?

I think the issue doesn't occur right now, because checkbashisms doesn't find any problems in our non-shell script files.


I think the goal here should be to add some general filters to tests/shell/check_bashisms.sh even before the explicit exclusions. You'd have to come up with those filters, but a good start would probably be:

  1. always check files that end in .sh
  2. don't check files that don't have shebang (#!)

@markus2330
Copy link
Contributor Author

I still get:

150: script scripts/api_review/generate_review_files.py does not appear to be a /bin/sh script; skipping
150: script scripts/dev/.devcontainer/devcontainer.json does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/dev/.shfmtignore does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/flatpak/org.libelektra.kdb.appdata.xml does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/flatpak/org.libelektra.kdb.desktop does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/flatpak/org.libelektra.kdb.svg does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/flatpak/org.libelektra.kdb.yaml does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/keynames.py does not appear to be a /bin/sh script; skipping
150: script scripts/packaging/debian/changelog does not appear to have a #! interpreter line;
150: you may get strange results
150: error: scripts/packaging/debian/changelog: Unterminated quoted string found, EOF reached. Wanted: <'>, opened in line 314
150: script scripts/packaging/debian/doc-base/elektra-doc does not appear to have a #! interpreter line;
150: you may get strange results
150: script scripts/packaging/fedora/changelog does not appear to have a #! interpreter line;
150: you may get strange results
150: testscr_check_bashisms.sh RESULTS: 2 test(s) done 0 error(s).
1/1 Test #150: testscr_check_bashisms ...........   Passed    7.53 sec

when running the test. Is this maybe dependent on the version of checkbashism?

I have "devscripts package, version 2.21.3+deb11u1"

@kodebach
Copy link
Member

kodebach commented Apr 2, 2022

I still get:

But you only see this when running ctest in verbose mode. The testscr_check_bashisms doesn't fail, so you normally don't see all the warnings and non-fatal errors. That's what I meant with "the issue doesn't occur". It doesn't occur, as in it doesn't fail CI builds, which was the case originally.

Is this maybe dependent on the version of checkbashism?

The version probably plays a role too.

@flo91
Copy link
Collaborator

flo91 commented Apr 2, 2022

As @kodebach already mentioned, the difference is that in the first posting the test fails, while currently it passes.
I tried to run the test locally on Gentoo with dev-util/checkbashisms-2.21.4 via ctest -V and got the same output as @markus2330.

Another point is that the checkbashisms-script is not installed in the docker-container that is used when following the tutorial at https://www.libelektra.org/tutorials/run-all-tests-with-docker.

If I try to run the test inside the container, I get the output:
181: checkbashisms command needed for this test, aborting

After installing the script (version 2.22.1) in the container, I get the same output as locally on Gentoo and in the posting of @markus2330.

@flo91 flo91 added lang/shell testing errors in tests without implication on real usage labels Apr 2, 2022
@markus2330 markus2330 changed the title [CM T0] checkbashism [CM T0] checkbashism prints "strange results" Apr 2, 2022
@markus2330
Copy link
Contributor Author

The proper fix would probably be fixing checkbashisms that it proper deals with non-scripts and does not have "strange results".

For CM and this issue a whitelist/blacklist in the style of e.g. tests/linkchecker.whitelist is probably a nice solution. Goals are:

  • newly added shell scripts get checked
  • all scripts get checked (there are also shell scripts outside of the folder scripts)
  • no warnings or "strange results" when running testscr_check_bashisms

@kodebach
Copy link
Member

kodebach commented Apr 2, 2022

The proper fix would probably be fixing checkbashisms that it proper deals with non-scripts and does not have "strange results".

This happens, because we call checkbashisms with a list of file. It therefore tries to check all the listed files. If we don't want the "strange results" and other warnings, we must ensure that only actual shell scripts are given to checkbashisms.

So I would actually suggest:

  1. Check all files that end with .sh (file ending suggests shell script)
  2. Check all files that have a shebang
  3. Never check files that don't have a shebang or end with .sh
  4. whitelist/blacklist additional files

If we just rely on a whitelist/blacklist solution, we'd have to blacklist all non-shell-script files in scripts or whitelist all shell scripts in scripts. Both are not a good solution, because the current distribution looks like this (output of tokei):

===============================================================================
 Language            Files        Lines         Code     Comments       Blanks
===============================================================================
 Autoconf               19          928          651          132          145
 BASH                    9          472          338           66           68
 CMake                  56         6886         4388         1654          844
 Dockerfile             56         3509         2770          271          468
 Fish                    1          480          313           55          112
 INI                     1          392          323            2           67
 Python                  4          979          801           53          125
 Ruby                    1          125           67           40           18
 Shell                  70         2690         1837          442          411
 SVG                     1           17           17            0            0
 Plain Text              4          158            0          140           18
 XML                     1           22           22            0            0
 YAML                    1           53           49            3            1
-------------------------------------------------------------------------------
 Markdown               12          625            0          445          180
 |- BASH                 1           32           10           13            9
 |- Fish                 1            5            5            0            0
 (Total)                            662           15          458          189
===============================================================================
 Total                 236        17336        11576         3303         2457
===============================================================================

@flo91 flo91 added floss2022w and removed cm2022s for university course labels Oct 5, 2022
@flo91 flo91 changed the title [CM T0] checkbashism prints "strange results" checkbashism prints "strange results" Oct 5, 2022
Copy link

github-actions bot commented Jan 5, 2024

I mark this stale as it did not have any activity for one year. I'll close it in two weeks if no further activity occurs. If you want it to be alive again, ping by writing a message here or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot added the stale label Jan 5, 2024
Copy link

I closed this now because it has been inactive for more than one year. If I closed it by mistake, please do not hesitate to reopen it or create a new issue with the remainder of this issue.
Thank you for your contributions 💖

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1p one point continuous integration floss2022w lang/shell stale testing errors in tests without implication on real usage
Projects
None yet
Development

No branches or pull requests

5 participants