Skip to content
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.

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

Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode #2739

Merged
merged 2 commits into from
Sep 25, 2023

Conversation

hkaelber
Copy link
Contributor

Description

Support CMake multi-config generators with PRE_TEST discovery mode.
Cf. #2670 (comment)

GitHub Issues

@codecov
Copy link

codecov bot commented Aug 27, 2023

Codecov Report

Merging #2739 (9b930ec) into devel (85eb465) will increase coverage by 0.03%.
Report is 15 commits behind head on devel.
The diff coverage is n/a.

@@            Coverage Diff             @@
##            devel    #2739      +/-   ##
==========================================
+ Coverage   91.32%   91.36%   +0.03%     
==========================================
  Files         192      190       -2     
  Lines        7850     7855       +5     
==========================================
+ Hits         7169     7176       +7     
+ Misses        681      679       -2     

@horenmar
Copy link
Member

How far along is this?

@horenmar horenmar added the Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration. label Sep 23, 2023
@hkaelber hkaelber changed the title [WIP]Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode Catch.cmake: Support CMake multi-config with PRE_TEST discovery mode Sep 24, 2023
@hkaelber hkaelber marked this pull request as ready for review September 24, 2023 08:37
@hkaelber
Copy link
Contributor Author

How far along is this?

@horenmar, IMHO it's ready for review. I was hesitating, because of missing experience with the usage of multi-config targets. After re-reading the docs, made it finally work correctly. Could you pls review?

extras/Catch.cmake Outdated Show resolved Hide resolved
Copy link
Member

@horenmar horenmar left a comment

Choose a reason for hiding this comment

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

I tested this with MSVC and it seems to work.

@hkaelber hkaelber force-pushed the hk/support-cmake-multi branch from 630c7f0 to 9b930ec Compare September 25, 2023 14:32
Comment on lines +267 to +271
"if(NOT CTEST_CONFIGURATION_TYPE)" "\n"
" message(\"No configuration for testing specified, use '-C <cfg>'.\")" "\n"
"else()" "\n"
" include(\"${ctest_file_base}_include-\${CTEST_CONFIGURATION_TYPE}.cmake\")" "\n"
"endif()" "\n"
Copy link
Member

Choose a reason for hiding this comment

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

This is fine to merge already, but I still have a question: is there a reason to do this like this (outside of consistency with the surrounding code), rather than either making the newlines part of the line-strings, or using multiline strings?

IIRC something like this would work as well and be simpler to read

set(ctest_include_multi_content [[
if(NOT CTEST_CONFIGURATION_TYPE)
  message("No configuration for testing specified, use '-C <cfg>'.")
else()
  include("C:/ubuntu/temp/discover-tests-tests/msvc-sln-1/tests-b12d07c_include-${CTEST_CONFIGURATION_TYPE}.cmake")
endif()
]])

Copy link
Member

Choose a reason for hiding this comment

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

Right, the issue is that we want to interpret some of the variables, while [[]] does not do any interpretation. nevermind then

@horenmar horenmar merged commit 7b79331 into catchorg:devel Sep 25, 2023
73 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Extras Touches utility scripts outside of Catch2 proper, e.g. CMake integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants