- 
          
- 
                Notifications
    You must be signed in to change notification settings 
- Fork 56.3k
Added support for Eigen cross-compilation #13337
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
Conversation
        
          
                CMakeLists.txt
              
                Outdated
          
        
      | OCV_OPTION(WITH_CUBLAS "Include NVidia Cuda Basic Linear Algebra Subprograms (BLAS) library support" ON IF (NOT IOS AND NOT WINRT) ) | ||
| OCV_OPTION(WITH_NVCUVID "Include NVidia Video Decoding library support" ON IF (NOT IOS AND NOT APPLE) ) | ||
| OCV_OPTION(WITH_EIGEN "Include Eigen2/Eigen3 support" (NOT CV_DISABLE_OPTIMIZATION) IF (NOT WINRT AND NOT CMAKE_CROSSCOMPILING) ) | ||
| OCV_OPTION(WITH_EIGEN "Include Eigen2/Eigen3 support" (NOT CV_DISABLE_OPTIMIZATION) IF (NOT WINRT) ) | 
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.
It is fine to allow Eigen for cross-compilation.
But lets continue enabling it explicitly via -DWITH_EIGEN=ON.
(NOT CV_DISABLE_OPTIMIZATION AND NOT CMAKE_CROSSCOMPILING)
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.
So why does WITH_EIGEN defaults to OFF when cross-compiling?
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.
Because of include directory.
What is going with cross-compilation if you add /usr/include (where Eigen is located on host system) into the list of include directories?
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.
This is the point of this PR. The host version of Eigen will not be used during cross-compilation anymore.
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.
I mean that result of compilation is unpredictable if you add host /usr/include (or /usr/local/include) into compiler list of include directories.
This dangerous behavior should be disabled by default.
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.
Yes, that's why this PR removed unconditional /usr/include and /usr/local/include Eigen search prefixes, which are set by CMake anyway. But not during cross-compilation.
The question is now why do you only exlude Eigen (and presumably few other libraries) during cross-compliation, but not other? This is inconsistent.
        
          
                cmake/OpenCVFindLibsPerf.cmake
              
                Outdated
          
        
      | PATH_SUFFIXES include/eigen3 include/eigen2 Eigen/include/eigen3 Eigen/include/eigen2 | ||
| DOC "The path to Eigen3/Eigen2 headers" | ||
| CMAKE_FIND_ROOT_PATH_BOTH) | ||
| find_package (Eigen3) | 
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 is no such package in CMake 2.8.12 (Ubuntu 14.04) at least.
Need to guard this statement by check with proper CMake version and "OR DEFINED Eigen3_DIR".
Use "QUIET" option to avoid unnecessary messages.
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.
The package is not part of CMake but installed by Eigen itself.
        
          
                cmake/OpenCVFindLibsPerf.cmake
              
                Outdated
          
        
      |  | ||
| if (TARGET Eigen3::Eigen) | ||
| # Use Eigen3 improted target if possible | ||
| ocv_include_directories ($<TARGET_PROPERTY:Eigen3::Eigen,INTERFACE_INCLUDE_DIRECTORIES>) | 
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.
Code style: please avoid "space" before bracket "("
104c4b5    to
    d0b00ad      
    Compare
  
    | This patch causes massive shadow warnings on newest versions of GCC, GCC 6.5: https://ocv-power.imavr.com/#/builders/1/builds/182 | 
| We prefer to follow this policy about CMake options for 3rdparty dependencies: 
 You can bypass (or disable) auto-detection code and setup required variables manually (including  | 
| Warnings from external headers are usually handled by using of system ( | 
| The PR does not do anything differently besides changing the  | 
| 
 The PR changes only the  | 
| 
 Looking closely at the CI logs of @seiko2plus, he does not seem to cross-compile. The host system is just a ppc64. | 
d0b00ad    to
    3af178e      
    Compare
  
    | @alalek PTAL | 
| 
 The PR does change search directory option for Eigen path from "-isystem" to "-I" and that disabled shadow suppressing in 
 The reason of changing gcc option is coming from passing generator expression to ocv_include_directories() and that made ocv_is_opencv_directory() fail to determine if the path part of opencv or not, since cmake expression is evaluated during build. opencv/cmake/OpenCVFindLibsPerf.cmake Line 48 in 3af178e 
 I suggest you to use EIGEN3_INCLUDE_DIR with ocv_include_directories instead of generator expression since even if you going to use include_directories(AFTER SYSTEM ) directly, the problem will still remain during generate pch. | 
| @seiko2plus That does not make any sense. The Eigen path was never part of OpenCV, hence the problem must be in  | 
| 
 This statement is obviously incorrect. The PR does not (actively) do such thing. | 
| 
 The problem isn't in  
 
 Indirect way it does | 
| I understand this. However, this does not change the fact that the issue is caused by  opencv/cmake/OpenCVUtils.cmake Lines 227 to 230 in 7e2ebec 
 Instead, the generator expression is converted to an absolute path by prepending the directory OpenCV resides in. Therefore the subdirectory check succeeds and the  This is obviously a separate issue. Using  | 
| This breaks the build of OpenCV 3.4.7 with Eigen and ENABLE_PRECOMPILED_HEADERS in Nixpkgs: 
 This issue was reported in #14868. The author has closed it because they found a workaround. It has not been fixed in OpenCV. The workaround is to configure cmake with  | 
| The build gets a bit further if I add   | 
This PR fixes two (closely related) issues:
WITH_EIGENcan still be enabled. In this case, however, CMake will use the Eigen version from the host system (typically from/usror/usr/local) even if the toolchain explicitly disables this behavior (and rightly so). The libraries in theCMAKE_FIND_ROOT_PATHset by toolchains must be located with higher priority.This PR uses the Eigen package config if it's available and falls back to previous
find_pathbehavior if the package config is not available. The latter behavior also respects the search mode for packages requested by toolchains now.