Enforce pre-commit in ArduPilot #24683

Ryanf55 · 2023-08-17T22:37:06Z

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 · 2023-08-18T02:13:44Z

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

.pre-commit-config.yaml

khancyr

we should fix the files instead of excluding them.

.pre-commit-config.yaml

tridge · 2023-08-22T00:35:37Z

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

Ryanf55 · 2023-08-22T02:31:09Z

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: home-assistant/core#86892
I'll give it a whirl.

Ryanf55 · 2023-10-31T00:11:15Z

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

.github/workflows/test_scripts.yml

Ryanf55 · 2023-10-31T01:14:21Z

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.

* Needed by ArduPilot/ardupilot#24683 Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 · 2023-11-28T05:44:35Z

@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 · 2023-11-28T08:34:53Z

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

MichelleRos · 2023-11-29T06:29:24Z

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 · 2023-11-29T06:44:22Z

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

khancyr · 2023-11-29T12:20:45Z

Tools/scripts/build_ci.sh

@@ -424,6 +424,13 @@ for t in $CI_BUILD_TARGET; do
        continue
    fi

+    if [ "$t" == "pre-commit-cleanliness" ]; then
+        echo "Checking pre-commit code cleanliness"
+        pre-commit install


@Ryanf55 that means that after a first run precommit will be enable on git commit. Probably not the best move. Just doing the pre-commit run is enough and don't mess the git config for now (I still want pre-commit)

Are you saying that local developers run build_ci with specific arguments to hit this code block?

Without the above line, the job would fail in CI because pre-commit wasn't enabled for the repo.

@Ryanf55 I think Pierre is saying we shouldn't be making this permanent change to their setup in here.

We can do it as part of the machine setup in the github workflows instead, yes?

yes, but I thought it would be ok because this is a CI script. I'm happy to move it if you think we need to worry about devs running this check and then being surprised pre-commit is now enabled.

Yes, please move it.

Done! Let's see how it goes...

@Ryanf55 normally there is not issue to use pre-commit run --all-files on CI, install isn't need for CI

magicrub · 2024-01-13T21:52:54Z

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

Ryanf55 · 2024-01-13T22:24:09Z

@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 · 2024-01-14T01:11:09Z

@khancyr ping!

Signed-off-by: Ryan Friedman <[email protected]>

* These are not scripts and should not be executable Signed-off-by: Ryan Friedman <[email protected]>

* It should be moved to the workflow Signed-off-by: Ryan Friedman <[email protected]>

Signed-off-by: Ryan Friedman <[email protected]>

khancyr · 2024-09-06T08:25:03Z

.github/workflows/test_scripts.yml

+      - name: install pre-commit
+        shell: bash
+        run: |
+          pre-commit install


why need this ? we don't run git command on CI, so the install isn't need normally, or I miss something ?

are you sure that shouldn't be pip install pre-commit instead ?

OK i have checked and we got pre-commit into the docker image already so we are fine. the pre-commit install is just to install the git hook so that isn't need on CI

You are conflating installation of the pre-commit tool in the environment and installation of the git hooks into the repo.
https://pre-commit.com/#3-install-the-git-hook-scripts

We need both at some point before calling pre-commit run in ArduPilot.

Nope you don't need to install the git hook to use precommit.

khancyr

something is wrong with this commit as it moves the submodules ! ce6e3fb (the xmllint one's )

Ryanf55 added DevCallTopic Experiment labels Aug 17, 2023

Ryanf55 mentioned this pull request Aug 18, 2023

Add pre-commit to the docker environment ArduPilot/ardupilot_dev_docker#18

Merged

khancyr reviewed Aug 18, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

khancyr reviewed Aug 18, 2023

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

CraigElder removed the DevCallTopic label Aug 22, 2023

Ryanf55 mentioned this pull request Aug 22, 2023

pre-commit: Fix current errors #24727

Merged

Ryanf55 mentioned this pull request Aug 22, 2023

pre-commit: Run autoupdate #24728

Merged

Ryanf55 force-pushed the enforce-pre-commit branch 4 times, most recently from 9f2f02a to 48cc7b1 Compare September 11, 2023 14:31

Ryanf55 added the DevCallTopic label Sep 11, 2023

Ryanf55 marked this pull request as ready for review September 11, 2023 14:32

CraigElder removed the DevCallTopic label Sep 11, 2023

tridge added the NeedsTesting label Sep 11, 2023

Ryanf55 force-pushed the enforce-pre-commit branch 2 times, most recently from 173500a to 7a89854 Compare October 31, 2023 00:06

Ryanf55 added the DevCallTopic label Oct 31, 2023

CraigElder removed the DevCallTopic label Oct 31, 2023

peterbarker reviewed Oct 31, 2023

View reviewed changes

.github/workflows/test_scripts.yml Show resolved Hide resolved

Ryanf55 mentioned this pull request Oct 31, 2023

Enforce line endings in AP_DDS with pre-commit #24540

Closed

Ryanf55 added a commit to Ryanf55/ardupilot_dev_docker that referenced this pull request Nov 10, 2023

Add missing pre-commit dependencies

4475bfa

* Needed by ArduPilot/ardupilot#24683 Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 mentioned this pull request Nov 10, 2023

Add missing pre-commit dependencies ArduPilot/ardupilot_dev_docker#20

Merged

khancyr pushed a commit to ArduPilot/ardupilot_dev_docker that referenced this pull request Nov 11, 2023

Add missing pre-commit dependencies

d087e87

* Needed by ArduPilot/ardupilot#24683 Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 force-pushed the enforce-pre-commit branch from c027dd7 to d3b4d7d Compare November 26, 2023 16:40

Ryanf55 force-pushed the enforce-pre-commit branch from 982299b to 2fa7168 Compare November 29, 2023 05:42

Ryanf55 added CI and removed NeedsTesting labels Nov 29, 2023

Ryanf55 added the DevCallEU label Nov 29, 2023

tridge approved these changes Nov 29, 2023

View reviewed changes

tridge added WikiNeeded needs wiki update MergeOnCIPass and removed DevCallEU labels Nov 29, 2023

khancyr suggested changes Nov 29, 2023

View reviewed changes

Ryanf55 added NeedAnswer NeedsHelp and removed MergeOnCIPass labels Jan 13, 2024

Ryanf55 added 7 commits September 5, 2024 17:17

.github: enforce pre-commit in ArduPilot CI

410327d

Signed-off-by: Ryan Friedman <[email protected]>

SITL: Fix missing newline

6ae04f5

Signed-off-by: Ryan Friedman <[email protected]>

Tools: Enforce pre-commit in build_ci.sh

fb420b1

Signed-off-by: Ryan Friedman <[email protected]>

Tools: Add xmllint to dependencies

ce6e3fb

Signed-off-by: Ryan Friedman <[email protected]>

AP_HAL_ChibiOS: Remove executable permissions from hwdef

ed69d10

* These are not scripts and should not be executable Signed-off-by: Ryan Friedman <[email protected]>

Tools: Do not install pre-commit in CI shell

ad599c3

* It should be moved to the workflow Signed-off-by: Ryan Friedman <[email protected]>

.github: Install pre-commit in workflow as-needed

7fb8fc7

Signed-off-by: Ryan Friedman <[email protected]>

Ryanf55 force-pushed the enforce-pre-commit branch from 2fa7168 to 7fb8fc7 Compare September 5, 2024 23:23

khancyr reviewed Sep 6, 2024

View reviewed changes

khancyr suggested changes Sep 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enforce pre-commit in ArduPilot #24683

Enforce pre-commit in ArduPilot #24683

Ryanf55 commented Aug 17, 2023 •

edited

Loading

Ryanf55 commented Aug 18, 2023

khancyr left a comment

tridge commented Aug 22, 2023

Ryanf55 commented Aug 22, 2023

Ryanf55 commented Oct 31, 2023 •

edited

Loading

Ryanf55 commented Oct 31, 2023

Ryanf55 commented Nov 28, 2023

khancyr commented Nov 28, 2023

MichelleRos commented Nov 29, 2023

Ryanf55 commented Nov 29, 2023 •

edited

Loading

khancyr Nov 29, 2023

Ryanf55 Nov 29, 2023 •

edited

Loading

peterbarker Sep 4, 2024

Ryanf55 Sep 5, 2024

peterbarker Sep 5, 2024

Ryanf55 Sep 5, 2024

khancyr Sep 6, 2024

magicrub commented Jan 13, 2024

Ryanf55 commented Jan 13, 2024

magicrub commented Jan 14, 2024

khancyr Sep 6, 2024

khancyr Sep 6, 2024

khancyr Sep 6, 2024

Ryanf55 Sep 6, 2024

khancyr Sep 6, 2024

khancyr left a comment •

edited

Loading

Enforce pre-commit in ArduPilot #24683

Are you sure you want to change the base?

Enforce pre-commit in ArduPilot #24683

Conversation

Ryanf55 commented Aug 17, 2023 • edited Loading

Related issues

Purpose

How to test

Future ideas

Ryanf55 commented Aug 18, 2023

khancyr left a comment

Choose a reason for hiding this comment

tridge commented Aug 22, 2023

Ryanf55 commented Aug 22, 2023

Ryanf55 commented Oct 31, 2023 • edited Loading

Ryanf55 commented Oct 31, 2023

Ryanf55 commented Nov 28, 2023

khancyr commented Nov 28, 2023

MichelleRos commented Nov 29, 2023

Ryanf55 commented Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Ryanf55 Nov 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

magicrub commented Jan 13, 2024

Ryanf55 commented Jan 13, 2024

magicrub commented Jan 14, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khancyr left a comment • edited Loading

Choose a reason for hiding this comment

Ryanf55 commented Aug 17, 2023 •

edited

Loading

Ryanf55 commented Oct 31, 2023 •

edited

Loading

Ryanf55 commented Nov 29, 2023 •

edited

Loading

Ryanf55 Nov 29, 2023 •

edited

Loading

khancyr left a comment •

edited

Loading