-
Notifications
You must be signed in to change notification settings - Fork 306
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
Make gtest optional. #78
Conversation
Don't build s2testing if there's no GTEST. Don't build examples if s2testing is not a target.
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for the patch! @hpolloni I guess you need to sign the CLA before this can be merged. |
@hpolloni Would you kindly sign the CLA? I'm using your patch in my own build. But it would be great if it got merged in upstream. This is the patch I use: diff --git a/CMakeLists.txt b/CMakeLists.txt
index 5ecd280..c6b15ac 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -198,10 +198,6 @@ add_library(s2
src/s2/util/math/exactfloat/exactfloat.cc
src/s2/util/math/mathutil.cc
src/s2/util/units/length-units.cc)
-add_library(s2testing STATIC
- src/s2/s2builderutil_testing.cc
- src/s2/s2shapeutil_testing.cc
- src/s2/s2testing.cc)
target_link_libraries(
s2
${GFLAGS_LIBRARIES} ${GLOG_LIBRARIES} ${OPENSSL_LIBRARIES}
@@ -242,7 +238,6 @@ install(FILES src/s2/_fp_contract_off.h
src/s2/s2builderutil_s2polyline_layer.h
src/s2/s2builderutil_s2polyline_vector_layer.h
src/s2/s2builderutil_snap_functions.h
- src/s2/s2builderutil_testing.h
src/s2/s2cap.h
src/s2/s2cell.h
src/s2/s2cell_id.h
@@ -319,9 +314,7 @@ install(FILES src/s2/_fp_contract_off.h
src/s2/s2shapeutil_range_iterator.h
src/s2/s2shapeutil_shape_edge.h
src/s2/s2shapeutil_shape_edge_id.h
- src/s2/s2shapeutil_testing.h
src/s2/s2shapeutil_visit_crossing_edge_pairs.h
- src/s2/s2testing.h
src/s2/s2text_format.h
src/s2/s2wedge_relations.h
src/s2/sequence_lexicon.h
@@ -407,12 +400,21 @@ install(FILES src/s2/util/math/mathutil.h
install(FILES src/s2/util/units/length-units.h
src/s2/util/units/physical-units.h
DESTINATION include/s2/util/units)
-install(TARGETS s2 s2testing DESTINATION lib)
+install(TARGETS s2 DESTINATION lib)
message("GTEST_ROOT: ${GTEST_ROOT}")
if (GTEST_ROOT)
+ add_library(s2testing STATIC
+ src/s2/s2builderutil_testing.cc
+ src/s2/s2shapeutil_testing.cc
+ src/s2/s2testing.cc)
add_subdirectory(${GTEST_ROOT} build_gtest)
include_directories(${GTEST_ROOT}/include)
+ install(FILES src/s2/s2builderutil_testing.h
+ src/s2/s2shapeutil_testing.h
+ src/s2/s2testing.h
+ DESTINATION include/s2)
+ install(TARGETS s2testing DESTINATION lib)
set(S2TestFiles
src/s2/encoded_s2cell_id_vector_test.cc
@@ -527,7 +529,7 @@ if (GTEST_ROOT)
endforeach()
endif()
-if (BUILD_EXAMPLES)
+if (BUILD_EXAMPLES AND TARGET s2testing)
add_subdirectory("doc/examples" examples)
endif() |
Hi, I have signed the CLA two times. Individual agreements we have on file for you:
Should I sign it again? |
Maybe you have to @googlebot? As in your commit, there are 3 testing header files unnecessarily installed when GTEST_ROOT is not set. |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I lost this. I'll merge this if it still works with against head.
@@ -198,10 +198,14 @@ add_library(s2 | |||
src/s2/util/math/exactfloat/exactfloat.cc | |||
src/s2/util/math/mathutil.cc | |||
src/s2/util/units/length-units.cc) | |||
|
|||
if (GTEST_ROOT) | |||
add_library(s2testing STATIC | |||
src/s2/s2builderutil_testing.cc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are additional _testing.cc files that include gunit now.
@@ -407,7 +411,11 @@ install(FILES src/s2/util/math/mathutil.h | |||
install(FILES src/s2/util/units/length-units.h | |||
src/s2/util/units/physical-units.h | |||
DESTINATION include/s2/util/units) | |||
if (GTEST_ROOT) | |||
install(TARGETS s2 s2testing DESTINATION lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
two space indent
Obsoleted by #236 |
Don't build s2testing if there's no GTEST.
Don't build examples if s2testing is not a target.