Enforce pre-commit in ArduPilot

[Ryanf55]

Related issues

Fixes #24555. Depends on #24727.

Purpose

Pre-commit provides basic guardrails for the repos, but it's not enforced. This PR adds it as a CI job.

How to test

CI_BUILD_TARGET=pre-commit-cleanliness ./Tools/scripts/build_ci.sh

Future ideas

We could autofix and re-commit the changes in CI to save developer time, but one could see it as risky.

Actions from Tridge's testing:

• We should add --allow-missing-config, this will support ArduPilot devs working on branches before 4.3.
• Stop pre-commit from running when using subsystem split
• Need dev documentation on pre-commit

[Ryanf55]

Need to install pre-commit as part of the normal set of python packages. https://github.com/ArduPilot/ardupilot/actions/runs/5896623868/job/15994803894#step:4:142

[tridge]

problem on older branches, this is on Copter-4.3:

[Ryanf55]

problem on older branches, this is on Copter-4.3:

Looks like we might be able to force an upgrade of isort to fix this: https://github.com/home-assistant/core/issues/86892 I'll give it a whirl.

[Ryanf55]

Rebased and fixed the regression. Dropped changes of installing pre-commit automatically.

ryan@B650-970:~/Dev/ros2_ws/src/ardupilot$ pre-commit run --all-files
Check line ending character (LF).........................................Failed
- hook id: mixed-line-ending
- exit code: 1
- files were modified by this hook

libraries/SITL/SITL_Airspeed.cpp: fixed mixed line endings

[Ryanf55]

Here's a demo for the CI script with a good PR:

ryan@B650-970:~/Dev/ros2_ws/src/ardupilot$ CI_BUILD_TARGET=pre-commit-cleanliness  ./Tools/scripts/build_ci.sh
...
+ echo 'Checking pre-commit code cleanliness'
Checking pre-commit code cleanliness
+ pre-commit install
pre-commit installed at .git/hooks/pre-commit
+ pre-commit run --all-files --verbose --show-diff-on-failure
Check line ending character (LF).........................................Passed
- hook id: mixed-line-ending
- duration: 0.06s
check for added large files..............................................Passed
- hook id: check-added-large-files
- duration: 0.06s
check that executables have shebangs.....................................Passed
- hook id: check-executables-have-shebangs
- duration: 0.04s
check that scripts with shebangs are executable..........................Passed
- hook id: check-shebang-scripts-are-executable
- duration: 0.06s
check for merge conflicts................................................Passed
- hook id: check-merge-conflict
- duration: 0.05s
check xml................................................................Passed
- hook id: check-xml
- duration: 0.04s
check yaml...............................................................Passed
- hook id: check-yaml
- duration: 0.04s
Formats XML files........................................................Passed
- hook id: format-xmllint
- duration: 0.02s

XML check for libraries/AP_DDS/dds_xrce_profile.xml: OK

black....................................................................Passed
- hook id: black
- duration: 0.07s

All done! ✨ 🍰 ✨
2 files left unchanged.

+ continue
+ echo build OK
build OK
+ exit 0

Now, if we accidentally make a cpp file executable, you get this in the CI logs:

+ pre-commit run --all-files --verbose --show-diff-on-failure
Check line ending character (LF).........................................Passed
- hook id: mixed-line-ending
- duration: 0.06s
check for added large files..............................................Passed
- hook id: check-added-large-files
- duration: 0.06s
check that executables have shebangs.....................................Failed
- hook id: check-executables-have-shebangs
- duration: 0.04s
- exit code: 1

ArduCopter/AP_ExternalControl_Copter.cpp: marked executable but has no (or invalid) shebang!
  If it isn't supposed to be executable, try: `chmod -x ArduCopter/AP_ExternalControl_Copter.cpp`
  If on Windows, you may also need to: `git add --chmod=-x ArduCopter/AP_ExternalControl_Copter.cpp`
  If it is supposed to be executable, double-check its shebang.

check that scripts with shebangs are executable..........................Passed
- hook id: check-shebang-scripts-are-executable
- duration: 0.06s
check for merge conflicts................................................Passed
- hook id: check-merge-conflict
- duration: 0.05s
check xml................................................................Passed
- hook id: check-xml
- duration: 0.03s
check yaml...............................................................Passed
- hook id: check-yaml
- duration: 0.04s
Formats XML files........................................................Passed
- hook id: format-xmllint
- duration: 0.02s

XML check for libraries/AP_DDS/dds_xrce_profile.xml: OK

black....................................................................Passed
- hook id: black
- duration: 0.07s

All done! ✨ 🍰 ✨
2 files left unchanged.

[Ryanf55]

@khancyr Can you create a new tag/release for the dev dockerfiles? v0.1.3 ,which is being used above, results in the failed build because it's missing xmllint.

[khancyr]

my bad, I haven't seen it wasn't in the release. I have force push a rebuild with the latest version

[MichelleRos]

Is there an option to disable this pre-vcommit though? I really wouldn't want to be forced to obey specific rules when I'm just doing test development in my own fork.

That would really slow down development.

[Ryanf55]

Is there an option to disable this pre-vcommit though? I really wouldn't want to be forced to obey specific rules when I'm just doing test development in my own fork.

That would really slow down development.

Yep! git commit --no-verify disables it when you commit.

That said, do you have examples of this slowing down development and specifics on which hooks or workflows have affected you? We only run a bare minimum set of hooks right now.

You can also disable specific hooks like so: https://stackoverflow.com/questions/7230820/skip-git-commit-hooks

[magicrub]

@Ryanf55 This is tagged MergeOnCiPass, and it's passing, but it has a merge conflict

[Ryanf55]

@Ryanf55 This is tagged MergeOnCiPass, and it's passing, but it has a merge conflict

I never got an answer on Pierre's comment. I don't have a path forward on this without feedback on the raised concern.

[magicrub]

@khancyr ping!

[peterbarker]

70 failing tests, un-addressed review comments, many conflicts.... and commits into the submodules?!

It looks like we might have been close to merging this one... but the ball's in your court here, @Ryanf55

Are you still wanting to pursue this one?

[Ryanf55]

70 failing tests, un-addressed review comments, many conflicts.... and commits into the submodules?!

It looks like we might have been close to merging this one... but the ball's in your court here, @Ryanf55

Are you still wanting to pursue this one?

I gave up - someone else can do this.