One CMakeLists.txt for all samples #23
Conversation
Clang gave me this error:
explicit template specialization cannot have a storage class
It can be fixed by just removing `static` from the specializations, but
I don't understand why any of these were declared static in the first
place, so let's just remove it entirely.
This makes it easier for users, they now only have to configure one CMake project to build all samples. They can also open just one directory to open all samples in their IDE. Using one CMakeLists.txt also gets rid of a lot of code duplication. Many of the samples depend on third party libraries, in particular Eigen 3, OpenCV or PCL. The user might not be interested in installing all of these dependencies to get some samples to work, so we included options USE_EIGEN3 etc. to allow turning samples with certain dependencies on or off. The user can then choose between providing a third party library, or turning off samples depending on that library. An alternative approach was discussed, where all samples with third party dependencies were turned off by default, and the user would have to explicitly turn them on. We decided against this, since we are afraid they'll then simply never notice these samples at all. This approach with one CMakeLists.txt also solves the potential future problem of how to handle samples which have more than one dependency. In this case the previous approach with one directory per dependency would fall apart. Finally, we also introduce an option to turn off all samples which depend on Zivid::Vis3D, as this is currently hard to get to build on Linux. We'll hopefully solve that problem soon, but in the mean time it's convenient to be able to skip those samples.
knatten
force-pushed
the
cmake
branch
2 times, most recently
from
52ca0bc to
7906bd4
Compare
| endmacro() | ||
|
|
||
| if(USE_EIGEN3) | ||
| if(NOT DEFINED EIGEN3_INCLUDE_DIR) |
There was a problem hiding this comment.
Why don't you wait with find_package(.. REQUIRED ..) until here? Then you would get the official error message to the consumer and you dont have to invent your own, that needs to be kept in sync with Eigen's config/find script.
There was a problem hiding this comment.
Eigen is not using find_package, I assume you're referring to PCL and OpenCV? That's a good point.
There was a problem hiding this comment.
I agree that it's a good idea to move find_package inside if(USE....). And I assume you're referring to PCL and OpenCV only, since Eigen is not found by a CMake script.
In that case: There's nothing in those error messages which needs to be kept in sync with config/find scripts. OpenCV_DIR and PCL_DIR are not inputs to the OpenCV and PCL config files, they are arguments to CMake to know where to look for those config files in the first place. So I don't think this is actually a problem.
There's also another very valuable thing in the custom error messages: Letting the user know they can turn the dependency off with -DUSE_THETHING=OFF. So I think we should keep those messages.
My suggestion then is to move find_package inside if(USE_...), but don't add REQUIRED. You can then both see CMake's verbose message about not finding a config file, and our own message.
What do you think @nedrebo and @SatjaSivcev ?
There was a problem hiding this comment.
Made a draft PR #26 so you can see what I'm proposing.
There was a problem hiding this comment.
Cool, I added a better description and un-drafted it now. Thanks for the feedback! :)
| foreach(SAMPLE ${SAMPLES}) | ||
| get_filename_component(SAMPLE_NAME ${SAMPLE} NAME) | ||
|
|
||
| add_executable(${SAMPLE_NAME} ${SAMPLE}/${SAMPLE_NAME}.cpp) |
| ZividOpenCV/ZDF2OpenCV | ||
| Downsample) | ||
|
|
||
| set(Eigen3_DEPENDING Downsample) |
There was a problem hiding this comment.
There is no (guaranteed) correlation between this lists and the directory convention used above? The list below is more flexible than what can be expressed in the folder structure. Where are new samples supposed to be placed? For example one that depends on OpenCV and PCL.
There was a problem hiding this comment.
I discussed that with @SatjaSivcev a few days ago and agreed it's outside the scope of this PR. With this PR in place, they are now free to change the folder structure as they like.
| ZividPCL/CaptureWritePCLVis3D | ||
| ZividPCL/CaptureFromFileWritePCLVis3D | ||
| ZividPCL/ZDF2PCD | ||
| ZividOpenCV/ZDF2OpenCV |
There was a problem hiding this comment.
If you add a Sample here that depends on PCL and you forget to add it to the list below then the CI would still pass I guess. A safety mechanism preventing that should be added.
I think the CI should have a list of deps and first build without deps, then rerun cmake and add one and one of the options and build during each iteration. This should add almost nothing to total build time as it will be incremental builds.
There was a problem hiding this comment.
Yeah that's a potential problem. I've made a note of it in #15 .
|
Oh no, seems like I published my review after this was merged. I would still appreciate if the comments was answered or addressed. Sorry for the inconvenience @SatjaSivcev and @knatten. |
This makes it easier for users, they now only have to configure one
CMake project to build all samples. They can also open just one
directory to open all samples in their IDE.
Using one CMakeLists.txt also gets rid of a lot of code duplication.
Many of the samples depend on third party libraries, in particular Eigen
3, OpenCV or PCL. The user might not be interested in installing all of
these dependencies to get some samples to work, so we included options
USE_EIGEN3 etc. to allow turning samples with certain dependencies on or
off. The user can then choose between providing a third party library,
or turning off samples depending on that library.
An alternative approach was discussed, where all samples with third
party dependencies were turned off by default, and the user would have
to explicitly turn them on. We decided against this, since we are afraid
they'll then simply never notice these samples at all.
This approach with one CMakeLists.txt also solves the potential future
problem of how to handle samples which have more than one dependency. In
this case the previous approach with one directory per dependency would
fall apart.
Finally, we also introduce an option to turn off all samples which
depend on Zivid::Vis3D, as this is currently hard to get to build on
Linux. We'll hopefully solve that problem soon, but in the mean time
it's convenient to be able to skip those samples.