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

Feature 32 build tests in tpl mode #33

Merged
merged 2 commits into from
Mar 11, 2019

Conversation

nlslatt
Copy link
Contributor

@nlslatt nlslatt commented Mar 6, 2019

I made some tweaks so that checkpoint can find gtest when building in TPL mode. I also deleted the (unused) configure option I added before we decided to change the behavior of find_package_local.

@nlslatt nlslatt requested a review from lifflander March 6, 2019 18:40
Copy link
Contributor

@lifflander lifflander left a comment

Choose a reason for hiding this comment

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

Everything looks good except for possibly a missing namespace on the target_link_libraries

@@ -27,8 +27,7 @@ file(
#
macro(checkpoint_link_target target has_mpi)
target_include_directories(${target} PUBLIC ${PROJECT_TEST_UNIT_DIR})
target_link_libraries(${target} PRIVATE GTest::GTest)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it ok that we are loosing the namespace here? Or am I missing something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When building using find_package(GTest), GTest::GTest and GTest::Main are defined and both included in the variable GTEST_BOTH_LIBRARIES as a result of the find_package call. When in TPL mode, we don't use find_package, so none of that is true. Instead, I am setting GTEST_BOTH_LIBRARIES to the actual library names because that is all I can do (without stealing code from the FindGTest cmake module).

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I see. That makes sense now

@nlslatt nlslatt merged commit 39621ee into develop Mar 11, 2019
@mperrinel mperrinel mentioned this pull request Mar 14, 2019
@nlslatt nlslatt deleted the feature-32-build-tests-in-tpl-mode branch March 21, 2019 17:15
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.

None yet

2 participants