Add linting/static-analysis for shell scripts to CI

[Bckempa]

The Space ROS organization has over 40 shell scripts, but they are currently not subjected to code style or testing requirements so any errors are discovered at the integration level where debugging and attribution is more difficult. For example, the recent space-ros/docker PR 76 fixed space-ros/docker issue 75 by adding common best-practice configuration to the build.sh files.

I propose adding a shell-focused listing and static analysis pass to CI using shellcheck, an actively developed open source project for detecting common errors in bash/sh scripts.

[Bckempa]

We discussed this at the last technical meeting and I'll take a swing at this soon if no one beats me to it.

[mkhansenbot]

It looks like someone has created an action for this here: https://github.com/marketplace/actions/action-shellcheck

It would be great if you can add the workflow to run it!

[EzraBrooks]

Thanks for bringing this up, I find shellcheck to be an invaluable tool for managing the inherent risk involved in shell scripts :D

[Bckempa]

@EzraBrooks were you intending to take this one or #90 ?

[EzraBrooks]

#90 - my bad, had them all open in tabs and switched to the wrong one.

[mkhansenbot]

Added to next milestone (proposed) humble-2024.04.0

[Ryanf55]

Hi team, would you consider pre-commit as part of your workflow?
https://github.com/koalaman/shellcheck?tab=readme-ov-file#pre-commit

We are using this for ArduPilot development and it helps new contributors run all the linters automatically just before they commit, before it gets to CI.

Here is the usage in the README:
https://github.com/ArduPilot/ardupilot_ros?tab=readme-ov-file#contribution-guideline

Feel free to check out that repo, install pre-commit, make some python code changes that ruin formatting (add whitespace) and see how it works locally.

You can see here we run an XML formatter:
https://github.com/ArduPilot/ardupilot_ros/blob/humble/.pre-commit-config.yaml#L18

Pre-commit also integrates nicely with Github Actions. There is even support for pre-commit to auto-fix contributions and push a follow up commit.

[EzraBrooks]

The MoveIt team uses pre-commit as well, I'd be open to a contribution of a pre-commit config as a solution to this issue!

[Ryanf55]

The MoveIt team uses pre-commit as well, I'd be open to a contribution of a pre-commit config as a solution to this issue!

Thanks. Should I open up a discussion or thread to select the set of hooks and configurations?

[EzraBrooks]

I would recommend we start with shellcheck and hadolint, as those both have pre-commit hooks and are already mentioned in this issue and space-ros/docker#82 :)

[Bckempa]

@Ryanf55 thanks for the suggestion! I also use pre-commit on several projects and see how it could be valuable here. Also your idea of opening a discussion for the specific settings of these tools makes sense to me.

An architectural point that came up during the last release was if these types of checks should be done via GHA directly, how to control dev machine variance, etc. and at the time we were leaning toward integrating it with the containerized build process to reduce requirements for devs and ensure we're "testing like we fly" between local machines and CI.

This has been a facet of the the running discussion on build-system rework and I'm interested in integrating a phase for these checks in the new build flow that I'm focused on for this release cycle.

I've only used it similar to how you linked in ArduPilot where it's a global install for a single repo, does anyone have thoughts on structuring to cover the whole org without copying config files and GHA jobs between repos?

[Ryanf55]

Thank you for the feedback.

1. The pre-commit config is designed to only enforce settings at a per-repo basis. The tool isolates the tool installations in a python virtual environment specific to each repo. Any tools that rely on apt packages such as clang-tidy or xmllint-dev are thus specific to the OS environment. Luckily, most ROS developers use the Tier 1 platform of Ubuntu 22 when developing humble, and this is not a problem.

2. Pre-commit designers are hostile towards making the .pre-commit configuration shareable between repos because the tool is not designed for it. My team did this work internally for about 30 repositories. It was not fun, and caused huge problems with merge conflicts in long-standing branches off main, but we did it anyways. Pre-commit can use a docker executor for isolation and re-use but it has a significant maintenance burden and slows down pre-commit by an order of magnitude. More details are found in this thread: [Question] How can I configure pre-commit globally for all projects? pre-commit/pre-commit#450. I think if we skip the docker executor, it would be possible to create an equivalent ament_lint_common of spaceros_precommit_common for repos here, but we may have problems if this tool will cause a significant code divergence from what's already merged. A good example of when this happens is a file written with DOS line endings instead of UNIX endings, which pre-commit protects against. Further information on sharing configuration is here: Metahooks pre-commit/pre-commit#731 (comment). TLDR: the pre-commit file itself, once established, does not change often, and is not a big maintenance problem. Adding one in the first place provides most of the value of the tool, and people rarely care that things are the same across all repos; what is more important to devs is that repositories are internally consistent, and this tool meets that need.

3. Re-use of github actions is a well-accepted pattern that can be added to avoid code duplication of CI files. I would be happy to do add this scope.

[Ryanf55]

Hello,

Would you like me to start a discussion tab to keep this conversation going? I'm happy to contribute an MVP of linting with pre-commit by the April release.

[Bckempa]

Yes, please do!

[Ryanf55]

Discussion: #142