From: Sergey Bronnikov via Tarantool-patches <tarantool-patches@dev.tarantool.org> To: Sergey Kaplun <skaplun@tarantool.org> Cc: Sergey Bronnikov <estetus@gmail.com>, max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Subject: Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets Date: Wed, 9 Aug 2023 17:55:02 +0300 [thread overview] Message-ID: <e1fcd295-df3c-0bc3-ced9-70e1223e0602@tarantool.org> (raw) In-Reply-To: <ZNOc6g21RTjhL58o@root> Hi, Sergey! On 8/9/23 17:04, Sergey Kaplun wrote: > Hi, Sergey! > Thanks for the patch! > > It's really amazing changes: I've already found some typos in my > commit messages and comments!:) > > Please, consider my comments below. > > On 04.08.23, Sergey Bronnikov via Tarantool-patches wrote: >> Hi, Max >> >> On 8/3/23 22:38, Maxim Kokryashkin via Tarantool-patches wrote: >>> Hi, Sergey! >>> Please consider my comments below. > <snipped> > >>> Patch introduces two new CMake targets: "LuaJIT-checkpatch", > Typo: s/Patch/The patch/ Fixed. > > <snipped> > >>> I suggest doing the same as with coverage — move that logic into a >>> separate module, >>> so the test/CMakeLists.txt remains readable and clean. >>> >> Done, force-pushed. > <snipped> > >> diff --git a/CMakeLists.txt b/CMakeLists.txt >> index 0204e852..b1a442b7 100644 >> --- a/CMakeLists.txt >> +++ b/CMakeLists.txt >> @@ -29,9 +29,12 @@ endif() >> >> set(CMAKE_MODULE_PATH "${CMAKE_CURRENT_SOURCE_DIR}/cmake") >> >> +set(GIT_MASTER_BRANCH "tarantool/master") >> + > I suppose this should be moved to <cmake/CheckPatch.cmake> too. No, this var is required for gcovr too and will be used by it in patches with coverage support. > >> include(LuaJITUtils) >> include(SetBuildParallelLevel) >> include(SetVersion) >> +include(CheckPatch) > And be included from test/CMakeLists.txt, not from root CMakeLists.txt. Why? How checkpatch is related to testing? > >> # --- Variables to be exported to child scopes >> --------------------------------- >> >> diff --git a/cmake/CheckPatch.cmake b/cmake/CheckPatch.cmake >> new file mode 100644 >> index 00000000..243ee426 >> --- /dev/null >> +++ b/cmake/CheckPatch.cmake >> @@ -0,0 +1,46 @@ >> +find_program(CHECKPATCH checkpatch.pl >> + HINTS ${PROJECT_SOURCE_DIR}/checkpatch) >> +add_custom_target(${PROJECT_NAME}-checkpatch) >> +if(CHECKPATCH) >> + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch >> + COMMENT "Running checkpatch" >> + COMMAND >> + ${CHECKPATCH} >> + # Description of supported checks in >> + # >> https://github.com/tarantool/checkpatch/blob/master/doc/checkpatch.rst >> + --codespell >> + --color=always >> + --git ${GIT_MASTER_BRANCH}..HEAD >> + --show-types >> + --ignore BAD_SIGN_OFF >> + --ignore BLOCK_COMMENT_STYLE >> + --ignore CODE_INDENT >> + --ignore COMMIT_LOG_LONG_LINE >> + # Requires at least two lines in commit message and this > Typo: s/in commit message/in the commit message/ > Typo: s/ and/, and/ Fixed. >> + # is annoying. > But we always have at least two lines, don't we? Sometimes no. See commit "codehealth: fix typos" in patch series. > >> + --ignore COMMIT_MESSAGE >> + --ignore CONSTANT_COMPARISON >> + --ignore FUNCTION_NAME_NO_NEWLINE >> + --ignore GIT_COMMIT_ID >> + --ignore INCLUDE_GUARD >> + --ignore LOGICAL_CONTINUATIONS >> + --ignore LONG_LINE >> + --ignore NO_CHANGELOG >> + --ignore NO_DOC >> + --ignore NO_TEST >> + --ignore PREFER_DEFINED_ATTRIBUTE_MACRO >> + --ignore SPACING >> + --ignore SUSPECT_CODE_INDENT >> + --ignore TABSTOP >> + --ignore TRAILING_STATEMENTS >> + --ignore UNCOMMENTED_DEFINITION >> + --ignore UNSAFE_FUNCTION > I suppose we should add `--ignore PREFER_FALLTHROUGH`, too. > | ERROR:PREFER_FALLTHROUGH: Prefer 'FALLTHROUGH;' over fallthrough comment > > Since it's already used in the codebase: > | $ grep fallthrough -R . | wc -l > | 47 Added. > >> + WORKING_DIRECTORY ${PROJECT_SOURCE_DIR} > Also, we can't just check all patches with this since it will have > TONS of errors. We must at least exclude all <src/*>, <dynasm/*> and > include back files that are maintained by us. > > Without this, it becomes pointless, since we either > * will have red always checkpatch job and don't be aware, so can miss > some errors, either > * will set ignore all (or something like it) to ignore all checkpatch > warnings, including meaningful one. Agree, but this requires patching checkpatch itself, right now it is not possible to suppress warnings for a certain files in patch. I'll try to implement such patch. >> + ) >> +else() >> + set(MSG "`checkpatch.pl' is not found, so ${PROJECT_NAME}-checkpatch >> target is dummy") > Please, split this line into several. > > Minor: I suggest to rename it to the WARN_MSG. > Feel free to ignore. Updated. >> + add_custom_command(TARGET ${PROJECT_NAME}-checkpatch >> + COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG} >> + COMMENT ${MSG} >> + ) >> +endif() > <snipped> >
next prev parent reply other threads:[~2023-08-09 14:55 UTC|newest] Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-08-02 8:52 [Tarantool-patches] [PATCH 0/5][v3] Fix typos and enable checkpatch Sergey Bronnikov via Tarantool-patches 2023-08-02 8:52 ` [Tarantool-patches] [PATCH 1/5][v3] ci: fix a step name Sergey Bronnikov via Tarantool-patches 2023-08-03 19:27 ` Maxim Kokryashkin via Tarantool-patches 2023-08-09 11:20 ` Sergey Kaplun via Tarantool-patches 2023-08-02 8:52 ` [Tarantool-patches] [PATCH 2/5][v3] codehealth: fix typos Sergey Bronnikov via Tarantool-patches 2023-08-03 19:29 ` Maxim Kokryashkin via Tarantool-patches 2023-08-09 12:58 ` Sergey Kaplun via Tarantool-patches 2023-08-09 14:41 ` Sergey Bronnikov via Tarantool-patches 2023-08-02 8:52 ` [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets Sergey Bronnikov via Tarantool-patches 2023-08-03 19:38 ` Maxim Kokryashkin via Tarantool-patches 2023-08-04 10:56 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 14:04 ` Sergey Kaplun via Tarantool-patches 2023-08-09 14:55 ` Sergey Bronnikov via Tarantool-patches [this message] 2023-08-09 15:45 ` Sergey Kaplun via Tarantool-patches 2023-08-15 8:57 ` Maxim Kokryashkin via Tarantool-patches 2023-08-02 8:52 ` [Tarantool-patches] [PATCH 4/5][v3] ci: enable checkpatch Sergey Bronnikov via Tarantool-patches 2023-08-03 19:38 ` Maxim Kokryashkin via Tarantool-patches 2023-08-09 14:40 ` Sergey Kaplun via Tarantool-patches 2023-08-09 15:05 ` Sergey Bronnikov via Tarantool-patches 2023-08-14 9:22 ` Sergey Bronnikov via Tarantool-patches 2023-08-02 8:52 ` [Tarantool-patches] [PATCH 5/5][v3] test: fix codestyle Sergey Bronnikov via Tarantool-patches 2023-08-03 19:45 ` Maxim Kokryashkin via Tarantool-patches 2023-08-04 10:42 ` Sergey Bronnikov via Tarantool-patches 2023-08-09 14:07 ` Sergey Kaplun via Tarantool-patches
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=e1fcd295-df3c-0bc3-ced9-70e1223e0602@tarantool.org \ --to=tarantool-patches@dev.tarantool.org \ --cc=estetus@gmail.com \ --cc=max.kokryashkin@gmail.com \ --cc=sergeyb@tarantool.org \ --cc=skaplun@tarantool.org \ --subject='Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets' \ /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