fix: add options for optional deps and find them after installation #354
Conversation
This adds options for the optional dependencies to require building them if needed. This makes sure that the library is built with a require feature. It also fixes an issue where these dependencies where not being found in the config file after installation
Fixes exporting issues
| @@ -53,6 +53,15 @@ option(MATPLOTPP_BUILD_HIGH_RESOLUTION_WORLD_MAP "Compile the high resolution ma | |||
| option(MATPLOTPP_BUILD_FOR_DOCUMENTATION_IMAGES "Bypass show() commands and save figures as .svg at destruction" OFF) | |||
| option(MATPLOTPP_BUILD_EXPERIMENTAL_OPENGL_BACKEND "Compile target with the experimental OpenGL backend" OFF) | |||
|
|
|||
| # Optional dependencies | |||
| option(MATPLOTPP_WITH_JPEG "Require building with JPEG support" OFF) | |||
There was a problem hiding this comment.
If any dependencies are going to fall back to fetch content (I haven't looked at it yet), we can just make them required dependencies. These optional dependencies are technical debt since they create an exponential explosion of test factors and the library is never properly tested.
We can make JPEF, TIFF, and PNG required, and then get rid of LAPACK, BLAS, FFTW, and OpenCV. There's no LAPACK, BLAS, FFTW, and OpenCV support in the library. These are just transitive optional dependencies, which are even worse than optional dependencies.
| option(MATPLOTPP_WITH_BLAS "Require building with BLAS support" OFF) | ||
| option(MATPLOTPP_WITH_FFTW "Require building with FFTW support" OFF) | ||
| option(MATPLOTPP_WITH_OpenCV "Require building with OpenCV support" OFF) | ||
|
|
||
| # Where to find dependencies | ||
| option(MATPLOTPP_WITH_SYSTEM_CIMG "Use system-provided CImg.h instead of bundled" OFF) |
There was a problem hiding this comment.
These could be merged into the options above since there's nothing special about them.
We can easily have find_package falling back to fetch content for these two. The bundled version is not useful at all anymore. The whole third_party directory is something from the past. It doesn't make sense in 2023 with vcpkg and all the alternatives we have now.
| option(MATPLOTPP_WITH_BLAS "Require building with BLAS support" OFF) | ||
| option(MATPLOTPP_WITH_FFTW "Require building with FFTW support" OFF) | ||
| option(MATPLOTPP_WITH_OpenCV "Require building with OpenCV support" OFF) | ||
|
|
||
| # Where to find dependencies | ||
| option(MATPLOTPP_WITH_SYSTEM_CIMG "Use system-provided CImg.h instead of bundled" OFF) | ||
| option(MATPLOTPP_WITH_SYSTEM_NODESOUP "Use system-provided nodesoup instead of bundled" OFF) |
There was a problem hiding this comment.
This option got here through a contribution but it doesn't make much sense either. Zero people have "system nodesoup" installed. nodesoup was a POC library that happened to be very useful here. It's not being maintained at all and should become part of matplot++. I talked to the author and he recommended the source files are just moved into the matplot++ source files into some detail directory and compiled with everything else.
| if(NOT ${MATPLOT_BUILT_SHARED}) | ||
| include(CMakeFindDependencyMacro) | ||
| list(APPEND CMAKE_MODULE_PATH ${MATPLOT_CONFIG_INSTALL_DIR}) | ||
| list(APPEND CMAKE_MODULE_PATH "${CMAKE_CURRENT_LIST_DIR}") | ||
|
|
||
| # optional dependencies | ||
| set(MATPLOT_OPTIONAL_DEPENDENCIES JPEG TIFF ZLIB PNG LAPACK BLAS FFTW3 OpenCV) |
There was a problem hiding this comment.
The solution has to be more complex than that, which is another drawback of optional dependencies.
find_dependency will forward the parameters QUIET and REQUIRED which were passed to the original find_package() call, so the REQUIRED parameter below is not really fixing the issue.
This means once the library is installed, there are no more optional dependencies. In the config template:
- The optional dependencies that were not found should have no corresponding find_dependency
- The optional dependencies that were found should have a corresponding find_dependency
- The optional dependencies that were fetched should be installed and added to the targets to export
Of course, another solution is using find_package, but that makes things even more complex because we have to replicate part of the build scripts in the config script.
| @@ -44,14 +46,17 @@ endif() | |||
| if(MASTER_PROJECT AND NOT BUILD_SHARED_LIBS) | |||
| install(TARGETS nodesoup | |||
There was a problem hiding this comment.
What are the implications of this change?
| @@ -214,6 +214,13 @@ if (MATPLOTPP_BUILD_EXPERIMENTAL_OPENGL_BACKEND) | |||
| # The biggest con of the OpenGL backend is that it cannot open a window | |||
| # in another thread. All it can do is get in the middle of the render | |||
| # loop and draw the plot. | |||
| add_library(matplot_opengl | |||
This adds options for the optional dependencies to require building them if needed. This makes sure that the library is built with a required feature.
It also fixes an issue where these dependencies were not being found in the config file after installation
Linked vcpkg pull request:
microsoft/vcpkg#31229