Feature/add gcov target #31

ClausKlein · 2024-11-24T17:13:00Z

Adds test coverage report feature.
Can be invoked with cmake process_coverage, the coverage report is generated in build/coverage/coverage.html.
Needs client to install gcovr.

NOTE: This is second of a series of PR based on #14

Depends on Feature/modernize cmake #33

see too bemanproject/optional#82

wusatosi

Thank you for splitting this change out as a separate PR.

Can you please keep the domain of a PR restricted to add gcov target.
No need to rush here.

Please include a description of the coverage feature in the PR, instead of:

Added VERIFY_INTERFACE_HEADER_SETS too.
(and fixed compiler errors found with this check)

NOTE: This is first of a series of PR based on https://github.com/bemanproject/inplace_vector/pull/14

Change to, e.g.

Adds test coverage report feature. 
Can be invoked with `cmake process_coverage`, the coverage report is generated in `build/coverage/coverage.html`.
Needs client to install `gcovr`.

Note: This is first of a series of PR based on #14 .

Request change:

remove unrelated CL to adding gcov target
Include a short description of this cmake target in README
Add license header for cmake/gcovr.cfg.in.

CMakeLists.txt

wusatosi · 2024-11-25T01:52:31Z

CMakeLists.txt

-cmake_minimum_required(VERSION 3.23)
+set(CMAKE_SKIP_TEST_ALL_DEPENDENCY FALSE)
+
+cmake_minimum_required(VERSION 3.25...3.31)


Please exclude changes unrelated to adding gcov target

.github/workflows/ci_tests.yml

wusatosi · 2024-11-25T01:53:36Z

CMakeLists.txt

+target_sources(
+    beman.inplace_vector
+    PUBLIC
+        FILE_SET inplace_vector_public_headers
+        TYPE HEADERS
+        BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/include
+        FILES
+            "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp"
+)
+set_target_properties(
+    beman.inplace_vector
+    PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON
+)
+target_compile_features(
    beman.inplace_vector
    INTERFACE
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-        $<INSTALL_INTERFACE:include>
+        "$<$<COMPILE_FEATURES:cxx_std_23>:cxx_std_23>"
+        "$<$<NOT:$<COMPILE_FEATURES:cxx_std_23>>:cxx_std_20>"
 )

+set(TARGET_PACKAGE_NAME ${PROJECT_NAME}-config)
+set(TARGETS_EXPORT_NAME ${PROJECT_NAME}-targets)
+set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})


Please exclude changes unrelated to adding gcov target

Please exclude

wusatosi · 2024-11-25T01:55:05Z

CMakeLists.txt

+if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
+    message(FATAL_ERROR "In-source builds are not allowed!")
+endif()


Please exclude changes unrelated to adding gcov target...

This is not done. Please exclude.

wusatosi · 2024-11-25T01:55:18Z

CMakeLists.txt

-    DESTINATION
-    ${CMAKE_INSTALL_LIBDIR}
-)
-
-# Install the header files to the appropriate destination
-install(
-    DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
-    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_PROJECT_NAME}
-    FILES_MATCHING
-    PATTERN
-    "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp"
+    FILE_SET inplace_vector_public_headers


Please exclude changes unrelated to adding gcov target

Please exclude

wusatosi · 2024-11-25T01:56:27Z

CMakeLists.txt

+configure_file(cmake/gcovr.cfg.in gcovr.cfg @ONLY)
+
+add_custom_target(
+    process_coverage


Can we test this in CI? Its fine if this doesn't have like a nice reporting interface, but can you add a CI target to ensure this cmake target doesn't fail?

Also can you update README regarding this target?

Can you add a CI job to test this.

cmake/gcovr.cfg.in

Use FILE_SET HEADERS for simplify installation Set VERIFY_INTERFACE_HEADER_SETS property Prevent use of include(CTest) Prepare CPack to easy distribution Add all_verify_interface_header_sets to CI builds Prevent in-source builds Ignore cmake configure trash

Co-authored-by: David Sankel <camior@gmail.com>

wusatosi

Please rebase and address previous concerns.

Edit: please stop falsely marking conversations as resolved

wusatosi · 2024-12-06T08:10:14Z

CMakeLists.txt

+if(${CMAKE_SOURCE_DIR} STREQUAL ${CMAKE_BINARY_DIR})
+    message(FATAL_ERROR "In-source builds are not allowed!")
+endif()


This is not done. Please exclude.

CMakeLists.txt

wusatosi · 2024-12-06T08:10:50Z

CMakeLists.txt

+    beman.inplace_vector
+    PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON
+)
+target_compile_features(


Please exclude unrelated changes

wusatosi · 2024-12-06T08:11:01Z

CMakeLists.txt

+target_sources(
+    beman.inplace_vector
+    PUBLIC
+        FILE_SET inplace_vector_public_headers
+        TYPE HEADERS
+        BASE_DIRS ${CMAKE_CURRENT_SOURCE_DIR}/include
+        FILES
+            "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp"
+)
+set_target_properties(
+    beman.inplace_vector
+    PROPERTIES VERIFY_INTERFACE_HEADER_SETS ON
+)
+target_compile_features(
    beman.inplace_vector
    INTERFACE
-        $<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/include>
-        $<INSTALL_INTERFACE:include>
+        "$<$<COMPILE_FEATURES:cxx_std_23>:cxx_std_23>"
+        "$<$<NOT:$<COMPILE_FEATURES:cxx_std_23>>:cxx_std_20>"
 )

+set(TARGET_PACKAGE_NAME ${PROJECT_NAME}-config)
+set(TARGETS_EXPORT_NAME ${PROJECT_NAME}-targets)
+set(INSTALL_CONFIGDIR ${CMAKE_INSTALL_LIBDIR}/cmake/${PROJECT_NAME})


Please exclude

wusatosi · 2024-12-06T08:11:11Z

CMakeLists.txt

-    DESTINATION
-    ${CMAKE_INSTALL_LIBDIR}
-)
-
-# Install the header files to the appropriate destination
-install(
-    DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR}/
-    DESTINATION ${CMAKE_INSTALL_INCLUDEDIR}/${CMAKE_PROJECT_NAME}
-    FILES_MATCHING
-    PATTERN
-    "${CMAKE_CURRENT_SOURCE_DIR}/include/beman/inplace_vector/inplace_vector.hpp"
+    FILE_SET inplace_vector_public_headers


Please exclude

wusatosi · 2024-12-06T08:11:39Z

CMakeLists.txt

+configure_file(cmake/gcovr.cfg.in gcovr.cfg @ONLY)
+
+add_custom_target(
+    process_coverage


Can you add a CI job to test this.

wusatosi · 2024-12-06T08:12:03Z

cmake/gcovr.cfg.in

+cobertura = @CMAKE_BINARY_DIR@/coverage/cobertura.xml
+sonarqube = @CMAKE_BINARY_DIR@/coverage/sonarqube.xml
+html-details = @CMAKE_BINARY_DIR@/coverage/coverage.html
+# XXX gcov-executable = @GCOV_EXECUTABLE@


What is this comment for

This is based on an other beman project, but this use presets ...

No I am asking what is this line of comment for

ClausKlein · 2024-12-11T16:28:37Z

This waste my time

ClausKlein requested review from camio, Hels15, DeveloperPaul123 and wusatosi as code owners November 24, 2024 17:13

ClausKlein mentioned this pull request Nov 24, 2024

Feature/install cmake config package #32

Closed

2 tasks

wusatosi requested changes Nov 25, 2024

View reviewed changes

ClausKlein marked this pull request as draft November 25, 2024 08:24

ClausKlein force-pushed the feature/add_gcov_target branch 2 times, most recently from aab71ef to 043fd7b Compare November 27, 2024 06:42

ClausKlein and others added 3 commits December 6, 2024 07:56

Modernize cmake files

39d540b

Use FILE_SET HEADERS for simplify installation Set VERIFY_INTERFACE_HEADER_SETS property Prevent use of include(CTest) Prepare CPack to easy distribution Add all_verify_interface_header_sets to CI builds Prevent in-source builds Ignore cmake configure trash

Update CMakeLists.txt

d744137

Co-authored-by: David Sankel <camior@gmail.com>

Rebased and minor fixes after review

d462d55

ClausKlein marked this pull request as ready for review December 6, 2024 07:59

wusatosi requested changes Dec 6, 2024

View reviewed changes

ClausKlein added 2 commits December 6, 2024 09:17

Prepare gcov processeing

5ac8f1c

Add missing license header to gcovr.cfg file

50f1556

ClausKlein force-pushed the feature/add_gcov_target branch from 043fd7b to 50f1556 Compare December 6, 2024 08:18

ClausKlein marked this pull request as draft December 6, 2024 08:23

Remove unneded line

3b44eda

ClausKlein requested a review from wusatosi December 6, 2024 10:23

ClausKlein closed this Dec 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/add gcov target #31

Feature/add gcov target #31

ClausKlein commented Nov 24, 2024 •

edited

Loading

wusatosi left a comment •

edited

Loading

wusatosi Nov 25, 2024

ClausKlein Nov 25, 2024

wusatosi Nov 25, 2024

ClausKlein Nov 25, 2024

wusatosi Dec 6, 2024

wusatosi Nov 25, 2024

ClausKlein Nov 25, 2024

wusatosi Dec 6, 2024

wusatosi Nov 25, 2024

ClausKlein Nov 25, 2024

wusatosi Dec 6, 2024

wusatosi Nov 25, 2024

ClausKlein Nov 25, 2024

wusatosi Dec 6, 2024

wusatosi left a comment •

edited

Loading

wusatosi Dec 6, 2024

wusatosi Dec 6, 2024

wusatosi Dec 6, 2024

wusatosi Dec 6, 2024

wusatosi Dec 6, 2024

wusatosi Dec 6, 2024

ClausKlein Dec 6, 2024

wusatosi Dec 6, 2024

ClausKlein commented Dec 11, 2024

Feature/add gcov target #31

Feature/add gcov target #31

Conversation

ClausKlein commented Nov 24, 2024 • edited Loading

wusatosi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wusatosi left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ClausKlein commented Dec 11, 2024

ClausKlein commented Nov 24, 2024 •

edited

Loading

wusatosi left a comment •

edited

Loading

wusatosi left a comment •

edited

Loading