Skip to content

CMakeLists.txt - Hide Unit Test related stuff behind option#171

Open
Jogi7819 wants to merge 1 commit intoWG21-SG14:masterfrom
Jogi7819:master
Open

CMakeLists.txt - Hide Unit Test related stuff behind option#171
Jogi7819 wants to merge 1 commit intoWG21-SG14:masterfrom
Jogi7819:master

Conversation

@Jogi7819
Copy link

@Jogi7819 Jogi7819 commented Feb 24, 2020

Hide required package THREAD behind option to give the consumer the choice to only add the project interface target without any test dependencies.

@Jogi7819 Jogi7819 changed the title Update CMakeLists.txt CMakeLists.txt - Hide Unit Test related stuff behind option Feb 24, 2020
@Quuxplusone
Copy link
Contributor

The offending line was added by @TBBle in b17b909, and I think I made it obsolete in c5e9751 by removing the one (bad) test file that used anything thread-related. Have you tried just reverting b17b909 and seeing if anything breaks?

@Jogi7819
Copy link
Author

Will check that today and give a feedback. But still if that commit would be fine I would suggest to hide all test related stuff behind an option that can be set by a top-level project.

@Jogi7819
Copy link
Author

The offending line was added by @TBBle in b17b909, and I think I made it obsolete in c5e9751 by removing the one (bad) test file that used anything thread-related. Have you tried just reverting b17b909 and seeing if anything breaks?

You are right the package is not required anymore and can be removed. Done in my PR.

@Jogi7819 Jogi7819 requested a review from Quuxplusone February 25, 2020 14:47
Copy link
Contributor

@Quuxplusone Quuxplusone left a comment

Choose a reason for hiding this comment

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

Seems harmless to me, but also IMHO unnecessary. I don't know much about how we expect this library to be consumed. Does it really make sense for someone to want to consume this library without running its tests? And if they did, would they really do that by wholesale-importing the CMakeLists.txt that we wrote, instead of by just cutting and pasting the single .h file they were interested in?

CMakeLists.txt Outdated
elseif ("${CMAKE_CXX_COMPILER_ID}" STREQUAL "GNU")
target_compile_options(${TEST_NAME} PRIVATE -Wall -Wextra -Werror)
set_source_files_properties(${SG14_TEST_SOURCE_DIRECTORY}/plf_colony_test.cpp PROPERTIES
COMPILE_FLAGS "-Wno-unused-parameter")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you uncuddle this parenthesis again, so that git show -b will have one fewer gratuitous diff?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@Jogi7819
Copy link
Author

Seems harmless to me, but also IMHO unnecessary. I don't know much about how we expect this library to be consumed. Does it really make sense for someone to want to consume this library without running its tests? And if they did, would they really do that by wholesale-importing the CMakeLists.txt that we wrote, instead of by just cutting and pasting the single .h file they were interested in?

One aim of modern cmake is to provide everything by target and target specific. Means to me: includes, flags, definitions, etc.
Also I guess to keep external dependencies as small you can, so why adding test related stuff (includes the target itself) if you are not a third-party lib developer. The THREAD dependency is a good example here. It has been required at some point to build the test but it was never required to use the lib itself.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants