[3party] Optionally allow building with packaged googletest #3528

2022-10-03T13:51:35Z

Ferenc- commented

2022-10-03 13:51:35 +00:00

(Migrated from github.com)

Following the approved patter in organicmaps/organicmaps#2863 (comment)

Signed-off-by: Ferenc Géczi ferenc.gm@gmail.com

* Following the approved patter in https://git.omaps.dev/organicmaps/organicmaps/pulls/2863#issuecomment-1262797057 Signed-off-by: Ferenc Géczi <ferenc.gm@gmail.com>

biodranik (Migrated from github.com) reviewed 2022-10-03 21:50:41 +00:00

drape/drape_tests/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-10-03 21:49:57 +00:00

Var is not needed if it's used only once.

biodranik (Migrated from github.com) commented

2022-10-03 21:50:37 +00:00

It would be great to update gtest with the latest version first manually, in a separate commit.

Ferenc- (Migrated from github.com) reviewed 2022-10-04 16:16:38 +00:00

drape/drape_tests/CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2022-10-04 16:16:38 +00:00

All right, it will be here: organicmaps/organicmaps#3538

All right, it will be here: https://git.omaps.dev/organicmaps/organicmaps/pulls/3538

Ferenc- (Migrated from github.com) reviewed 2022-10-04 16:17:02 +00:00

drape/drape_tests/CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2022-10-04 16:17:01 +00:00

Removed.

vng commented

2022-10-06 07:39:00 +00:00

(Migrated from github.com)

Do we really need a full GA matrix for Linux with packages?
Why this GA checks didn't start?

- Do we really need a full GA matrix for Linux with packages? - Why this GA checks didn't start?

Ferenc- commented

2022-10-06 15:16:16 +00:00

(Migrated from github.com)

Do we really need a full GA matrix for Linux with packages?

Why this GA checks didn't start?

I could remove the CI part for now, so the rest of the commit can progress. Would that be all right?
Would that merit a merge otherwise?

> * Do we really need a full GA matrix for Linux with packages? > * Why this GA checks didn't start? I could remove the CI part for now, so the rest of the commit can progress. Would that be all right? Would that merit a merge otherwise?

vng commented

2022-10-06 16:36:58 +00:00

(Migrated from github.com)

I don't mind

Ferenc- commented

2022-10-06 18:11:40 +00:00

(Migrated from github.com)

I don't mind

All right I have ditched the CI code. Please take a look.

> I don't mind All right I have ditched the CI code. Please take a look.

biodranik commented

2022-10-06 20:12:01 +00:00

(Migrated from github.com)

@Ferenc- Interesting, why are you focusing on the google test now? Just for training purposes? It is used for unit tests only, e.g. usually should not be packaged with Linux distros.

biodranik (Migrated from github.com) reviewed 2022-10-06 20:23:47 +00:00

biodranik (Migrated from github.com) left a comment

Good start! I suggest you try to make a function (or macro?) that encapsulates this logic and can be reused for most of the other 3party libs.

drape/drape_tests/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-10-06 20:19:25 +00:00

Probably QUIET option is needed to avoid log pollution when the package is not found, and it is found. What is the output in these both cases?

biodranik (Migrated from github.com) commented

2022-10-06 20:22:40 +00:00

  message(STATUS "Using the system GoogleTest ${GTest_VERSION}")

```suggestion message(STATUS "Using the system GoogleTest ${GTest_VERSION}") ```

Ferenc- (Migrated from github.com) reviewed 2022-10-07 00:09:41 +00:00

drape/drape_tests/CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2022-10-07 00:09:41 +00:00

The current output for the not found scenario is:

-- Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR GTEST_MAIN_LIBRARY) (Required is at least version "1.12.0")

When found it is:

-- Found GTest: /usr/local/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.12.1", minimum required is "1.12.0")
-- The build will rely on the system provided GoogleTest version

The current output for the **not found** scenario is: ```` -- Could NOT find GTest (missing: GTEST_LIBRARY GTEST_INCLUDE_DIR GTEST_MAIN_LIBRARY) (Required is at least version "1.12.0") ```` When found it is: ```` -- Found GTest: /usr/local/lib/cmake/GTest/GTestConfig.cmake (found suitable version "1.12.1", minimum required is "1.12.0") -- The build will rely on the system provided GoogleTest version ````

Ferenc- commented

2022-10-07 00:33:09 +00:00

(Migrated from github.com)

@Ferenc- Interesting, why are you focusing on the google test now? Just for training purposes? It is used for unit tests only, e.g. usually should not be packaged with Linux distros.

Changes are difficult in every organization and project. I am trying to "tread carefully", not to cause a lot of anger for established project members.
Plus changes to the tests only, can really not break the production code, on some walled platform that I have no access to.
There is also some training purpose. If this PR gets accepted, then there will be more to come, following the patterns demonstrated here.
Although the tests are not packaged by Linux distros, they are often executed to validate builds and build environments, so they shouldn't be neglected either.

> @Ferenc- Interesting, why are you focusing on the google test now? Just for training purposes? It is used for unit tests only, e.g. usually should not be packaged with Linux distros. Changes are difficult in every organization and project. I am trying to _"tread carefully"_, not to cause a lot of anger for established project members. Plus changes to the tests only, can really not break the production code, on some walled platform that I have no access to. There is also some training purpose. If this PR gets accepted, then there will be more to come, following the patterns demonstrated here. Although the tests are not packaged by Linux distros, they are often executed to validate builds and build environments, so they shouldn't be neglected either.

biodranik commented

2022-10-07 05:23:23 +00:00

(Migrated from github.com)

Good strategy :)

Comparing outputs, it's better to add a message when gtest is not found in the system and an embedded one is used, to avoid confusion and make it clearer for developers.

Good strategy :) Comparing outputs, it's better to add a message when gtest is not found in the system and an embedded one is used, to avoid confusion and make it clearer for developers.

Ferenc- commented

2022-10-07 15:24:09 +00:00

(Migrated from github.com)

Good strategy :)

Thanks! :)

Comparing outputs, it's better to add a message when gtest is not found in the system and an embedded one is used, to avoid confusion and make it clearer for developers.

Done!

> Good strategy :) Thanks! :) > Comparing outputs, it's better to add a message when gtest is not found in the system and an embedded one is used, to avoid confusion and make it clearer for developers. Done!

Ferenc- commented

2022-10-09 17:12:05 +00:00

(Migrated from github.com)

@biodranik so what do you think about the current state of this PR?
Do we absolutely need a function/macro already, when there would only be one sinle use?
Or does it make sense to apply these changes to more libraries first, and then we would see how to generalize best?

Another question:
Are you OK with the fact that if GoogleTest's cmake compatibility check is broken , then even a lover than required version would be picked up by this code?
The way I see, it is probably broken, hence I was suggesting putting all this behind a -DSYSTEM_.... switch, just to be sure.

@biodranik so what do you think about the current state of this PR? Do we absolutely need a function/macro already, when there would only be one sinle use? Or does it make sense to apply these changes to more libraries first, and then we would see how to generalize best? Another question: Are you OK with the fact that if GoogleTest's cmake compatibility check is broken , then even a lover than required version would be picked up by this code? The way I see, it is probably broken, hence I was suggesting putting all this behind a `-DSYSTEM_....` switch, just to be sure.

biodranik commented

2022-10-09 17:23:32 +00:00

(Migrated from github.com)

No, lower version should not be used.

Only that one which is already in the repo, or greater. Otherwise, a repo's version should be used.

Why do you think the macro implementation can be different?

No, lower version should not be used. Only that one which is already in the repo, or greater. Otherwise, a repo's version should be used. Why do you think the macro implementation can be different?

Ferenc- commented

2022-10-09 17:50:34 +00:00

(Migrated from github.com)

No, lower version should not be used.

I fully agree. At least not by accident, and we should try to avoid such accidents, so I would go with the -DSYSTEM_.... switch if that is OK with you.

Why do you think the macro implementation can be different?

Well the way find_package is looking for .cmake files, that might not be availabe for every dependency.
And alsofor example how fast double parser is simply inserted into SRC, doesn't seem to fit this pattern immediately.

> No, lower version should not be used. I fully agree. At least not by accident, and we should try to avoid such accidents, so I would go with the `-DSYSTEM_.... switch` if that is OK with you. > Why do you think the macro implementation can be different? Well the way `find_package` is looking for `.cmake` files, that might not be availabe for every dependency. And alsofor example how [fast double parser is simply inserted into SRC](https://github.com/organicmaps/organicmaps/blob/master/base/CMakeLists.txt#L5), doesn't seem to fit this pattern immediately.

biodranik commented

2022-10-09 18:13:32 +00:00

(Migrated from github.com)

Why DSYSTEM... is needed? Because if a lower version is installed in the system, it will be found and used?
What about syntax "1.12...<" ? Did you try it?

Why DSYSTEM... is needed? Because if a lower version is installed in the system, it will be found and used? What about syntax "1.12...<" ? Did you try it?

Ferenc- commented

2022-10-16 18:43:28 +00:00

(Migrated from github.com)

Why DSYSTEM... is needed? Because if a lower version is installed in the system, it will be found and used?

Yes exactly.

What about syntax "1.12...<" ? Did you try it?

1.12...< or 1.12.0...< appear to be invalid systax as of the latest CMake realease.

 find_package called with invalid argument "1.12.0...<"

A full version range like 1.12.0...<2.0.0 is valid, but firstly that results in a warning about GTest not supporting it:

`find_package()` specify a version range but the module GTest does not                                                                                                        
  support this capability.  Only the lower endpoint of the range will be                                                                                                        
  used.

And secondly, empirical tests show, that it doesn't work.

> Why DSYSTEM... is needed? Because if a lower version is installed in the system, it will be found and used? Yes exactly. > What about syntax "1.12...<" ? Did you try it? `1.12...<` or `1.12.0...<` appear to be invalid systax as of the latest CMake realease. ```` find_package called with invalid argument "1.12.0...<" ```` A full version range like `1.12.0...<2.0.0` is valid, but firstly that results in a warning about `GTest` not supporting it: ```` `find_package()` specify a version range but the module GTest does not support this capability. Only the lower endpoint of the range will be used. ```` And secondly, empirical tests show, that it doesn't work.

biodranik commented

2022-10-16 20:05:18 +00:00

(Migrated from github.com)

Yes, I misinterpret the cmake documentation. Why can't you check the version found by find_program, and use the embedded one if it is lower than the requested one?

Ferenc- commented

2022-10-16 22:10:27 +00:00

(Migrated from github.com)

Why can't you check the version found by find_program, and use the embedded one if it is lower than the requested one?

It is possible, it adds some extra complexity, either we need some 3party for that like this, or do some naive implementation for ourselves, that is not hundreads of lines:

set(use_system_gtest FALSE)
find_package(GTest)
if (${GTest_FOUND})
  string(REGEX MATCH "^([0-9]+)\\.([0-9]+)\\.([0-9]+)(.+)?$" SEMVER "${GTest_VERSION}")
  if ((${CMAKE_MATCH_1} GREATER 1) OR
      ((${CMAKE_MATCH_1} EQUAL 1) AND (${CMAKE_MATCH_2} GREATER 12)) OR
      ((${CMAKE_MATCH_1} EQUAL 1) AND (${CMAKE_MATCH_2} EQUAL 12) AND (${CMAKE_MATCH_3} GREATER_EQUAL 1)))
    set(use_system_gtest TRUE)
    message(STATUS "Using the system GoogleTest with version: ${GTest_VERSION}")
  endif()
endif()

if (NOT ${use_system_gtest})
  message(STATUS "Using the GoogleTest from submodule: ${CMAKE_CURRENT_BINARY_DIR}/googletest")
  add_subdirectory(../../3party/googletest ${CMAKE_CURRENT_BINARY_DIR}/googletest)
endif()

Which direction do you prefer?

> Why can't you check the version found by find_program, and use the embedded one if it is lower than the requested one? It is possible, it adds some extra complexity, either we need some 3party for that like [this](https://github.com/toeb/cmakepp/blob/master/cmake/semver/README.md), or do some naive implementation for ourselves, that is not hundreads of lines: ````cmake set(use_system_gtest FALSE) find_package(GTest) if (${GTest_FOUND}) string(REGEX MATCH "^([0-9]+)\\.([0-9]+)\\.([0-9]+)(.+)?$" SEMVER "${GTest_VERSION}") if ((${CMAKE_MATCH_1} GREATER 1) OR ((${CMAKE_MATCH_1} EQUAL 1) AND (${CMAKE_MATCH_2} GREATER 12)) OR ((${CMAKE_MATCH_1} EQUAL 1) AND (${CMAKE_MATCH_2} EQUAL 12) AND (${CMAKE_MATCH_3} GREATER_EQUAL 1))) set(use_system_gtest TRUE) message(STATUS "Using the system GoogleTest with version: ${GTest_VERSION}") endif() endif() if (NOT ${use_system_gtest}) message(STATUS "Using the GoogleTest from submodule: ${CMAKE_CURRENT_BINARY_DIR}/googletest") add_subdirectory(../../3party/googletest ${CMAKE_CURRENT_BINARY_DIR}/googletest) endif() ```` Which direction do you prefer?

Ferenc- commented

2022-10-22 18:46:15 +00:00

(Migrated from github.com)

Which direction do you prefer?

@biodranik What is your opinion about the proposal above?

> Which direction do you prefer? @biodranik What is your opinion about the proposal above?

biodranik commented

2022-10-23 12:23:11 +00:00

(Migrated from github.com)

@Ferenc- CMake has built-in IF for VERSION_GREATER_EQUAL, why can't we use it?

@Ferenc- CMake has built-in IF for [VERSION_GREATER_EQUAL](https://cmake.org/cmake/help/latest/command/if.html#version-greater-equal), why can't we use it?

Ferenc- commented

2022-10-25 15:27:19 +00:00

(Migrated from github.com)

@Ferenc- CMake has built-in IF for VERSION_GREATER_EQUAL, why can't we use it?

Something must have falsified my previous tests. now it appears to be a viable option.
I have pushed a new commit with VERSION_LESS.

> @Ferenc- CMake has built-in IF for [VERSION_GREATER_EQUAL](https://cmake.org/cmake/help/latest/command/if.html#version-greater-equal), why can't we use it? Something must have falsified my previous tests. now it appears to be a viable option. I have pushed a new commit with `VERSION_LESS`.

biodranik (Migrated from github.com) reviewed 2022-10-25 17:56:10 +00:00

biodranik (Migrated from github.com) left a comment

You see, it is almost perfect ) The next steps are:

To test if it works in all cases.
To make a macro (or function?) to apply a similar approach for other modules, if they need a similar approach.

You see, it is almost perfect ) The next steps are: 1. To test if it works in all cases. 2. To make a macro (or function?) to apply a similar approach for other modules, if they need a similar approach.

drape/drape_tests/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-10-25 17:54:58 +00:00

  message(STATUS "Using the GoogleTest from submodule: ${CMAKE_CURRENT_LIST_DIR}/../../googletest")
  add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/../../3party/googletest ${CMAKE_CURRENT_BINARY_DIR}/googletest)

```suggestion message(STATUS "Using the GoogleTest from submodule: ${CMAKE_CURRENT_LIST_DIR}/../../googletest") add_subdirectory(${CMAKE_CURRENT_LIST_DIR}/../../3party/googletest ${CMAKE_CURRENT_BINARY_DIR}/googletest) ```

biodranik (Migrated from github.com) approved these changes 2022-10-29 14:18:48 +00:00

Ferenc- commented

2022-10-29 19:15:57 +00:00

(Migrated from github.com)

Should I push the macro into this PR, or in a new one, when we can actually use it more than once?

biodranik (Migrated from github.com) reviewed 2022-10-30 13:28:21 +00:00

biodranik (Migrated from github.com) left a comment

What are the benefits of some intermediate commits? Can you try to write/test the macro on top of these changes? (We'll squash them at the end into one commit).

Ferenc- commented

2022-10-31 18:57:46 +00:00

(Migrated from github.com)

What are the benefits of some intermediate commits?

They can be reviewed and tested for correctness individually, and most importantly they can be reverted individually,
if all that failed.

Can you try to write/test the macro on top of these changes? (We'll squash them at the end into one commit).

Done. The macro is now part of this PR.

> What are the benefits of some intermediate commits? They can be reviewed and tested for correctness individually, and most importantly they can be reverted individually, if all that failed. > Can you try to write/test the macro on top of these changes? (We'll squash them at the end into one commit). Done. The macro is now part of this PR.

biodranik (Migrated from github.com) reviewed 2022-10-31 20:18:35 +00:00

biodranik (Migrated from github.com) left a comment

Thanks! Looks good )

cmake/FindPackageOrElseAddSubdirectory.cmake

biodranik (Migrated from github.com) commented

2022-10-31 19:59:08 +00:00

    if (${ARGC} GREATER 3)

Is this if necessary? What if ARGN is always passed to add_subdirectory?

```suggestion if (${ARGC} GREATER 3) ``` Is this if necessary? What if ARGN is always passed to add_subdirectory?

biodranik (Migrated from github.com) commented

2022-10-31 20:02:23 +00:00

Why is it better than

  if (${${PACKAGE_NAME}_FOUND} AND ("${${PACKAGE_NAME}_VERSION}" VERSION_GREATER_EQUAL "${MIN_REQUIRED_VERSION}"))

?

Why is it better than ```suggestion if (${${PACKAGE_NAME}_FOUND} AND ("${${PACKAGE_NAME}_VERSION}" VERSION_GREATER_EQUAL "${MIN_REQUIRED_VERSION}")) ``` ?

biodranik (Migrated from github.com) commented

2022-10-31 20:16:00 +00:00

    message(STATUS "Found ${PACKAGE_NAME} ${${PACKAGE_NAME}_VERSION} (minimum required version ${MIN_REQUIRED_VERSION})")

It would be useful to also print a path to found package, but it is not critical.

```suggestion message(STATUS "Found ${PACKAGE_NAME} ${${PACKAGE_NAME}_VERSION} (minimum required version ${MIN_REQUIRED_VERSION})") ``` It would be useful to also print a path to found package, but it is not critical.

drape/drape_tests/CMakeLists.txt

biodranik (Migrated from github.com) commented

2022-10-31 20:17:11 +00:00

Does it make sense to include this macro once from cmake/OmimHelpers.cmake and avoid inserting include into each 3p wrapper?

Does it make sense to include this macro once from `cmake/OmimHelpers.cmake` and avoid inserting include into each 3p wrapper?

drape/drape_tests/CMakeLists.txt

					
				@ -31,7 +31,12 @@ target_compile_definitions(${PROJECT_NAME}

				  PRIVATE GTEST_DONT_DEFINE_TEST WITH_GL_MOCK

biodranik (Migrated from github.com) commented

2022-10-31 20:18:16 +00:00

find_package_or_fallback_to_3party

Ferenc- (Migrated from github.com) reviewed 2022-11-03 17:54:22 +00:00

drape/drape_tests/CMakeLists.txt

					
				@ -31,7 +31,12 @@ target_compile_definitions(${PROJECT_NAME}

				  PRIVATE GTEST_DONT_DEFINE_TEST WITH_GL_MOCK

Ferenc- (Migrated from github.com) commented

2022-11-03 17:54:22 +00:00

Done

Ferenc- (Migrated from github.com) reviewed 2022-11-03 17:55:04 +00:00

cmake/FindPackageOrElseAddSubdirectory.cmake

Ferenc- (Migrated from github.com) commented

2022-11-03 17:55:04 +00:00

Removed.

Ferenc- (Migrated from github.com) reviewed 2022-11-03 17:58:57 +00:00

drape/drape_tests/CMakeLists.txt

Ferenc- (Migrated from github.com) commented

2022-11-03 17:58:57 +00:00

Makes things simpler for sure. The original idea was to avoid silently including macros, as it unintentionally invoked macros are rather hard and unpleasant to debug. But now I see, that it is considered premature optimization here.
So I moved the macro to cmake/OmimHelpers.cmake

Makes things simpler for sure. The original idea was to avoid silently including macros, as it unintentionally invoked macros are rather hard and unpleasant to debug. But now I see, that it is considered premature optimization here. So I moved the macro to `cmake/OmimHelpers.cmake`

Ferenc- (Migrated from github.com) reviewed 2022-11-03 18:45:22 +00:00

cmake/FindPackageOrElseAddSubdirectory.cmake

Ferenc- (Migrated from github.com) commented

2022-11-03 18:45:21 +00:00

The requested changes are done.

The path printing is tricky because for example the current gtest code doesn't even set the GTEST_INCLUDE_DIRS, the way I think it should. And GTEST_INCLUDE_DIRS or such are useless, empty, so we would have to look for what has been last added to the INCLUDE_DIRECTORIES, and get it out of there somehow.

The requested changes are done. The path printing is tricky because for example the current [gtest code](https://github.com/google/googletest/blob/main/googletest/CMakeLists.txt#L116) doesn't even set the `GTEST_INCLUDE_DIRS`, the way I think it should. And `GTEST_INCLUDE_DIRS` or such are useless, empty, so we would have to look for what has been last added to the `INCLUDE_DIRECTORIES`, and get it out of there somehow.

Ferenc- (Migrated from github.com) reviewed 2022-11-03 18:48:46 +00:00

cmake/FindPackageOrElseAddSubdirectory.cmake

Ferenc- (Migrated from github.com) commented

2022-11-03 18:48:45 +00:00

Because the current logical expression, expresses the relevant case that we have to handle.
The suggested conditional expression, does not express a relevant case, that is a rather irrelevant case, the fact that we even do something like logging in that scenario is just out of curtesy. We could skip that branch and the code would still work.

Because the current logical expression, expresses the relevant case that we have to handle. The suggested conditional expression, does not express a relevant case, that is a rather irrelevant case, the fact that we even do something like logging in that scenario is just out of curtesy. We could skip that branch and the code would still work.