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 C4935575CA1; Wed, 9 Aug 2023 17:09:16 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org C4935575CA1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1691590156; bh=bhAs6VjcK9h9zwwSs474lLErn7KppZx6VT6pfKq5+xA=; 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=qOgMRlSlR/VIReW0JakeLyQNSLz3u9+jSQ+VcwQRZ6JVlCJfZK/yILfz3Hwo654qj fwrkUFRXXRuhII0YiBvIHp54EhI1Tjy9jj8sj1rZTY1xTnSOeiNse16XlvI+D9G64s jjDTtJc3YhHmV+bpyIQSsNJCRpFm6Skxp9MK1koA= Received: from smtp31.i.mail.ru (smtp31.i.mail.ru [95.163.41.72]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id 511D456CD18 for ; Wed, 9 Aug 2023 17:09:15 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 511D456CD18 Received: by smtp31.i.mail.ru with esmtpa (envelope-from ) id 1qTjs9-00AXlr-26; Wed, 09 Aug 2023 17:09:14 +0300 Date: Wed, 9 Aug 2023 17:04:26 +0300 To: Sergey Bronnikov Message-ID: References: <1691091489.404344804@f148.i.mail.ru> <0b5bc26f-cf20-2b13-ce53-c1a5ebeabe5d@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <0b5bc26f-cf20-2b13-ce53-c1a5ebeabe5d@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC804E38A5F9341E5D89B81E0241E25E490182A05F538085040F3D2057B87B74FF19038BAC7A2C3F6FBC5C1797901FA280218C29AE64C348FD2 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7B9FBA884A7C9B8BAEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F7900637C6EF84A9D301917D8638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8FED9874F23964E5668F3021AA967BB86117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCC0EC8C44E4C1BEE2A471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD182CC0D3CB04F14752D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE437C869540D2AB0FE2071C6999E77799D8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3DA532D2019E286A7AD7EC71F1DB88427C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407959CC434672EE6371089D37D7C0E48F6C8AA50765F7900637C970FD8DF19C51D2EFF80C71ABB335746BA297DBC24807EABDAD6C7F3747799A X-C1DE0DAB: 0D63561A33F958A5401E46751AC6FA3813575C575BD3EF68411A8D1EFCC30483F87CCE6106E1FC07E67D4AC08A07B9B0AD0E433DBF1FBFA3CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF9CA5E3F863961C8A14E94A6BB73922015AB92BEB32AA88303D875C7604F59D74D2D1C8B3CBCE0EE81529AA6C8ABD04CABB2304F3A2932DEA895B98B85766E2F0461A413F07889F2102C26D483E81D6BE5EF9655DD6DEA7D65774BB76CC95456EEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUzxoxvtYX2opqTrGsDzglA== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A7694350D5E08CE013B39038BAC7A2C3F6FB81AE5054A97140B9DEDBA653FF35249392D99EB8CC7091A70E183A470755BFD208F19895AA18418972D6B4FCE48DF648AE208404248635DF X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH 3/5][v3] cmake: introduce new targets 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 Kaplun via Tarantool-patches Reply-To: Sergey Kaplun Cc: Sergey Bronnikov , max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" 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. > > > > Patch introduces two new CMake targets: "LuaJIT-checkpatch", Typo: s/Patch/The patch/ > > > > 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. > 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 too. >  include(LuaJITUtils) >  include(SetBuildParallelLevel) >  include(SetVersion) > +include(CheckPatch) And be included from test/CMakeLists.txt, not from root CMakeLists.txt. > >  # --- 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/ > +        # is annoying. But we always have at least two lines, don't we? > +        --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 > +    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 , 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. > +  ) > +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. > +  add_custom_command(TARGET ${PROJECT_NAME}-checkpatch > +    COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red ${MSG} > +    COMMENT ${MSG} > +  ) > +endif() > > -- Best regards, Sergey Kaplun