Create `reusable-pre-commit.yml` and add workflow for it

[christophfroehlich]

Summary from the discussions below:

• we should use the pre-commit hook for clang instead of installing it locally
• the pre-commit github action does not work well with repo: local hooks, because it is run in a virtual environment and I didn't manage to get the ament_linters installed in it. I now manually run pre-commit from the same step as installing the linters, this works fine.
• To run the ament_linters with the CI workflow, I removed the stage: [commit] config.
• I also ran pre-commit autoupdate to bump the versions of the hooks. Additionally, I added a scheduled workflow for doing this once a week.
• Minor changes to pre-commit setup: adding linters for the github action/workflows and set env variable for cppcheck.

[christophfroehlich]

I'm not sure if I understand you..

We have this in the pre-commit config files, I think in every repo of ros-controls https://github.com/ros-controls/ros2_control_ci/blob/aa866611a771ebc3d9db342854925d6e881f08a0/.pre-commit-config.yaml#L63-L67

So the problem is here that it is installed with apt? I just copied that from ros2_controllers https://github.com/ros-controls/ros2_controllers/blob/d0987700e45782df54ae966e9a9bae251cde4a85/.github/workflows/ci-format.yml#L19

but maybe this is not necessary if it is installed via python later on?

[bmagyar]

We may not have this change consistently everywhere but yes, there is precommit native support now for.clang and we should use that everywhere. When we first set things up it either didn't exist yet or we missed it and went with the local clang instance which is problematic.

[fmauch]

I think I missed that this actually runs pre-commit. So basically, it boils down to: yes, there is not necessarily a problem, but it might be confusing that clang-format is installed in the workflow in version 14. Same goes for cppcheck. It isn't used in the pre-commit config (at least in the linked ros2_controllers workflow), but there we use ament_cppcheck which is configured for the commit stage only.

If a specific repo needs a specific apt package for their pre-commit configuration it can be installed in the specific workflow where this reusable workflow is included as a step.

Also, it might make sense to rename this workflow to reusable-pre-commit to make clear what it actually does. I mean, it doesn't even have to be doing any formatting at all :-)

When we first set things up it either didn't exist yet or we missed it and went with the local clang instance which is problematic.

Yes, that's also the progression I've seen and done in other repositories. This might be a good point to clean things up (which in this case boils down to remove some unused leftovers).

[christophfroehlich]

To summarize: I remove the apt install section totally, because pre-commit hooks install their correct dependencies already?

[christophfroehlich]

@fmauch problem:

 ament_cppcheck...........................................................Failed
- hook id: ament_cppcheck
- exit code: 1

Executable `ament_cppcheck` not found

ament_cpplint............................................................Failed
- hook id: ament_cpplint
- exit code: 1

Executable `ament_cpplint` not found

ament_lint_cmake.........................................................Failed
- hook id: ament_lint_cmake
- exit code: 1

Executable `ament_lint_cmake` not found

ament_copyright..........................................................Failed
- hook id: ament_copyright
- exit code: 1

Executable `ament_copyright` not found

[fmauch]

To summarize: I remove the apt install section totally, because pre-commit hooks install their correct dependencies already?

Not necessarily. If they are predefined python hooks: Yes. If they are a custom command, no. Hence the failures in https://github.com/ros-controls/ros2_control_ci/pull/10#issuecomment-1951459680.

However, since the missing packages are a consequence of the pre-commit-config.yml in the target repo I think it would only be right to install them in the workflow call.

The solution would be to add

 - uses: ros-tooling/setup-ros@0.7.1
 - name: Install system hooks
      run: sudo apt-get install -qq ros-${ROSDISTRO}-ament-cppckeck ros-${ROSDISTRO}-ament-cpplint  ros-${ROSDISTRO}-ament-lint-cmake  ros-${ROSDISTRO}-ament-copyright

where ROSDISTRO is specified somewhere.

Again, in my opinion it seems semantically correct to keep this together with the pre-commit-config file (meaning in each repo) but for maintenance reasons it might make sense to add those two steps to the reusable-pre-commit.yml and simply install all ament linters that we use anywhere.

[christophfroehlich]

As you have more experience, does it make sense to use pre-commit hooks like https://github.com/cmake-lint/cmake-lint/ or https://github.com/cpplint/cpplint instead? If we can do that for all linters, it would be much faster instead of running ros-tooling/setup-ros?

Edit: It's too complicated to get all the settings from the ament-linters into the default hooks. This is not maintainable.

[fmauch]

Edit: It's too complicated to get all the settings from the ament-linters into the default hooks. This is not maintainable.

Yes, that would have been my guess, as well.

[christophfroehlich]

Any guesses why pre-commit doesn't find the executables now? Do I have to source the ROS installation?

This doesn't fix it unfortunately

| Traceback (most recent call last):
|   File "/opt/ros/rolling/bin/ament_copyright", line 33, in <module>
|     sys.exit(load_entry_point('ament-copyright==0.16.2', 'console_scripts', 'ament_copyright')())
|   File "/opt/ros/rolling/bin/ament_copyright", line 22, in importlib_load_entry_point
|     for entry_point in distribution(dist_name).entry_points
|   File "/usr/lib/python3.10/importlib/metadata/__init__.py", line 969, in distribution
|     return Distribution.from_name(distribution_name)
|   File "/usr/lib/python3.10/importlib/metadata/__init__.py", line 548, in from_name
|     raise PackageNotFoundError(name)
| importlib.metadata.PackageNotFoundError: No package metadata was found for ament-copyright

python version 3.10 seems to be ok. Does this action run in an virtual environment?

my friend Copilot tells me:

However, since pre-commit runs each hook in its own isolated environment, the ament-copyright command might not be available to the hooks even if it's installed in the GitHub Actions runner. In this case, you might need to run ament-copyright as a separate step in your workflow, rather than as a pre-commit hook.

maybe this is just not possible, we have to add the commit-stage again and run the four linters manually. maybe this is the reason why it was set-up like this.

edit: running pre-commit in the same step manually works fine.