laxnpander / OpenREALM

OpenREALM is a pipeline for real-time aerial mapping utilizing visual SLAM and 3D reconstruction frameworks.
GNU Lesser General Public License v2.1
453 stars 117 forks source link

Additional CMake Options to allow disabling various LGPL/GPL Libraries #45

Closed zthorson closed 3 years ago

zthorson commented 3 years ago

Description

Update the cmake file with a few minor fixes, as well as adding the following options to disable/enable compilation of certain code portions. All options default to ON unless otherwise set, so the build itself shouldn't change.

Reason

To avoid licensing issues with some LGPL and GPL libraries, it is useful to be able to disable compilation of certain features. This will disable some functionality of the code, but allow the rest of it to be used without using the entire codebase as LGPL.

Method / Design

The option flags are put in the cmake files that are closest to the option being changed. In some cases disabling a module will have side effects. For example, disabling the ortho module will disable some stages in the stage library. This should be handled seamlessly.

Testing

Compiled and run on: Ubuntu 20.04 - Desktop macOS Catalina iOS (targeting iPad/iPhone)

Other Notes

This should not be merged until the previous statistics pull request is merged. If that merge doesn't take care of the extra changes in this file, I can rebase before the final merge.

laxnpander commented 3 years ago

@zthorson: Can you try the latest dev build? I think it might not work yet, I at least have still problems using it in my ROS environment. But this may be due to some custom libraries I installed. Especially the openvslam::openvslam and Eigen3::Eigen targets are causing some major headache. But I will wrap my head around it. Also your additions will require cmake 3.15 due to the add_compile_definitions. Will have to add this to the install_deps script and the CI.

zthorson commented 3 years ago

@laxnpander I was able to build it for Ubuntu 20.04 with a few changes. Notably I had to move the find_package to Exiv2 to inside the WITH_EXIV2 flag, and remove FindOpenCV.cmake from the cmake directory. I found the FindOpenCV.cmake was causing issues with being able to find that package.

Cmake is able to configure fine for iOS, but I'm seeing a new error I haven't seen before when building the static library. It could be a change on my end, but I'll have to dig in a bit more (It has to do with making fat static libraries, nothing that you would hit on Linux).

Both Eigen and OpenVSLAM appeared to work for me. What error messages are you seeing?

Changes made: delete cmake/FindOpenCV.cmake

diff --git a/modules/realm_io/CMakeLists.txt b/modules/realm_io/CMakeLists.txt
index cb748ba..944a2f0 100755
--- a/modules/realm_io/CMakeLists.txt
+++ b/modules/realm_io/CMakeLists.txt
@@ -20,7 +20,6 @@ if (NOT OpenCV_FOUND)
     message(WARNING "OpenCV 4 Support is experimental, use at your own risk!")
 endif()

-find_package(Exiv2 REQUIRED)
 find_package(GDAL REQUIRED)
 find_package(PCL 1.7 REQUIRED)
 list(REMOVE_ITEM PCL_LIBRARIES "vtkproj4")
@@ -61,6 +60,7 @@ set(SOURCE_FILES

 # Conditionally add exif import/export to handle removing Exiv2 dependancy
 if (WITH_EXIV2)
+    find_package(Exiv2 REQUIRED)
     list(APPEND HEADER_FILES
             ${root}/include/realm_io/exif_export.h
             ${root}/include/realm_io/exif_import.h)
@@ -195,4 +195,4 @@ if(TESTS_ENABLED)
                 ${CMAKE_BINARY_DIR}/${CMAKE_INSTALL_BINDIR}/run_realm_io_tests
     )

-endif()
\ No newline at end of file
+endif()
zthorson commented 3 years ago

The iOS build issue was something on my end, not with the files themselves. I was able to fix it without any further changes to the cmake files of the project.

laxnpander commented 3 years ago

@zthorson: Okay perfect! I have the find_package(Exiv2 REQUIRED) a bit down the file when the library is linked. I guess this should be sufficient? Removed it from the top. You added a FindOpenCV.cmake file in your PR. What was the reasoning? Was it intentional? On 18.04 it didnt workt with yours, I guess in 20.04 mine didnt do it. If we can just delete it, it's fine by me.

When compiling OpenREALM openvslam and eigen don't complain, it's only when I link against the OpenREALM lib it says it can't find the targets openvslam::openvslam and Eigen3::Eigen. This would be the case when they are not exported properly, but it seems fine to me.

zthorson commented 3 years ago

@laxnpander We can just delete it. I had put it in there to solve finding OpenCV on different root filesystems for cross compilation, but I changed my mind and instead added the OpenCVConfig.cmake files to my cross compiled root filesystem, which seemed to work better. I thought I had removed them all (I had them in each module that needed it), but must have missed that one.

A bit further down should be fine for the Exiv2 include. I tried just removing the line as you did and it worked fine on my end.

I see. I'm not getting the warning, but I'm compiling the OpenREALM library as a submodule of my cmake project. Since I'm not installing the library, then linking against it that could be the difference. I can try installing and linking instead later today or tomorrow to see if I find anything.

laxnpander commented 3 years ago

@zthorson: Then I will just remove it! I am currently working on fixing the CI to use a new CMake version. As soon as this works, I may know if it's a problem that only occurs on my system. But feel free to try and link against it!

laxnpander commented 3 years ago

@zthorson: With the latest commit a8c6e212011798c7c9b69ce8a20bed61a6582864 I feel quite confident, that everything should work as expected. CI is back up, docker container updated and PSL removed from the main repository. If you give your go, I'd close this up and recommend to rebase.

zthorson commented 3 years ago

I saw you added a WITH_PCL flags, but since there wasn't an cmake option line, it is defaulting to off and isn't visible with a cmake -LAH command. Was this intentional? Did you want PCL included or excluded by default?

I added the option to /module/CMakeLists.txt if you want to use that:

diff --git a/modules/CMakeLists.txt b/modules/CMakeLists.txt
index 1e0f585..dbb3798 100644
--- a/modules/CMakeLists.txt
+++ b/modules/CMakeLists.txt
@@ -24,6 +24,7 @@ cmake_minimum_required(VERSION 3.15)

 # Optional Libraries
 option(WITH_EXIV2      "Enable/Disable support for Exiv2 input libraries" ON)
+option(WITH_PCL        "Enable/Disable support for PCL libraries"         ON)

 # Optional Modules
 option(WITH_core       "Enable/Disable building of core library"          ON)
zthorson commented 3 years ago

@laxnpander With or without the option change mentioned above, it appeared to compile fine on Ubuntu 20.04 as a shared library, and iOS as a static library. Seems good to go.

The only minor issue I hit was having to run a git submodule init and update to get the PSL libraries so that CMake was happy. I tested it with the WITH_densifier=ON on the 20.04 build and WITH_densifier=OFF on the iOS build, and both worked after the submodule update.

laxnpander commented 3 years ago

@zthorson Ah jeah right, PCL should be turned on by default. Missed that, thanks. It's not really required atm, but I'll keep it on for now.

Yes, the submodule init is already added to the readme. I guess ideally it would automatically run the commands or check if the directory exists, when compiled WITH_densifier flag on, but I am not sure if its worth the additional code. Just makes the CMake file more complex with non-cmake commands, while it can be resolved just by reading the readme. I'll see if there is ever issues popping up about it and then decide to make it more foolproof.

I will add your recommended option and then merge into master. Reopen in case you find any new problems.