bareos-check-sources: add shell_format plugin#2267
Conversation
arogge
left a comment
There was a problem hiding this comment.
The functionality itself seems fine.
I left left quite a lot of remarks, mostly on error-checking. pylint also wasn't too happy and reported only 5.77 out of 10.
devtools/pip-tools/check_sources/plugins/shell_format_plugin.py
Outdated
Show resolved
Hide resolved
devtools/pip-tools/check_sources/plugins/shell_format_plugin.py
Outdated
Show resolved
Hide resolved
devtools/pip-tools/check_sources/plugins/shell_format_plugin.py
Outdated
Show resolved
Hide resolved
| if shfmt_exe: | ||
| logging.getLogger(__name__).info("using executable {}".format(shfmt_exe)) | ||
| else: | ||
| logging.getLogger(__name__).error("cannot find a shfmt executable") |
There was a problem hiding this comment.
Not having shfmt will log an error, but won't exit.
As a result you get a backtrace that doesn't really help a lot. I guess we can easily do better than that.
There was a problem hiding this comment.
Is this change still required even though we manage shfmt with pip? If that fails we will get an error before even calling bareos-check-sources anyway I guess..
There was a problem hiding this comment.
No if setting up pip-tools will install shfmt, there is no reason to check for it again.
However, when using which("shfmt") there is no guarantee that you get the tool that came from shfmt-py. But I guess we can ignore that until it becomes a problem (like we already do for the other tools).
devtools/pip-tools/check_sources/plugins/shell_format_plugin.py
Outdated
Show resolved
Hide resolved
I basically copied the clang_format_plugin.py and changed it from there. |
986d447 to
b0ca44d
Compare
arogge
left a comment
There was a problem hiding this comment.
This looks pretty good already!
Nevertheless, I found two issues that still need to be fixed:
- reading from disk instead of using the in-memory buffer (see detailed comment).
- When running
bareos-check-sources --plugin shell_format --all, the plugin will fail on at leastcore/platforms/redhat/bareos-dir.in. This is correct, because the shebang in that file seems to be wrong (i.e. should bebashnot justsh). I guess it makes sense to fix the files thatshfmtdoesn't like in a separate commit, so the plugin works correctly on all pre-existing files.
Maybe we should also run this with --all to reformat all existing scripts, but maybe this is out-of-scope for this PR and should happen in another one.
| if shfmt_exe: | ||
| logging.getLogger(__name__).info("using executable {}".format(shfmt_exe)) | ||
| else: | ||
| logging.getLogger(__name__).error("cannot find a shfmt executable") |
There was a problem hiding this comment.
No if setting up pip-tools will install shfmt, there is no reason to check for it again.
However, when using which("shfmt") there is no guarantee that you get the tool that came from shfmt-py. But I guess we can ignore that until it becomes a problem (like we already do for the other tools).
| invocation = [shfmt_exe] + list(argv) + [file_path] | ||
| try: | ||
| proc = subprocess.run( | ||
| invocation, | ||
| stdout=subprocess.PIPE, | ||
| stderr=subprocess.PIPE, | ||
| encoding="utf-8", | ||
| universal_newlines=True, | ||
| check=True, | ||
| ) |
There was a problem hiding this comment.
This code runs shfmt with the file's path as a parameter, so shfmt will open and read the file.
While this looks good at first glance, the intended way is to provide file_content on stdin and pass the filename (preferably with the suffix .in stripped) as a hint to the tool using the --filename parameter.
This is because bareos-check-sources will read the file once and then run every eligible plugin on the content in no particular order. Thus, when your plugin runs another plugin may already have mutated the content. Therefore the file_content given to your plugin can differ from what is in the file.
A simple way to test this is to create a file test.sh with the following content and run bareos-check-sources on it (see --diff and --untracked parameters)
# Copyright (C) 2020-2020 Bareos GmbH & Co. KG
echo This is not well formatted >> /dev/null
If the plugin-order is the same for you (I'm not 100% sure it will be) the copyright plugin will change the 2020-2020 to 2020-2025 first and then your current plugin will ignore that change, effectively undoing it to 2020-2020 again, which is not what we want, of course.
| @@ -0,0 +1,72 @@ | |||
| # BAREOS® - Backup Archiving REcovery Open Sourced | |||
| # | |||
| # Copyright (C) 2020-2025 Bareos GmbH & Co. KG | |||
There was a problem hiding this comment.
this is new, so it should probably read 2025-2025
2770459 to
584c99b
Compare
- update shebang regex such that it detects "/usr/bin/env bash" - precompile regex - format .sh and .sh.in files without shebang - add type annotations - address pylint warnings
apply shfmt on file_content, not file_path and fix copyright-year
c8929d4 to
f91e82a
Compare
Fixes issue #321: Automate formatting of shell scripts with shfmt
This PR adds a plugin to the bareos-check-sources tool to check the formatting for shell scripts.
Detecting Shell Scripts:
A file is detected as a shell script not by its extension (not every shell file has a .sh extension)
but by its shebang at the start of the script.
Every file is a shell script that starts with
"#!some/path/to/sh" or "#!some/path/to/bash"
Note: The shebang can have whitespace before the path, i.e. "#! /bin/sh" but not within the path.
This means that "#!/home/me/some directory with spaces/sh" is not detected as a shell shebang and therefore no formatting is checked.
Thank you for contributing to the Bareos Project!
Please check
If you have any questions or problems, please give a comment in the PR.
Helpful documentation and best practices
Checklist for the reviewer of the PR (will be processed by the Bareos team)
Make sure you check/merge the PR using
devtools/pr-toolto have some simple automated checks run and a proper changelog record added.General
Required backport PRs have been createdCorrect milestone is setSource code quality