ci: add a checkmake workflow by obnoxxx · Pull Request #15683 · rook/rook

obnoxxx · 2025-04-09T16:46:15Z

Description of changes:

This adds
linting of the Makefile in the CI
in order to prevent PRs from introducing errors in the Makefile

Checklist:

Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
Reviewed the developer guide on Submitting a Pull Request
Pending release notes updated with breaking and/or notable changes for the next minor release.
Documentation has been updated, if necessary.
Unit tests have been added, if necessary.
Integration tests have been added, if necessary.

travisn · 2025-04-22T14:19:12Z

+          wget https://github.com/mrtazz/checkmake/releases/download/0.2.2/checkmake-0.2.2.linux.amd64
+          sudo install ./checkmake-0.2.2.linux.amd64  /usr/local/bin/checkmake
+      - name: Run CheckMake
+        run: make checkmake


Looks like the action isn't working yet. The log shows:

Run make checkmake make checkmake shell: /usr/bin/bash -e {0} bash: line 1: /home/runner/work/rook/rook/.cache/tools/linux_amd6[4](https://github.com/rook/rook/actions/runs/14591180591/job/40945472851#step:4:4)/yq-v[4](https://github.com/rook/rook/actions/runs/14591180591/job/40945472851#step:4:5).45.1: No such file or directory checkmake Makefile

@travisn wrote:

Looks like the action isn't working yet. The log shows:

Run make checkmake make checkmake shell: /usr/bin/bash -e {0} bash: line 1: /home/runner/work/rook/rook/.cache/tools/linux_amd6[4](https://github.com/rook/rook/actions/runs/14591180591/job/40945472851#step:4:4)/yq-v[4](https://github.com/rook/rook/actions/runs/14591180591/job/40945472851#step:4:5).45.1: No such file or directory checkmake Makefile

This is strange! The workflow is reported as succeeding but the log is showing this error.

And it is yet unclear to me why it should require yq.
make checkmake does not require yq locally and the workflow yaml does not reference yq.

I can only guess that github is somehow using yq internally when processing the workflow yaml file.

If that is the case, I am still unclear how it can be fixed in the PR.

I tried to improve the situation by installing yq as part of the workflow, but it did not help.

Since it did not help, I removed installation of yq again (or rather, commented it out). Instead, I changed the workflow to run checkmake Makefile directly instead of invoking make checkmake because I suspected the error message might be related to the use of make. The result looks promising:
The job still succeeds but now without the weird yq-related error message.

As a next step, I intend to add a test commit to trigger failure of checkmake.

now that the the suspicion was confirmed, I removed the commented-out yq-installation lies to make the workflow file cleaner.

This change is not intended to be merged. It is intended to prove for PR rook#15683 that checkmake fails under circumstances. The commit be removed from the PR brfore merging. This adds a target "long" to the Makefile which should trigger the 'maxbodylength failure of checkmake: 'Target body for "long" exceeds allowed length of 5 (8).' Signed-off-by: Michael Adam <[email protected]>

obnoxxx · 2025-05-07T10:10:52Z

@travisn as we discussed, I added a test commit that triggers failure of checkmake to prove that this ci workflow can catch regressions in the Makefile.

I see that the job has failed as expected with the maxbodylength rule:

'Target body for "long" exceeds allowed length of 5 (8).'

If this PR is otherwise fine and agreed to be added, I'll remove the test commit before merging.

travisn · 2025-05-07T14:01:24Z

If this PR is otherwise fine and agreed to be added, I'll remove the test commit before merging.

Great to see it's working as expected now, so feel free to revert the test commit.

obnoxxx · 2025-05-07T14:34:11Z

@travisn wrote:

If this PR is otherwise fine and agreed to be added, I'll remove the test commit before merging.

Great to see it's working as expected now, so feel free to revert the test commit.

done. commit removed

travisn

no need to backport

parth-gr · 2025-05-07T15:39:44Z

+        run: |
+          sudo apt-get update
+          sudo apt-get install -y wget
+          wget https://github.com/mrtazz/checkmake/releases/download/0.2.2/checkmake-0.2.2.linux.amd64


does it not have a explicitly action plugin likehttps://github.com/rook/rook/blob/master/.github/workflows/auto-assign.yaml#L18C15-L18C75?

@parth-gr wrote:

does it not have a explicitly action plugin like https://github.com/rook/rook/blob/master/.github/workflows/auto-assign.yaml#L18C15-L18C75?

It does have an action:
https://github.com/marketplace/actions/checkmake-action

But I prefer to write workflows with commands that can easily be invoked locally by developers. Actions, in contrast, are putting the invocation in a blackbox that makes it difficult to understand what exactly is run.

In rook we have mostly use actions, @travisn for thoughts

One advantage of the actions is that the dependabot will automatically update them, otherwise we have to update them manually, which doesn't have often enough. So I do like the idea of using the action instead.

@travisn wrote:

One advantage of the actions is that the dependabot will automatically update them,
otherwise we have to update them manually, which doesn't have often enough. So I do like the idea of using the action instead.

Fair. rewritten to use the action. (but reluctantly ... ;)

obnoxxx · 2025-05-07T17:20:28Z

added a test commit again to trigger failure of the action.
It works as expected.

travisn

Looks good with the failure commit, feel free to revert that

travisn · 2025-05-07T17:24:45Z

+    steps:
+      - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+      - name: Run CheckMake
+        uses: Uno-Takashi/checkmake-action@main


Instead of main is there some tag we can use for the latest release?

@travisn wrote:

Instead of main is there some tag we can use for the latest release?

according to https://github.com/marketplace/actions/checkmake-action, v2is the latest version available.

I changed the workflow to use that.

obnoxxx · 2025-05-07T17:27:38Z

Looks good with the failure commit, feel free to revert that

done.

lint the Makefile in the CI This is to prevent PRs from introducing errors in the Makefile Signed-off-by: Michael Adam <[email protected]>

travisn · 2025-05-07T17:39:18Z

+      - name: Run CheckMake
+        uses: Uno-Takashi/checkmake-action@v2
+        with:
+          Makefile: Makefile


Does this only run on the makefile in the root folder? Can we also run it against the makefiles in other folders?

@travisn wrote:

Does this only run on the makefile in the root folder?
yes currently only against the main Makefile.

Can we also run it against the makefiles in other folders?

We can, but locally, these files show errors:

build/release/Makefile images/Makefile images/ceph/Makefile

Ok as discussed offline, let's go ahead with just the main Makefile for now.

travisn · 2025-05-07T18:00:09Z

+      - name: Run CheckMake
+        uses: Uno-Takashi/checkmake-action@v2
+        with:
+          Makefile: Makefile


Ok as discussed offline, let's go ahead with just the main Makefile for now.

obnoxxx requested review from sp98, subhamkrai and travisn April 9, 2025 16:47

travisn changed the title ~~ci: add a heckmake workflow~~ ci: add a checkmake workflow Apr 9, 2025

travisn reviewed Apr 9, 2025

View reviewed changes

Comment thread .github/workflows/checkmake.yaml Outdated

Comment thread .github/workflows/checkmake.yaml Outdated

obnoxxx force-pushed the ci-add-checkmake-workflow branch from a217bab to f77a1d0 Compare April 22, 2025 09:15

obnoxxx requested a review from travisn April 22, 2025 09:22

travisn requested changes Apr 22, 2025

View reviewed changes

obnoxxx force-pushed the ci-add-checkmake-workflow branch 3 times, most recently from 8d9f60f to 9f598b2 Compare April 23, 2025 11:57

travisn added the skip-ci label May 6, 2025

obnoxxx force-pushed the ci-add-checkmake-workflow branch 5 times, most recently from 60aabbd to d1b795d Compare May 7, 2025 10:05

obnoxxx force-pushed the ci-add-checkmake-workflow branch from d1b795d to 49ce1bf Compare May 7, 2025 10:13

obnoxxx requested a review from travisn May 7, 2025 10:17

obnoxxx mentioned this pull request May 7, 2025

document the rules in detail checkmake/checkmake#108

Open

obnoxxx force-pushed the ci-add-checkmake-workflow branch from 49ce1bf to b5625d1 Compare May 7, 2025 14:33

obnoxxx added backport-release-1.17 backport-release-1.16 labels May 7, 2025

travisn removed backport-release-1.16 backport-release-1.17 labels May 7, 2025

travisn approved these changes May 7, 2025

View reviewed changes

parth-gr approved these changes May 7, 2025

View reviewed changes

parth-gr reviewed May 7, 2025

View reviewed changes

obnoxxx force-pushed the ci-add-checkmake-workflow branch from b5625d1 to dcbf37d Compare May 7, 2025 17:10

obnoxxx requested a review from travisn May 7, 2025 17:22

travisn reviewed May 7, 2025

View reviewed changes

obnoxxx force-pushed the ci-add-checkmake-workflow branch from 87c1369 to dcbf37d Compare May 7, 2025 17:27

ci: add a checkmake workflow

5aa6d3b

lint the Makefile in the CI This is to prevent PRs from introducing errors in the Makefile Signed-off-by: Michael Adam <[email protected]>

obnoxxx force-pushed the ci-add-checkmake-workflow branch from dcbf37d to 5aa6d3b Compare May 7, 2025 17:29

obnoxxx requested a review from travisn May 7, 2025 17:33

travisn reviewed May 7, 2025

View reviewed changes

travisn approved these changes May 7, 2025

View reviewed changes

travisn merged commit 8a6afe7 into rook:master May 7, 2025
49 checks passed

obnoxxx deleted the ci-add-checkmake-workflow branch May 8, 2025 13:51

Conversation

obnoxxx commented Apr 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx Apr 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx Apr 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx commented May 7, 2025

Uh oh!

travisn commented May 7, 2025

Uh oh!

obnoxxx commented May 7, 2025

Uh oh!

travisn left a comment

Choose a reason for hiding this comment

Uh oh!

parth-gr May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx commented May 7, 2025

Uh oh!

travisn left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

obnoxxx commented May 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

obnoxxx commented Apr 9, 2025 •

edited

Loading

obnoxxx Apr 22, 2025 •

edited

Loading

obnoxxx Apr 23, 2025 •

edited

Loading

parth-gr May 7, 2025 •

edited

Loading

obnoxxx May 7, 2025 •

edited

Loading