Skip to content

Separated CMakeLists into self-contained files in subdirectories.#180

Closed
cboulay wants to merge 2 commits into
thp:masterfrom
psmoveservice:cmake_separation
Closed

Separated CMakeLists into self-contained files in subdirectories.#180
cboulay wants to merge 2 commits into
thp:masterfrom
psmoveservice:cmake_separation

Conversation

@cboulay
Copy link
Copy Markdown
Contributor

@cboulay cboulay commented Jul 10, 2015

I broke up CMakeLists into 5 files in the following directories:

  • psmoveapi
  • psmoveapi/src
  • psmoveapi/src/tracker
  • psmoveapi/src/utils
  • psmoveapi/examples
  • psmoveapi/bindings

All I did was cut & paste, and I changed a few directories to using relative CMAKE_CURRENT_LIST_DIR where possible to make the folders more portable if there's ever a desire to remove them or rename them. I did not change any of the logic.

With these changes, the directories listed above are self-contained. The main advantage for me is that it will be much easier to modify the preprocessor logic for the tracker without worrying about what I'm screwing up elsewhere. This is a prerequisite for me to be able to do the work in #163

Note that I am using the CMake include(path/to/sub/CMakeLists.txt) command instead of add_subdirectory(path/to/sub) because the latter places the binaries in similarly named build/path/to/sub/ and I don't know if that is desired.(It is possible to specify that all targets build to the same directory, but that generates many warnings and also interferes with CMake's ability to calculate % complete).

Also, when using add_subdirectory, variables defined in sister directories are not available, so examples and bindings cannot see OpenCV. I was able to include and link OpenCV successfully for the targets in these folders but there's currently an error in the OpenCV include and link logic that masked some other problems. I will revisit this in a future pull request.

I tested this with Windows MinGW64 mingw32-make, ubuntu, and in OS X.

I only tested the Python bindings in Windows, and the Python, Java, and Processing bindings in OS X.

@cboulay cboulay force-pushed the cmake_separation branch from 84a28aa to ac89881 Compare July 11, 2015 03:53
@cboulay
Copy link
Copy Markdown
Contributor Author

cboulay commented Jul 11, 2015

I updated the original comment after adding new commits and squashing. It now works on all 3 platforms.

@thp
Copy link
Copy Markdown
Owner

thp commented Jul 11, 2015

Good idea, but I'd even go for add_subdirectory(), which should theoretically allow building only subprojects if that is desired? I think this also makes it more modular than just using include()?

@cboulay
Copy link
Copy Markdown
Contributor Author

cboulay commented Jul 11, 2015

OK. The examples and bindings are both dependent on OpenCV. Should I put the OpenCV-related CMake options and logic in the root directory or should I repeat them for every child directory (root/src/tracker; root/examples; root/bindings)?

While I'm at it I will fix up the OpenCV logic. The way it is now, it always includes & links a system OpenCV if found. Then, additionally, if PSMOVE_USE_LOCAL_OPENCV is set then it will ALSO include and link the local version. This actually hid a problem from me when I was trying to update to opencv3, because I still had system opencv 2.4.11 but had local opencv3.

@cboulay
Copy link
Copy Markdown
Contributor Author

cboulay commented Jul 12, 2015

I think the best option for better OpenCV support (also addressing my comment for #170) is to provide a FindOpenCV.cmake file. Whether or not USE_LOCAL_OPENCV is set will determine whether or not OPENCV_SRC_DIR is used as a path hint for find_package. Then FindOpenCV.cmake will look for the OpenCV libraries in lib, in the appropriate directory from the precompiled binary distribution (win only), or wherever OpenCVConfig.cmake says to look.

As for using add_subdirectory instead of include, there are still a few issues. For example, if we allow the examples or bindings folders to be independent entry-points, then how do any of the executables in examples know where to find the psmoveapi (and _tracker) shared objects and include files? We could code in a ../include but then it is no longer strictly modular, so there's not much point in using add_subdirectory. We might add use PSMOVE_LIB_DIR and PSMOVE_INCLUDE_DIR options that are automatically written when calling from top-level but otherwise have to be specified?

Anyway, I think that is a bit much for this first PR.

@cboulay cboulay force-pushed the cmake_separation branch from ad0778b to 8fd1921 Compare July 14, 2015 02:58
@cboulay
Copy link
Copy Markdown
Contributor Author

cboulay commented Jul 14, 2015

CMake's built-in ability to find the OpenCV package works very well. I put OpenCV 2.4.11 or OpenCV 3 in psmoveapi/external/opencv and it worked fine. As long as the user builds OpenCV with cmake (as it will generate a file needed by find_package) I don't see any issues. I tested in Windows MinGW64-32 with local OpenCV, with OSX with a local opencv and homebrew opencv, and in ubuntu with local opencv and system opencv. find_package found it every time without me doing anything extra. I was able to build and run test_tracker in all the above systems.

Also note that I changed the PS3EYEDriver logic around a little so now it builds with MinGW64-32, so that's an option even without the CL Eye driver.

I'm still using include(path/CMakeLists.txt). As mentioned above, add_subdirectory works but only when called on the top level, and the src folder. It will take a lot of work to make it work when calling cmake on any other folder.

The other thing about add_subdirectory is that it is giving me an error if I try to build everything into the same folder. By building things into different folders, we end up with the shared libraries in build/src, the utils in build/src/utils, and there's also build/examples and build/bindings. None of the examples work when they are not in the same folder as the shared library, so we have to add another step to move things around (like make install).

So there are two options:

  1. Leave it as it is with include(path/CMakeLists.txt)
  2. Use add_subdirectory but now make install becomes mandatory.

I'm going to stop working on the CMake part and I'm going to use this version as my base for MSVC support.

@cboulay cboulay mentioned this pull request Jul 14, 2015
@thp
Copy link
Copy Markdown
Owner

thp commented Aug 12, 2015

What's the status here? Let me know when it's reviewable / mergeable.

…oved OpenCV searching/installing, instead rely on cmake find_package. Tested in Windows Mingw64-32 bit, Ubuntu local and system OpenCV, OSX local and system OpenCV.
@thp
Copy link
Copy Markdown
Owner

thp commented Jan 8, 2016

Superseded by #190

@thp thp closed this Jan 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants