Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 19 commits into from
Feb 21, 2024

Conversation

christophfroehlich
Copy link
Contributor

@christophfroehlich christophfroehlich commented Feb 18, 2024

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.

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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

# 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
Copy link
Member

bmagyar commented Feb 18, 2024

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
Copy link
Contributor

fmauch commented Feb 18, 2024

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
Copy link
Contributor Author

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

@christophfroehlich christophfroehlich changed the title Create ci-format.yml Create reusable-pre-commit.yml Feb 18, 2024
@christophfroehlich
Copy link
Contributor Author

@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
Copy link
Contributor

fmauch commented Feb 18, 2024

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/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
Copy link
Contributor Author

christophfroehlich commented Feb 18, 2024

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
Copy link
Contributor

fmauch commented Feb 18, 2024

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
Copy link
Contributor Author

christophfroehlich commented Feb 18, 2024

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.

@christophfroehlich christophfroehlich changed the title Create reusable-pre-commit.yml Create reusable-pre-commit.yml and add workflow for it Feb 19, 2024
Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

run: |
sudo apt-get install -qq ros-${{ inputs.ros_distro }}-ament-cppcheck ros-${{ inputs.ros_distro }}-ament-cpplint ros-${{ inputs.ros_distro }}-ament-lint-cmake ros-${{ inputs.ros_distro }}-ament-copyright
source /opt/ros/${{ inputs.ros_distro }}/setup.bash
python -m pip install pre-commit
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?)

.github/workflows/reusable-pre-commit.yml Outdated Show resolved Hide resolved

- name: Create Pull Request
if: steps.git_status.outputs.changed == 'true'
uses: peter-evans/create-pull-request@v6
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ;)

Copy link
Contributor

@fmauch fmauch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems nice now. Thank you @christophfroehlich!

@christophfroehlich christophfroehlich merged commit ec7c2ac into master Feb 21, 2024
2 checks passed
@christophfroehlich christophfroehlich deleted the ci_format branch February 21, 2024 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants