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 78602575CA7; Wed, 9 Aug 2023 17:55:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 78602575CA7 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1691592905; bh=xij1h5pZBITGhrxPjVAEhS6MIKl3yoWpS2N3XD///Ok=; 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=x87U/8/JYzlTtqumB5aSr8Ba/kOmJrZTtY+VaewMX6bVLN0rm5tghVHewvmjYanQQ XNus/s00MFXjTVLIP5tsnF24AwGEHDjIeObavyzORQ4NnNPPFOZb1oN+eyR69lX6IT kr4/NnepBWw8KiXu8VYq5M3Ks143XG+3kEHKTLlc= Received: from smtp63.i.mail.ru (smtp63.i.mail.ru [95.163.41.100]) (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 47D5B575CA7 for ; Wed, 9 Aug 2023 17:55:04 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 47D5B575CA7 Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1qTkaU-00CS3D-2H; Wed, 09 Aug 2023 17:55:03 +0300 Message-ID: Date: Wed, 9 Aug 2023 17:55:02 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.13.0 Content-Language: en-US To: Sergey Kaplun References: <1691091489.404344804@f148.i.mail.ru> <0b5bc26f-cf20-2b13-ce53-c1a5ebeabe5d@tarantool.org> 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: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC8BEB87106826C459512B2000DD660D84D182A05F5380850407008F1908A44AE0627105B040EEDD77F6D317E8F5D78DC5D6383A2F3ED00EC99 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE7D6964C9E324ABA58EA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006379763315F766189898638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D8A19C40D1ED96067306C66D1650FE5F27117882F4460429724CE54428C33FAD305F5C1EE8F4F765FC93A51A5089774A2DA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F446042972877693876707352033AC447995A7AD18CB629EEF1311BF91D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EECCD848CCB6FE560C1DBC1C451FC279AAD8FC6C240DEA76429C9F4D5AE37F343AA9539A8B242431040A6AB1C7CE11FEE3D2AC72D04CD5349BAD7EC71F1DB88427C4224003CC836476E2F48590F00D11D6E2021AF6380DFAD1A18204E546F3947CB11811A4A51E3B096D1867E19FE1407978DA827A17800CE7FCEEFFE83360B3FE2DBA43225CD8A89FB26E97DCB74E6252C6EABA9B74D0DA47B5C8C57E37DE458BEDA766A37F9254B7 X-C1DE0DAB: 0D63561A33F958A533A9C6D97B4E0AE4D24599D352B26099E4D9A925463ED545F87CCE6106E1FC07E67D4AC08A07B9B0F254576263B31EA9CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADE00A9FD3E00BEEDF3FED46C3ACD6F73ED3581295AF09D3DF87807E0823442EA2ED31085941D9CD0AF7F820E7B07EA4CF35196211FFD88FD2FE3EA1A154DCD725F75A89AD7FC70321F08469F273B88B908853F4E7B59623281529AA6C8ABD04CA4F66C2F40DFC6B81DFE330D932E51E2E461A413F07889F2102C26D483E81D6BE0DBAE6F56676BC7117BB6831D7356A2DEC5B5AD62611EEC62B5AFB4261A09AF0 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojUzxoxvtYX2qTNC9c/PNHsg== X-Mailru-Sender: 11C2EC085EDE56FAC07928AF2646A769E50F3AC134B879FA27105B040EEDD77F228AB1844EA588C7EBA65886582A37BD66FEC6BF5C9C28D98A98C1125256619760D574B6FC815AB872D6B4FCE48DF648AE208404248635DF 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 Bronnikov via Tarantool-patches Reply-To: Sergey Bronnikov 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! 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. > > >>> Patch introduces two new CMake targets: "LuaJIT-checkpatch", > Typo: s/Patch/The patch/ Fixed. > > > >>> 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. 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 , 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() > >