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 911B95C3298; Tue, 15 Aug 2023 11:57:58 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 911B95C3298 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692089878; bh=dnYXs7MMUrxxB9hPBi1vcMUu4ZBqLMAiingIIJz3xbk=; 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=mYLeoz30vdM3r8X7UeLwCcQpRFX/HDD60KFiT/gPrN/VVOXGQyJ1dAmoEPE06OV2E AwFJKbSug8nzzDGcV5ZzdLhXGylRopXaUJRu0vaXfAf2c1kp2frh5HmeyBicMoRIx5 cipiACJCbw4wohp6UOBltC+Rg9sRMEN5REemMyNI= 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 EC82B5AA586 for ; Tue, 15 Aug 2023 11:57:56 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org EC82B5AA586 Received: by smtp63.i.mail.ru with esmtpa (envelope-from ) id 1qVpsB-0038gn-1s; Tue, 15 Aug 2023 11:57:55 +0300 Date: Tue, 15 Aug 2023 11:57:55 +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: X-Mailru-Src: smtp X-7564579A: B8F34718100C35BD X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC86EE92E42F0C271DDDF6A6B73F41FC074182A05F538085040E3DC91E57404B5D2B40EECAA179E6C8D2B4A1AE09D896F7434CF8774D0EABA66 X-C1DE0DAB: 0D63561A33F958A5B06A735BF4E6ED877712659E38DA7223F03BE6AF0DAC99A5F87CCE6106E1FC07E67D4AC08A07B9B04E7D9683544204AFCB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527FAEDEC7F6F5477E8AD99ED55C20108713E01FC33F1F2547E7F58D77320B493523AC6C77D7EA284A7537FD76D11AF80A0D1791D512A6D5372E748C80A9A47E73A6EA455F16B58544A2D06CB91D864A7BD2965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJ1ceUZTkown7auytyumDfw== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE4075CF6B45D6AA59F8CE2BC3F3898E009155661F96F1BDCAECDD51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 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: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: max.kokryashkin@gmail.com, tarantool-patches@dev.tarantool.org, Sergey Bronnikov Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "Tarantool-patches" Hi, Sergey! LGTM, but I am really concerned about that `src/*` supression problem. Looking forward to see the patch with supression in this series. On Wed, Aug 09, 2023 at 05:55:02PM +0300, Sergey Bronnikov wrote: > 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? Agree with Sergey Bronnikov here. > > > > >  # --- 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() > > Best regards, Maxim Kokryashkin > >