Skip to content

added remaining make targets to CMake#4026

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
firewave wants to merge 3 commits into from
Closed

added remaining make targets to CMake #4026

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9c7fcc0
renamed CMake target `check` to `check-ctest`
firewave Apr 12, 2022
abb9efc
added make targets `check` and `test` to CMake
firewave Apr 12, 2022
4ff8a3e
disabled CMake target `test` as the name is reserved by CTest [skip ci]
firewave Apr 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 9 additions & 2 deletions test/CMakeLists.txt
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -59,6 +59,13 @@ if (BUILD_TESTS)
DEPENDS cppcheck validateCFG)
endif()

# TODO: the target name "test" is reservered by CTest
#add_custom_target(test $<TARGET_FILE:testrunner>
# DEPENDS cppcheck testrunner)

add_custom_target(check $<TARGET_FILE:testrunner> -q

@pfultz2pfultz2Apr 15, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont understand the purpose of this, as ctest will already run the tests in the testrunner and it will run faster. Can we name this run-testrunner instead?

@firewavefirewaveApr 15, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I want to align Makefile with CMake so there's an easy transition when moving to CMake. The Makefile should be completely deprecated or stripped down a bit.

There's lots of more things in CMake which can be simplified and consolidated.

@pfultz2pfultz2Apr 21, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First I want to align Makefile with CMake so there's an easy transition when moving to CMake.

I think only @danmar uses the Makefile(and he seems more focused on cppcheck premium recently too). As far as I know, most other developers and contributors use cmake.

We can definitely provide targets to do the same functionality as in the makefile, but I dont see the need to name them exactly the same, especially when the name already overlaps with targets developers are already using in cmake.

DEPENDS cppcheck testrunner)

if (REGISTER_TESTS)
# CMP0064 requires 3.4
# CMAKE_MATCH_<n> usage for if (MATCHES) requires 3.9
Expand All@@ -71,7 +78,7 @@ if (BUILD_TESTS)
ProcessorCount(N)
set(CTEST_PARALLEL_LEVEL ${N} CACHE STRING "CTest parallel level")
set(CTEST_TIMEOUT 90 CACHE STRING "CTest timeout")
add_custom_target(check ${CMAKE_CTEST_COMMAND} --output-on-failure -j ${CTEST_PARALLEL_LEVEL} -C ${CMAKE_CFG_INTDIR} --timeout ${CTEST_TIMEOUT}
add_custom_target(check-ctest ${CMAKE_CTEST_COMMAND} --output-on-failure -j ${CTEST_PARALLEL_LEVEL} -C ${CMAKE_CFG_INTDIR} --timeout ${CTEST_TIMEOUT}

@pfultz2pfultz2Apr 15, 2022

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to have check run ctest as currently does. I dont see the purpose of renaming this target.

@firewavefirewaveApr 15, 2022

Copy link
Copy Markdown
CollaboratorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is to align it with the Makefile for now. This target (among others) will be deprecated since it is totally unnecessary since you should just use the ctest command.

@pfultz2pfultz2Apr 21, 2022•
edited
Loading

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most developers(including me) use make check to run the tests with ctest. I really dont want to see it change to something that is suboptimal.

Also, its shorter to do make -j $(nproc) check than doing make -j $(nproc) cppcheck testrunner && ctest --output-on-failure -j $(nproc) ., so this shouldn't be removed either.

DEPENDS testrunner cppcheck)

set(SKIP_TESTS "" CACHE STRING "A list of tests to skip")
Expand All@@ -80,7 +87,7 @@ if (BUILD_TESTS)
if (${NAME} IN_LIST SKIP_TESTS)
elseif(TEST ${NAME})
else()
add_test(NAME ${NAME} COMMAND testrunner ${NAME} WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY})
add_test(NAME ${NAME} COMMAND $<TARGET_FILE:testrunner> ${NAME} WORKING_DIRECTORY ${CMAKE_RUNTIME_OUTPUT_DIRECTORY})
endif()
endfunction()

Expand Down