Android toolchain breaks ExternalProject_Add behaviour about polly HOT 10 CLOSED

So I assume it's safe to remove the CMAKE_FIND_ROOT_PATH_MODE_PROGRAM line

I think in this case find_program will find arm executables which can't be run on x86 hosts. I think you just need to modify CMAKE_FIND_ROOT_PATH variable so it contain path to git, svn etc.

from polly.

Well, this line overwrites the default search paths and then disables system executables here. So now as a developer I either have to fill CMAKE_FIND_ROOT_PATH manually with all the possible search locations or forward paths to all the binaries used in the project (for example Subversion_SVN_EXECUTABLE). And all of this only in case if android toolchain is used. Any other toolchain will compile without problems. By the way, CMake doesn't really helps with finding a cause of an error. Messages like error: could not find svn for checkout ${name} is not really helpful then you see it first time :)

My points are:

• Android toolchain is the only place in this repository where CMAKE_FIND_ROOT_PATH_MODE_PROGRAM variable is set.
• According to the find_program documentation this variable is not required to be set the way it is now set (the platform-specific executables will be used with higher priority anyway)
• ExternalProject_Add is only an example. If any other CMake commands or user lists uses the find_program command or rely on the CMAKE_FIND_ROOT_PATH variable they will get unexpected results with the android toolchain.

Maybe I'm missing something, but if you could provide me with a simple example, explaining why theCMAKE_FIND_ROOT_PATH_MODE_PROGRAM variable is set the way it is now, that would be very helpful.

from polly.

Android toolchain is the only place in this repository where CMAKE_FIND_ROOT_PATH_MODE_PROGRAM variable is set

actually this may indicate potential problems in other toolchains

the platform-specific executables will be used with higher priority anyway

if they will be found. in case you have arm svn in SYSTEM_ROOT and no x86 svn then arm version will be found. In case you have ONLY you'll get error.

Well, this line overwrites the default search paths and then disables system executables here.

I think this is the root of the issue. @yowidin Can you try to modify this place so it will be list(APPEND ...) instead of set(...)?

from polly.

I think this is the root of the issue. @yowidin Can you try to modify this place so it will be list(APPEND ...) instead of set(...)?

No, I tried and it doesn't help. According to the find_program documentation:

The CMake variable CMAKE_FIND_ROOT_PATH specifies one or more directories to be prepended to all other search directories. This effectively “re-roots” the entire search under given locations.

So only android executables are taken into account.

from polly.

Okay, seems that for android you need to use find_host_package(Subversion). I think putting this code before any ExternalProject_Add will do the trick:

if(ANDROID)
  find_host_program(Subversion REQUIRED)
endif()

I think the problem is a little bit wider than just Android toolchain. There are some really important stuff missing in CMake itself (multiple toolchains/root).

from polly.

I personally prefer to keep my CMake files platform-independent so I just built in the following test in my code:

find_package(Subversion QUIET)
if(NOT Subversion_SVN_EXECUTABLE)
   if(DEFINED CMAKE_TOOLCHAIN_FILE)
      message(FATAL_ERROR "Seems like CMake could not find svn and you are using a toolchain file. Try to provide svn path via -DSubversion_SVN_EXECUTABLE=`which svn`")
   endif()
endif()

Also I tried to remove the CMAKE_FIND_ROOT_PATH_MODE_PROGRAM line from the android toolchain and everything worked as expected.

I really cannot imagine any case where current behaviour would be appropriate. All the required binaries for the toolchain itself are set within the toolchain and in case if system binaries should be ignored the ONLY_CMAKE_FIND_ROOT_PATH flag could be provided to the find_program command within toolchain.

In my opinion a toolchain shouldn't change project-wide search behaviour, it should manipulate it on the find_* command basis using the appropriate flags.

My suggestion is to remove this line and maybe replace both find_host_package and find_host_program commands with dummies containing error/warning message. Something like:

find_program is already host-based, check your other find_program calls and append ONLY_CMAKE_FIND_ROOT_PATH where required.

from polly.

find_package(Subversion) should not behave the same way as find_package(OpenSSL) for example. You have to explain somehow that you need SVN on host and you need OpenSSL for target. I.e. you need x86 SVN binary and arm OpenSSL libraries. In terms of Android toolchain you have to call find_host_package(Subversion) and find_package(OpenSSL).

My suggestion is to remove this line and maybe replace both find_host_package and find_host_program commands with dummies containing error/warning message

Make no sense and will break OpenCV build: https://github.com/hunter-packages/opencv/blob/4af193ed31df03bda72f493afefc39543ea8c43c/cmake/OpenCVDetectPython.cmake#L40

from polly.

Whats why the ONLY_CMAKE_FIND_ROOT_PATH parameter exists.

In case if your library needs a host binary it calls find_package(Subversion) or find_package(Subversion NO_CMAKE_FIND_ROOT_PATH) either is fine in case if the CMAKE_FIND_ROOT_PATH_MODE_PROGRAM variable isn't set.

In case if your toolchain needs target binary or library it should use find_package(OpenSSL ONLY_CMAKE_FIND_ROOT_PATH).

But a toolchain definitely shouldn't set global flags like that. It's not like if you are using a toolchain every other library and find_package call in the user's project should be adjusted and replaced with find_host_package just for the toolchain's sake.

As for my suggestion it's sole purpose is to ensure that toolchain users know how to fix the issue.

from polly.

In case if your toolchain needs target binary or library it should use find_package(OpenSSL ONLY_CMAKE_FIND_ROOT_PATH)

in a lot of cases finding system OpenSSL is quite fine. as far as I understand this command will not check default system paths.

But a toolchain definitely shouldn't set global flags like that. It's not like if you are using a toolchain every other library and find_package call in the user's project should be adjusted and replaced with find_host_package just for the toolchain's sake

that is why I said that this functionality is missing from CMake (at least is not clear to me).

in general I'm not planning to make such critical changes since I want it work as close to OpenCV as possible, so we can avoid fixing non-trivial bugs. if you want to discuss this problem you really should start some topic on OpenCV forum or CMake mailing list. In my opinion there is quite nothing to discuss in terms of Polly.

from polly.

Hope this helps. Let me know if you have any further questions.

from polly.