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 B2C175C3282; Tue, 15 Aug 2023 11:41:11 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org B2C175C3282 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1692088871; bh=O7+vsk/QyJiQ+yGI3ygMqmtI7jO8sJbq8sY+v+nwBnA=; 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=M7HaqpjDn8+aKhJaPZeVF+CUhvjrKLNik0VG5Kl8wiBiyYsQEAuBcdXAlZzzDw1wy 6BYNFM3jP6mGE7GXTrW2BJqHQs5S0/FjBplf53Fa5S+lXJPNGLGwCVlYYZ/nTAIRK/ jogJk/D4wd+bIu+/H83+Y5E41Z0UJ2s4XbLmso3s= Received: from smtp29.i.mail.ru (smtp29.i.mail.ru [95.163.41.68]) (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 3DF685602C1 for ; Tue, 15 Aug 2023 11:41:10 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 3DF685602C1 Received: by smtp29.i.mail.ru with esmtpa (envelope-from ) id 1qVpbx-002WI7-0J; Tue, 15 Aug 2023 11:41:09 +0300 Date: Tue, 15 Aug 2023 11:41:08 +0300 To: Sergey Bronnikov Message-ID: References: <27qwncp6oubrmexp6bwsur5nnm5xnlxaby3slxk4rrk2beyuin@5wtqxgcetnfk> <63fb786f-b593-6da6-78b6-cfd9b2435740@tarantool.org> <71807b71-5445-1ce9-f073-a708f87eb29b@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <71807b71-5445-1ce9-f073-a708f87eb29b@tarantool.org> X-Mailru-Src: smtp X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC868303E4FA23A046C075EC7AC197E9C0B182A05F538085040A347228405F33D6111BB6AF64865CA412D6F7CA856DD298F5BD23F82A0689FF8 X-C1DE0DAB: 0D63561A33F958A533DAD2BF18464BCE5C80C1C64938B39F959FF6914E02ADEEF87CCE6106E1FC07E67D4AC08A07B9B0CF7CD7A0D5AA5F25CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0AD5177F0B940C8B66ECE892A7B2722663E91682638B966EB3F662256BEEFA9527F9B03B23AFD56191FF4CD833779771391E16987FE375F22F8E292F72CDA22A252EF8867A193A2F79E37FD76D11AF80A0DD773366A44F7A1DDC788B7AAB4E6E88AEA455F16B58544A2D06CB91D864A7BD2965026E5D17F6739C77C69D99B9914278E50E1F0597A6FD5CD72808BE417F3B9E0E7457915DAA85F X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojJ1ceUZTkowkGzsrm/Vk6Dg== X-Mailru-Sender: 11C2EC085EDE56FA38FD4C59F7EFE40744F3C78B7DB078B62EBCCA9992A5548E06F365114516B2B6D51284F0FE6F529ABC7555A253F5B200DF104D74F62EE79D27EC13EC74F6107F4198E0F3ECE9B5443453F38A29522196 X-Mras: OK Subject: Re: [Tarantool-patches] [PATCH 1/2 v2] cmake: add code coverage support 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! Thanks for the fixes! LGTM, except for a single comment below. On Mon, Aug 07, 2023 at 04:39:27PM +0300, Sergey Bronnikov wrote: > Hello, Sergey! > > > On 8/6/23 14:35, Sergey Kaplun wrote: > > Hi, Sergey! > > Thanks for the patch! > > LGTM, just a minor nits below. > > > > On 02.08.23, Sergey Bronnikov via Tarantool-patches wrote: > > > Hi, Max > > > > > > On 8/2/23 11:06, Maxim Kokryashkin via Tarantool-patches wrote: > > > > Hi, Sergey! > > > > Thanks for the fixes! > > > > LGTM, except for a few comments below. > > > > > > > > Side note: I see that coverage job in CI is red. Why is that > > > > happening? > > > This happened because from time to time total code coverage number > > > changes a bit. > > > > > > It is really annoying, to solve this we need to increase the threshold > > > in Coveralls service. > > I see that now this job is green. Was it fixed? > Actually no. I'll ask someone who has access to settings to increase > threshold. > > > > > > > > > > > > On Tue, Aug 01, 2023 at 09:46:08PM +0300, Sergey Bronnikov via Tarantool-patches wrote: > > > > > From: Sergey Bronnikov > > > > > > > > > > The patch adds building code coverage report using gcovr [1] and gcov. > > > > > gcovr is a better version of lcov, see [2]. There were two new CMake > > > > > targets added: LuaJIT-coverage proccess *.gcno and *.gcda files with > > > > Typo: s/process/processes/ > > > Fixed. > > > > > gcov, builds a detailed HTML report and prints a summary, target > > > > > coverage executes LuaJIT-tests and then runs LuaJIT-coverage. Target > > > > > LuaJIT-coverage is useful for building code coverage report for a custom > > > > > set of regression tests. > > > > > > > > > > ``` > > > > > $ cmake -S . -B build -DENABLE_COVERAGE=ON > > > > > $ cmake --build build --parallel --target coverage > > > > > > > > > > > > > > > > > > > > lines: 84.1% (26056 out of 30997) > > > > > functions: 88.8% (2055 out of 2314) > > > > > branches: 71.5% (14801 out of 20703) > > > > > ``` > > > > > > > > > > 1. https://gcovr.com/ > > > > > 2. https://gcovr.com/en/stable/faq.html#what-is-the-difference-between-lcov-and-gcovr > > > > > --- > > > > > CMakeLists.txt | 9 ++++++ > > > > > cmake/CodeCoverage.cmake | 45 +++++++++++++++++++++++++++ > > > > > test/CMakeLists.txt | 7 +++++ > > > > > test/tarantool-c-tests/CMakeLists.txt | 6 +++- > > > > > 4 files changed, 66 insertions(+), 1 deletion(-) > > > > > create mode 100644 cmake/CodeCoverage.cmake > > > > > > > > > > diff --git a/CMakeLists.txt b/CMakeLists.txt > > > > > index 6ef24bba..fe6582fa 100644 > > > > > --- a/CMakeLists.txt > > > > > +++ b/CMakeLists.txt > > > > > @@ -116,6 +116,15 @@ if(LUAJIT_ENABLE_WARNINGS) > > > > > ) > > > > > endif() > > I suggest to add comment here, that the user should run tests _before_ > > coverage report, or this may be confusing (yes, I'm this user :)): > > > > | $ make LuaJIT-coverage > > | Building coverage report > > | lines: 0.0% (0 out of 23883) > > | functions: 0.0% (0 out of 1765) > > | branches: 0.0% (0 out of 17131) > > | Built target LuaJIT-coverage > > The difference for LuaJIT-coverage and coverage targets is described in > commit message. > > Comment is already there: > > >   add_custom_command(TARGET ${PROJECT_NAME}-coverage > >    COMMENT "Building coverage report" > > > > > > > > > +set(LUAJIT_ENABLE_COVERAGE_DEFAULT OFF) > > > > > +option(LUAJIT_ENABLE_COVERAGE > > > > > + "Enable integration with gcovr, a code coverage program" > > > > > + ${LUAJIT_ENABLE_COVERAGE_DEFAULT}) > > > > > +if (LUAJIT_ENABLE_COVERAGE) > > > > > + AppendFlags(CMAKE_C_FLAGS --coverage) > > > > > + include(CodeCoverage) > > > > > +endif(LUAJIT_ENABLE_COVERAGE) I believe it would be better to do that in the `test/CMakeLists.txt` instead of the main one, since coverage is semantically relevant to tests. Feel free to ignore. > > > > > + > > > > > # Auxiliary flags for main targets (libraries, binaries). > > > > > AppendFlags(TARGET_C_FLAGS > > > > > -D_FILE_OFFSET_BITS=64 > > > > > diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake > > > > > new file mode 100644 > > > > > index 00000000..2be7d129 > > > > > --- /dev/null > > > > > +++ b/cmake/CodeCoverage.cmake > > > > > @@ -0,0 +1,45 @@ > > > > > +find_program(GCOVR gcovr) > > > > > +find_program(GCOV gcov) > > > > > + > > > > > +set(COVERAGE_DIR "${PROJECT_BINARY_DIR}/coverage") > > > > > +set(COVERAGE_HTML_REPORT "${COVERAGE_DIR}/luajit.html") > > > > > +set(COVERAGE_XML_REPORT "${COVERAGE_DIR}/luajit.xml") > > > > > + > > > > > +if(NOT GCOVR OR NOT GCOV) > > > > > + add_custom_target(${PROJECT_NAME}-coverage > > > > > + COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target" > > I suggest to split this line into several too. > > splitted > > > > > > > > > + ) > > > > > + message(WARNING "Either `gcovr' or `gcov` not found, \ > > > > > +so ${PROJECT_NAME}-coverage target is dummy") > > > > Nit: Something is wrong with alignment here. > > > No, it is intentionally. If you add indentation then these whitespaces > > > will be added to a message. > > Works just fine with the following diff for me: > > > > =================================================================== > > diff --git a/cmake/CodeCoverage.cmake b/cmake/CodeCoverage.cmake > > index 2be7d129..83e23d7f 100644 > > --- a/cmake/CodeCoverage.cmake > > +++ b/cmake/CodeCoverage.cmake > > @@ -9,8 +9,8 @@ if(NOT GCOVR OR NOT GCOV) > > add_custom_target(${PROJECT_NAME}-coverage > > COMMAND ${CMAKE_COMMAND} -E cmake_echo_color --red "LuaJIT-coverage is a dummy target" > > ) > > - message(WARNING "Either `gcovr' or `gcov` not found, \ > > -so ${PROJECT_NAME}-coverage target is dummy") > > + message(WARNING "Either `gcovr' or `gcov` not found, " > > + "so ${PROJECT_NAME}-coverage target is dummy") > > return() > > endif() > > =================================================================== > > Applied, thanks! > > > > > > > > > > > # Exclude DynASM files, that contain a low-level VM code for CPUs. > > > --exclude ".*\.dasc" > > > # Exclude buildvm source code, it's the project's infrastructure. > > > --exclude ".*/host/" > > Why don't use ${PROJECT_SOURCE_DIR} instead of .* here? > > > It is not needed here. gcovr searches *.gcda/*.gcno files in > PROJECT_BINARY_DIRECTORY > > and additionally all paths excluded except PROJECT_SOURCE_DIR/src. So > absolute path is excessive in regexes specified in --exclude options. > > > > > > > > > >