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.

[fmauch]

I would have my two cents on this, I'll try to write them down until tonight.

[fmauch]

Alright, my two cents: In my opinion it is better to run clang-format through pre-commit and then run pre-commit in CI.

This has the advantage, that the configured clang-format version

a) only has to be configured in once place (the pre-commit-config)
b) is the same for users if they setup pre-commit independent of their installed version, since it installed via python in the pre-commit config (see this example)

This is of course only my opinion, apart from that the workflow seems fine, so I'm still approving.

[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

ros2_control_ci/.pre-commit-config.yaml

Lines 63 to 67 in aa86661

	# CPP hooks
	- repo: https://github.com/pre-commit/mirrors-clang-format
	rev: v15.0.6
	hooks:
	- id: clang-format

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 #10 (comment).

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/[email protected]
 - 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.

[fmauch]

Maybe we went down a rabbit hole withe the "ament linters through pre-commit" approach, sorry for triggering this...

[fmauch]

This will probably get problematic on 24.04, since python 3.11 doesn't allow to install pip packages without a venv by default (see e.g. https://stackoverflow.com/questions/75602063/pip-install-r-requirements-txt-is-failing-this-environment-is-externally-mana). We can keep that in mind for later, though...

[christophfroehlich]

For me it seemed that every step has its own vritual environment? this is why https://github.com/pre-commit/action/blob/main/action.yml#L13 didn't show any of the ROS packages before?

[fmauch]

yes, the hooks use their own venv (if they are python hooks). However, pre-commit itself is installed without a venv here. I am simply referring to the python -m pip install pre-commit command. This will not work on 24.04 (without any modifications to the systemat least). One way to resolve this (Doing this on my 23.10 machine for instance) would be to install pipx (available in apt) and then do pipx install pre-commit && pipx ensurepath

[christophfroehlich]

then we will have more problems, for example also with the sphinx build I guess
https://github.com/ros-controls/control.ros.org/blob/51ff1372c4014845b4b3b7f9fd86644d05fa1db3/.github/workflows/sphinx-make-page.yml#L22-L32

let's keep it like this for now and remember it until rolling is targeting on 24.04 (which might be together with Jazzy release?)

[fmauch]

Would we want to specify the author here, as well? Otherwise, the default author Defaults to the user who triggered the workflow run which might be misleading in some situations.

[christophfroehlich]

ah, you are right: https://github.com/orgs/community/discussions/47855#discussioncomment-7061183

[christophfroehlich]

I'd test this first without specifying it, because in the demos of the action the PR is also from the github action bot without having specified it.. let's see ;)

[fmauch]

That seems nice now. Thank you @christophfroehlich!