Skip to content
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.

Sign up for GitHub

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

fix: add options for optional deps and find them after installation #354

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

fix: add options for optional deps and find them after installation #354

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c7f669f
fix: add options for optional deps and find them after installation
aminya Jun 14, 2023
666da20
fix: fix the PNG option
aminya Jun 14, 2023
17056e3
fix: only use the old style dependencies if target is not defined
aminya Jun 14, 2023
6f6a531
fix: fix nodesoup installation
aminya Jun 14, 2023
914c869
fix: ensure that all the required libraries are exported
aminya Jun 14, 2023
92b806b
fix: fix finding FFTW3
aminya Jun 14, 2023
ed3d1c4
fix: fix finding the libpng
aminya Jun 14, 2023
99c3dea
fix: directly link dependencies instead of aliasing
aminya Jun 14, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_TIFF "Require building with TIFF support" OFF)
option(MATPLOTPP_WITH_PNG "Require building with PNG support" OFF)
option(MATPLOTPP_WITH_LAPACK "Require building with LAPACK support" OFF)
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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_SYSTEM_NODESOUP "Use system-provided nodesoup instead of bundled" OFF)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down
13 changes: 13 additions & 0 deletions Matplot++Config.cmake.in
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,24 @@ if (NOT CMAKE_CXX_COMPILER_ID STREQUAL MATPLOT_BUILT_CXX_COMPILER_ID)
endif()

# Find dependencies

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)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

foreach(MATPLOT_OPTIONAL_DEPENDENCY ${MATPLOT_OPTIONAL_DEPENDENCIES})
if (@MATPLOTPP_WITH_${MATPLOT_OPTIONAL_DEPENDENCY}@)
find_dependency(${MATPLOT_OPTIONAL_DEPENDENCY} REQUIRED)
else()
find_dependency(${MATPLOT_OPTIONAL_DEPENDENCY})
endif()
endforeach()

find_dependency(Filesystem COMPONENTS Experimental Final)

# OpenGL backend
if (@MATPLOTPP_BUILD_EXPERIMENTAL_OPENGL_BACKEND@)
find_dependency(glad)
Expand Down
107 changes: 85 additions & 22 deletions source/3rd_party/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
set(EXPORTED_TARGETS)

#######################################################
### NodeSoup ###
#######################################################
Expand Down Expand Up @@ -44,14 +46,17 @@ endif()
if(MASTER_PROJECT AND NOT BUILD_SHARED_LIBS)
install(TARGETS nodesoup
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the implications of this change?

EXPORT Matplot++Targets
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}/Matplot++)
RUNTIME DESTINATION bin
LIBRARY DESTINATION lib
ARCHIVE DESTINATION lib)
endif()


#######################################################
### CImg ###
#######################################################
add_library(cimg INTERFACE)
list(APPEND EXPORTED_TARGETS cimg)
if(WITH_SYSTEM_CIMG)
find_path(CIMG_INCLUDE_DIR CImg.h REQUIRED)
else()
Expand All @@ -69,57 +74,115 @@ find_package(PkgConfig)
# Lots of optional packages are not a good idea in general.
# It makes the library much less "packagable" (https://youtu.be/sBP17HQAQjk)
# and much more difficult to make sure it works on multiple OSs
find_package(JPEG)

if(MATPLOTPP_WITH_JPEG)
find_package(JPEG REQUIRED)
else()
find_package(JPEG QUIET)
endif()
if(JPEG_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_jpeg)
target_link_libraries(cimg INTERFACE ${JPEG_LIBRARIES})
target_include_directories(cimg INTERFACE ${JPEG_INCLUDE_DIRS})
endif()

find_package(TIFF)
if(MATPLOTPP_WITH_TIFF)
find_package(TIFF REQUIRED)
else()
find_package(TIFF QUIET)
endif()
if(TIFF_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_tiff)
target_link_libraries(cimg INTERFACE ${TIFF_LIBRARIES})
target_include_directories(cimg INTERFACE ${TIFF_INCLUDE_DIRS})
endif()

find_package(ZLIB)
if(ZLIB_FOUND)
find_package(PNG)
if (PNG_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_zlib cimg_use_png)
target_include_directories(cimg INTERFACE ${ZLIB_INCLUDE_DIRS} ${PNG_INCLUDE_DIRS})
target_link_libraries(cimg INTERFACE ${ZLIB_LIBRARIES} ${PNG_LIBRARIES})
endif ()
if(MATPLOTPP_WITH_PNG)
find_package(ZLIB REQUIRED)
find_package(libpng)
if(NOT libpng_FOUND)
find_package(PNG REQUIRED)
endif()
else()
find_package(ZLIB QUIET)
find_package(libpng QUIET)
if(NOT libpng_FOUND)
find_package(PNG QUIET)
endif()
endif()
if(ZLIB_FOUND AND (libpng_FOUND OR PNG_FOUND))
target_compile_definitions(cimg INTERFACE cimg_use_zlib cimg_use_png)
if (TARGET ZLIB::ZLIB)
target_link_libraries(cimg INTERFACE ZLIB::ZLIB)
else()
target_include_directories(cimg INTERFACE ${ZLIB_INCLUDE_DIRS})
target_link_libraries(cimg INTERFACE ${ZLIB_LIBRARIES})
endif()
if (TARGET png)
target_link_libraries(cimg INTERFACE png)
else()
target_include_directories(cimg INTERFACE ${libpng_INCLUDE_DIRS} ${PNG_INCLUDE_DIRS})
target_link_libraries(cimg INTERFACE ${libpng_LIBRARIES} ${PNG_LIBRARIES})
endif()
endif()

find_package(LAPACK)
if(MATPLOTPP_WITH_LAPACK)
find_package(LAPACK REQUIRED)
else()
find_package(LAPACK QUIET)
endif()
if(LAPACK_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_lapack)
target_link_libraries(cimg INTERFACE ${LAPACK_LIBRARIES})
target_include_directories(cimg INTERFACE ${LAPACK_INCLUDE_DIRS})
endif()

find_package(BLAS)
if(MATPLOTPP_WITH_BLAS)
find_package(BLAS REQUIRED)
else()
find_package(BLAS QUIET)
endif()
if(BLAS_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_blas)
target_link_libraries(cimg INTERFACE ${BLAS_LIBRARIES})
target_include_directories(cimg INTERFACE ${BLAS_INCLUDE_DIRS})
endif()

find_package(FFTW)
if(FFTW_FOUND)
if(MATPLOTPP_WITH_FFTW)
find_package(FFTW3)
if(NOT FFTW3_FOUND)
find_package(FFTW REQUIRED)
endif()
else()
find_package(FFTW3 QUIET)
if(NOT FFTW3_FOUND)
find_package(FFTW QUIET)
endif()
endif()
if(FFTW3_FOUND OR FFTW_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_fftw3)
target_link_libraries(cimg INTERFACE ${FFTW_LIBRARIES})
target_include_directories(cimg INTERFACE ${FFTW_INCLUDE_DIRS})
if (TARGET FFTW3::fftw3)
target_link_libraries(cimg INTERFACE FFTW3::fftw3)
else()
target_include_directories(cimg INTERFACE ${FFTW3_INCLUDE_DIRS} ${FFTW_INCLUDE_DIRS})
target_link_libraries(cimg INTERFACE ${FFTW3_LIBRARIES} ${FFTW_LIBRARIES})
endif()
endif()

if (CMAKE_MODULE_PATH)
find_package(OpenCV QUIET)
if(MATPLOTPP_WITH_OpenCV)
find_package(OpenCV REQUIRED)
else()
find_package(OpenCV QUIET)
endif()
if (OpenCV_FOUND)
target_compile_definitions(cimg INTERFACE cimg_use_opencv)
target_link_libraries(cimg INTERFACE ${OpenCV_LIBRARIES})
target_include_directories(cimg INTERFACE ${OpenCV_INCLUDE_DIRS})
if(TARGET opencv_core)
target_link_libraries(cimg INTERFACE opencv_core)
else()
target_include_directories(cimg INTERFACE ${OpenCV_INCLUDE_DIRS})
target_link_libraries(cimg INTERFACE ${OpenCV_LIBRARIES})
endif()
endif()
else()
message("No CMAKE_MODULE_PATH path for OpenCV configured")
Expand Down Expand Up @@ -151,6 +214,6 @@ endif()

# Install (only necessary for static lib build)
if(MASTER_PROJECT AND NOT BUILD_SHARED_LIBS)
install(TARGETS cimg
install(TARGETS ${EXPORTED_TARGETS}
EXPORT Matplot++Targets)
endif()
endif()
33 changes: 17 additions & 16 deletions source/matplot/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ add_library(matplot
freestanding/plot.h
)

set(TARGETS matplot)
set(EXPORTED_TARGETS matplot)

# Target aliases
add_library(Matplot++::matplot ALIAS matplot)
Expand Down Expand Up @@ -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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😥

backend/opengl_embed.h
backend/opengl_embed.cpp
backend/opengl.h
backend/opengl.cpp
)

find_package(OpenGL)

# https://github.com/Dav1dde/glad
Expand All @@ -226,13 +233,13 @@ if (MATPLOTPP_BUILD_EXPERIMENTAL_OPENGL_BACKEND)
# find_package(GLAD REQUIRE) would suffice if it worked well
FetchContent_Declare(glad GIT_REPOSITORY https://github.com/Dav1dde/glad.git GIT_TAG v0.1.36)
FetchContent_MakeAvailable(glad)
list(APPEND EXPORTED_TARGETS glad)
endif()
if(TARGET glad AND NOT TARGET glad::glad)
# Alias glad to glad::glad
add_library(glad::glad ALIAS glad)
list(APPEND TARGETS glad)
endif()
if(NOT TARGET glad::glad)
if(TARGET glad::glad)
target_link_libraries(matplot_opengl PUBLIC glad::glad)
elseif(TARGET glad)
target_link_libraries(matplot_opengl PUBLIC glad)
else()
# FindGLAD does not usually create a target, so we create an interface target
add_library(glad::glad INTERFACE)
target_include_directories(glad::glad INTERFACE ${GLAD_INCLUDE_PATH})
Expand All @@ -248,23 +255,17 @@ if (MATPLOTPP_BUILD_EXPERIMENTAL_OPENGL_BACKEND)
FetchContent_MakeAvailable(glfw3)
endif()

add_library(matplot_opengl
backend/opengl_embed.h
backend/opengl_embed.cpp
backend/opengl.h
backend/opengl.cpp
)
target_link_libraries(matplot_opengl PUBLIC matplot glad::glad glfw ${CMAKE_DL_LIBS})
target_link_libraries(matplot_opengl PUBLIC matplot glfw ${CMAKE_DL_LIBS})

list(APPEND TARGETS matplot_opengl)
list(APPEND EXPORTED_TARGETS matplot_opengl)
endif()

#######################################################
### Installer ###
#######################################################
if (MATPLOTPP_BUILD_INSTALLER)
# Install targets
install(TARGETS ${TARGETS}
install(TARGETS ${EXPORTED_TARGETS}
EXPORT Matplot++Targets
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
Expand Down