From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id BE750516A66; Mon, 17 Jul 2023 16:22:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org BE750516A66 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1689600176; bh=EsYtCWjXJntQTLb3EHYkGeRvOMsx6XI4gQA7YxY26aQ=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=hjIGkQgU8l3FupIS1w9hWLSU+I1ewg67z2Sv/6TRnE0j8ct6LXGi+UyipuKyznRmN cA4ExOHuu5vd42V7m64dIN+xfYQLa1TNpqNGURUPAirsx7oMnJnu3Y05/JURG8lhfu VwemmkSKk77XDQXuYtUfCbzDQNrpwcFYNfMwztJ8= Received: from smtp45.i.mail.ru (smtp45.i.mail.ru [95.163.41.83]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 5482F50D8D2 for ; Mon, 17 Jul 2023 16:22:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 5482F50D8D2 Received: by smtp45.i.mail.ru with esmtpa (envelope-from ) id 1qLOBj-009DKB-7I; Mon, 17 Jul 2023 16:22:55 +0300 Message-ID: <22b03068-25ce-53e9-f623-1eaa851e064a@tarantool.org> Date: Mon, 17 Jul 2023 16:22:54 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 To: Sergey Kaplun , Sergey Bronnikov References: Content-Language: en-US In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD9A075ED63965291975FF12524F3FDD82BF1CE581C66542C39182A05F5380850406AC7F3F9F042484DC0409A687920E6D3C0414D1428E7486B732C998FFA3DBAB9 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7CC84CC3AD347B910EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006374393CE590E20DA798638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8E6F9A6FAA650507D7E6D7B07D57C8A8A117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAA867293B0326636D2E47CDBA5A96583BD4B6F7A4D31EC0BC014FD901B82EE079FA2833FD35BB23D27C277FBC8AE2E8B2FD16FCC8DB5F8BEA471835C12D1D977C4224003CC836476EB9C4185024447017B076A6E789B0E975F5C1EE8F4F765FC1ECCCC3F2AC951FC3AA81AA40904B5D9CF19DD082D7633A0C84D3B47A649675F3AA81AA40904B5D98AA50765F79006374B64FF5809E74304D81D268191BDAD3D3666184CF4C3C14F3FC91FA280E0CE3D1A620F70A64A45A98AA50765F7900637F9425D8FA97DB4396D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637B8F435DEDE9E76EBEFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A571EE8ED6710DCAC2DE8CEC3199C22427806BE4EADB0A4C08F87CCE6106E1FC07E67D4AC08A07B9B01DAA61796BF5227BCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF3EBA2DF1BA496E1A35CD1606C0CABAA6759106A9F632276240011665C14C8C2E136FDBFCF0F91439DA6E887A44188BD69987C823BEC192F8D79094EB385D55D3A74DFFEFA5DC0E7F02C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojLxQD/rvAZi7m+mvc0HmDcQ== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7694B639D5831D89D45C0409A687920E6D3D776CA2DFBE07C79EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 2/2] ci: enable checkpatch X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Sergey Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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 >> >> 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: > * > * > * > * > * > * > * > * > * > 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