Update cmake scripts and add them to github actions #1302
Conversation
|
Cool PR, I review later |
test/CMakeLists.txt
Outdated
| .) | ||
|
|
||
| # To compile with C++14 | ||
| target_compile_features(${unit_test_name} PRIVATE cxx_std_14) | ||
|
|
||
| # To be able to run ctest | ||
| add_test(NAME ${unit_test_name} COMMAND ${unit_test_name}) | ||
| if (NOT ("boost_geometry_${prefix}_${item}" STREQUAL "boost_geometry_cs_undefined_setops1") AND | ||
| NOT ("boost_geometry_${prefix}_${item}" STREQUAL "boost_geometry_cs_undefined_setops2")) |
There was a problem hiding this comment.
Isn't this too specific?
There was a problem hiding this comment.
And why is it excluded. I get a memory violation - so that might be the reason.
Can't we pass an optional boolean variable to this function, to add to the test suite, or not?
(and then for ones like this we should not, but with a comment why not).
And then, probably, it can still be added as a unit test - but not as dependency to the custom tests target
| setops2 | ||
| ) | ||
| boost_geometry_add_unit_test("cs_undefined" ${item}) | ||
| endforeach() |
There was a problem hiding this comment.
So, in line with my comment above, you would get a second loop here
foreach(item IN ITEMS
setops1
setops2
)
boost_geometry_add_unit_test("cs_undefined" ${item} FALSE)
endforeach()
There was a problem hiding this comment.
Thanks, I added then one by one though.
| PRIVATE | ||
| "${PROJECT_SOURCE_DIR}/test/robustness" | ||
| .) | ||
| endforeach() |
There was a problem hiding this comment.
These are not unit tests. These are programs.
I think we need to add a specific function for them.
But anyway, great they are at least compiled now.
| # Include the main Geometry test folder and the current folder | ||
| target_include_directories(${unit_test_name} | ||
| PRIVATE | ||
| "${PROJECT_SOURCE_DIR}/test" | ||
| "${PROJECT_SOURCE_DIR}/test" | ||
| "${PROJECT_SOURCE_DIR}/index/test" | ||
| .) | ||
|
|
||
| # To compile with C++14 | ||
| target_compile_features(${unit_test_name} PRIVATE cxx_std_14) |
There was a problem hiding this comment.
The PR description mentions: test compilation with C++17
But won't this setting override that?
There was a problem hiding this comment.
It seems that cmake selects the highest version of the standard specified in both CMAKE_CXX_STANDARD and target_compile_features.
see also https://discourse.cmake.org/t/how-to-handle-different-cxx-standards-in-the-same-project/6510/6
However, it is not very neat to mix CMAKE_CXX_STANDARD and target_compile_features. As the code is right now I think it is simpler to just use CMAKE_CXX_STANDARD and avoid target_compile_features, as the former only needed to be set once while the latter need to be set for each individual test. But maybe for this PR it is OK as is. What do you think?
There was a problem hiding this comment.
I agree, maybe we should just use CMAKE_CXX_STANDARD
And for this PR, it is OK as it is.
Agreed.
| #ticket_9081 | ||
| ) | ||
| boost_geometry_add_unit_test("robustness" ${item}) | ||
| target_include_directories(boost_geometry_robustness_${item} |
There was a problem hiding this comment.
You might use ${BOOST_GEOMETRY_UNIT_TEST_NAME}, it is set by previous function for this purpose
|
@barendgehrels thansk for the review and the comments. I addressed them all. Only the discussion on the c++ standard needs some check. |
| .) | ||
|
|
||
| # To compile with C++14 | ||
| target_compile_features(${unit_test_name} PRIVATE cxx_std_14) | ||
|
|
||
| # To be able to run ctest | ||
| add_test(NAME ${unit_test_name} COMMAND ${unit_test_name}) | ||
| set(extra_macro_args ${ARGN}) | ||
| if (NOT extra_macro_args STREQUAL "not_run") |
|
Thanks for handling everything, completely fine! |
This PR: