From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org>, Sergey Bronnikov <estetus@gmail.com> Cc: tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 2/2] ci: enable checkpatch Date: Mon, 17 Jul 2023 16:22:54 +0300 [thread overview] Message-ID: <22b03068-25ce-53e9-f623-1eaa851e064a@tarantool.org> (raw) In-Reply-To: <ZLFDdBcCpgDLP3V0@root> Hi, Sergey! thanks for review! Fixes applied and force-pushed. On 7/14/23 15:45, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > Please, consider my comments below. > > On 11.07.23, Sergey Bronnikov wrote: >> From: Sergey Bronnikov <sergeyb@tarantool.org> >> >> Patch enables checkpatch [1] for checking patch on a pre-commit stage. > Minor: Strictly saying it's not a precommit stage (since it's not a > pre-commit hook). But this is a prerequisite for other testing workflow. > >> In Tarantool we use our own fork of checkpatch [2] with additional check >> types. It's logical to use it in a LuaJIT development. We don't need > Also, it is good to have some checks for our C code like: > * <src/lib_misc.c> > * <src/lj_mapi.c> > * <src/lj_memprof.[ch]> > * <src/lj_symtab.[ch]> > * <src/lj_sysprof.[ch]> > * <src/lj_utils.h> > * <src/lj_utils_leb128.c> > * <src/lj_wbuf.[ch]> > * <src/lmisclib.h> > But they are contradicting with Tarantool's guidelines as far as they > are written in LuaJIT's style. Have we some way to check them too? New findings: diff --git a/src/lj_sysprof.c b/src/lj_sysprof.c index 2e9ed9b3..745eb1a3 100644 --- a/src/lj_sysprof.c +++ b/src/lj_sysprof.c @@ -485,7 +485,7 @@ int lj_sysprof_stop(lua_State *L) if (SPS_HALT == sp->state) { errno = sp->saved_errno; sp->state = SPS_IDLE; - /* wbuf was terminated when error occured. */ + /* wbuf was terminated when error occurred. */ return PROFILE_ERRIO; } diff --git a/src/lj_wbuf.h b/src/lj_wbuf.h index 9eaa5e51..33ec8463 100644 --- a/src/lj_wbuf.h +++ b/src/lj_wbuf.h @@ -75,7 +75,7 @@ void lj_wbuf_addn(struct lj_wbuf *buf, const void *src, size_t n); /* Write string to the buffer. */ void LJ_FASTCALL lj_wbuf_addstring(struct lj_wbuf *buf, const char *s); -/* Immediatly flush the buffer. */ +/* Immediately flush the buffer. */ void LJ_FASTCALL lj_wbuf_flush(struct lj_wbuf *buf); /* Check flags. */ new fixes were added (amended) to a commit with typos fixes. > >> check tags in commit messages like NO_DOC, NO_CHANGELOG, NO_TEST and > Side note: Not so sure, that NO_TEST check is a good thing, but I'm OK > with it, since it may be useful for backporting refactoring changes. I don't remember a case when someone missed a test for the patch with fix, so I would leave this options disabled. (Let's not cure what does not hurt.) > >> others, so to be able to customize command-line options Github Action, provided >> by checkpatch repository [3], was added to the repository. >> >> See documentation for used checkpatch in [4]. >> >> 1. https://docs.kernel.org/dev-tools/checkpatch.html >> 2. https://github.com/tarantool/checkpatch >> 3. https://github.com/tarantool/checkpatch/blob/master/.github/actions/checkpatch/action.yml >> 4. https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst > Also, is it possible to create the make target similar to > LuaJIT-luacheck? It should: > 0) Be dummy if there is no chekcpatch or codespell installed. Added. > 1) Be included in the make test check. Didn't get it. What make target did you mean? "test" or "check"? I've added a new target "check" and added LuaJIT-luacheck and LuaJIT-checkpatch there. > It is useful for local spellcheck without any push to remote CI. > >> --- >> .github/actions/checkpatch/action.yml | 17 +++++++++++++++++ >> .github/workflows/lint.yml | 14 ++++++++++++++ >> 2 files changed, 31 insertions(+) >> create mode 100644 .github/actions/checkpatch/action.yml >> >> diff --git a/.github/actions/checkpatch/action.yml b/.github/actions/checkpatch/action.yml >> new file mode 100644 >> index 00000000..df2e2a2b >> --- /dev/null >> +++ b/.github/actions/checkpatch/action.yml >> @@ -0,0 +1,17 @@ >> +name: Checkpatch >> +description: Check patches against LuaJIT development guidelines >> +inputs: >> + revision-range: >> + description: Git revision range to check >> + required: true >> +runs: >> + using: composite >> + steps: >> + - uses: actions/checkout@v3 >> + with: >> + repository: tarantool/checkpatch >> + path: 'checkpatch' >> + - run: apt install -y codespell >> + shell: bash >> + - run: checkpatch/checkpatch.pl --codespell --color=always --show-types --git ${{ inputs.revision-range }} --codespellfile /usr/lib/python3/dist-packages/codespell_lib/data/dictionary.txt --ignore NO_CHANGELOG,NO_DOC,NO_TEST,COMMIT_LOG_LONG_LINE > Please, split this line into several. Done. --- a/.github/actions/checkpatch/action.yml +++ b/.github/actions/checkpatch/action.yml @@ -13,5 +13,12 @@ runs: path: 'checkpatch' - run: apt install -y codespell shell: bash - - run: checkpatch/checkpatch.pl --codespell --color=always --show-types --git ${{ inputs.revision-range }} --ignore NO_CHANGELOG,NO_DOC ,NO_TEST,COMMIT_LOG_LONG_LINE + - run: checkpatch/checkpatch.pl --codespell \ + --color=always \ + --show-types \ + --git ${{ inputs.revision-range }} \ + --ignore NO_CHANGELOG \ + --ignore NO_DOC \ + --ignore NO_TEST \ + --ignore COMMIT_LOG_LONG_LINE shell: bash > >> + shell: bash >> diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml >> index 71ceee9a..d28aa15b 100644 >> --- a/.github/workflows/lint.yml >> +++ b/.github/workflows/lint.yml >> @@ -53,3 +53,17 @@ jobs: >> - name: test >> run: cmake --build . --target LuaJIT-luacheck >> working-directory: ${{ env.BUILDDIR }} >> + >> + checkpatch: >> + runs-on: [self-hosted, lightweight, Linux, x86_64] >> + >> + steps: >> + - uses: actions/checkout@v3 >> + with: >> + fetch-depth: 0 >> + # ref: ${{ github.event.pull_request.head.sha }} > Minor: Why do we need this comment? Forgot to remove, removed. > >> + submodules: recursive >> + - name: checkpatch >> + uses: ./.github/actions/checkpatch >> + with: >> + revision-range: HEAD~${{ github.event.pull_request.commits }}..HEAD > There is something wrong with this definition; in CI [1] it changes to the > following: > > | checkpatch/checkpatch.pl --codespell --color=always --show-types --git HEAD~..HEAD --ignore NO_CHANGELOG,NO_DOC,NO_TEST,COMMIT_LOG_LONG_LINE > > So, only the top commit is verified (instead of 2 -- this is why there > is a typo in the previous commit message). Fixed. Now checkpatch always checks a range tarantool/master..HEAD. > > Also, is it possible to do this activity on push instead, depending on > difference with tarantool/master HEAD? Yes, fixed. > > Also, how can we avoid some errors here, if this will show incorrect > spelling in the backported patches? Should we avoid these errors? If yes, then, I suppose, we can ignore them. >> -- >> 2.34.1 >> > [1]: https://github.com/tarantool/luajit/actions/runs/5519618350/jobs/10065116711#step:3:143
prev parent reply other threads:[~2023-07-17 13:22 UTC|newest] Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-07-11 16:51 [Tarantool-patches] [PATCH 0/2] Fix typos and " Sergey Bronnikov via Tarantool-patches 2023-07-11 16:51 ` [Tarantool-patches] [PATCH 1/2] test: fix typos Sergey Bronnikov via Tarantool-patches 2023-07-14 11:53 ` Sergey Kaplun via Tarantool-patches 2023-07-14 12:36 ` Sergey Bronnikov via Tarantool-patches 2023-07-11 16:51 ` [Tarantool-patches] [PATCH 2/2] ci: enable checkpatch Sergey Bronnikov via Tarantool-patches 2023-07-14 12:45 ` Sergey Kaplun via Tarantool-patches 2023-07-17 13:22 ` Sergey Bronnikov via Tarantool-patches [this message]
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=22b03068-25ce-53e9-f623-1eaa851e064a@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 2/2] ci: enable checkpatch' \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox