Enforcement of commit formalities in CI

This is spawned from https://github.com/openwrt/luci/pull/7008#issuecomment-2017384640, where it was discussed about the length of the commit subjects (which I agree with). Looking at https://openwrt.org/submitting-patches, there's certain formal requirements on the commits, such as the length of subject and description, no full stop at end of subject, no capitalizing of word directly after prefix etc. which isn't enforced in the shared actions workflow while some things are (SoB, author name etc.)

Now, does anyone know if there's a specific reason these parameters aren't enforced? Adding 'em would catch a lot of formal errors which may otherwise get through the review process, especially for new contributors (just like me). Of course, I'm aware that GitHub isn't the only place to contribute (actually a fairly new one if I've got the picture correct) so it won't catch everything (for example direct commit access not going through GitHub PR:s) but since we're enforcing formal requirements anyway, is there a reason not to add 'em that I've missed?

1 Like

Having this formalized in the CI sounds like a good idea to me. It would take a tiny bit of burden off the maintainers and let submitters know sooner that something may get flagged in their commit message.

In the packages repo, I wonder if there's an easy way to check the Makefile's PKG_RELEASE, as I see it often being set incorrectly by those new to submitting PRs?

And now that I'm thinking about this a bit more, it would be even better if we had the script that's in the CI available to run on our local repo before we even submit the PR. I just cut out the guts from the shared action workflow, added in the ci_helpers.sh that it uses and ran that on a recent PR. Seems like a useful pre-push utility to me, and I'll start using it as part of my usual checks.

Is scripts/checkpatch.pl in the buildroot not performing (some of) these checks?

Maybe (I can barely read Perl so someone needs to help me out), but isn't that pretty tightly coupled to the openwrt repo? I was thinking it needs to work for at least the packages and luci repositories too. We can maybe copy it to the different repositories, but it doesn't actually solve the problem as I can't see that checkpatch.pl is run as part of the CI, then we need to trust each and every single developer to run it, which someone will forget.

We could always add checkpatch.pl as part of the CI if trivial to do so, my main question then (coming back to the beginning) is how easy/hard it would be to adapt it to the other repositories.

Sorry, I was thinking about checking patches locally prior to submitting, and you were thinking of checking them through the GitHub CI. I should go to bed, really :see_no_evil:.

No worries :stuck_out_tongue:

Hacked up script extracted from CI that enforces those items (free for anyone to use in any way: play with it or create a PR for the CI script or whatever you want).

formalities.sh
#!/bin/sh

#source .github/workflows/scripts/ci_helpers.sh
color_out() {
        printf "\e[0;$1m%s\e[0;0m\n" "$2"
}

success() {
        color_out 32 "$1"
}

info() {
        color_out 36 "$1"
}

err() {
        color_out 31 "$1"
}

warn() {
        color_out 33 "$1"
}

err_die() {
        err "$1"
        exit 1
}
# end ci_helpers.sh
BRANCH=master

          RET=0
          for commit in $(git rev-list HEAD ^origin/$BRANCH); do
            info "=== Checking commit '$commit'"
            if git show --format='%P' -s $commit | grep -qF ' '; then
              err "Pull request should not include merge commits"
              RET=1
            fi

            author="$(git show -s --format=%aN $commit)"
            if echo $author | grep -q '\S\+\s\+\S\+'; then
              success "Author name ($author) seems ok"
            else
              err "Author name ($author) need to be your real name 'firstname lastname'"
              RET=1
            fi

            subject="$(git show -s --format=%s $commit)"
            subject_len=$(echo "$subject" | wc -L)
            if echo "$subject" | grep -q -e '^[0-9A-Za-z,+/_\.-]\+: ' -e '^Revert '; then
              success "Commit subject line seems ok ($subject)"
            else
              err "Commit subject line MUST start with '<area>: ' ($subject)"
              RET=1
            fi
            if echo "$subject" | grep -q ': [A-Z]'; then
              err "Subject description must not start with capital letter"
              RET=1
            fi
            if [ "${subject: -1}" = '.' ]; then
              err "Subject line must not end with '.'"
              RET=1
            fi

            if [ "$subject_len" -gt 50 ]; then
              err "Commit subject line length of $subject_len is greater than maximum of 50"
              RET=1
            fi

            body="$(git show -s --format=%b $commit)"
            body_len=$(echo "$body" | wc -L)
            sob="$(git show -s --format='Signed-off-by: %aN <%aE>' $commit)"
            if echo "$body" | grep -qF "$sob"; then
              success "Signed-off-by match author"
            else
              err "Signed-off-by is missing or doesn't match author (should be '$sob')"
              RET=1
            fi

            if echo "$body" | grep -v "Signed-off-by:"; then
              success "A commit message exists"
            else
              err "Missing commit message. Please describe your changes"
              RET=1
            fi

            if [ "$body_len" -gt 75 ]; then
              err "Commit message line length of $body_len is greater than maximum of 75"
              RET=1
            fi
          done

          if [ "$RET" -ne 0 ]; then
            err "See https://openwrt.org/submitting-patches#submission_guidelines"
          fi

          exit $RET

Example run on a recent PR:

1 Like