CMake: Support external NanoSVG library (PR #22407)
Maarten
Partially based on #22393, but it leaves the other build systems alone.
Support both header-only NanoSVG library, and NanoSVG library that already have the functions implemented.
Since upstream NanoSVG doesn't have CMake support, there is no official target name or namespace to check. Currently it checks for NanoSVG::nanosvg and NanoSVG::nanosvgrast, but this list can easily be extended.
Default CMake build still uses the old method (including ../../3rdparty/nanosvg/src/nanosvg.h), but it can also use external build without implementation using:
set(wxUSE_NANOSVG_EXTERNAL 1 PARENT_SCOPE) set(NANOSVG_LIBRARIES ) set(NANOSVG_INCLUDE_DIRS "${wxSOURCE_DIR}/3rdparty/nanosvg/src")
You can view, comment on, or merge this pull request online at:
https://github.com/wxWidgets/wxWidgets/pull/22407
Commit Summary
- fb8e860 CMake: Include third-party library directories when they are defined
- c8bfd53 CMake: Fix using Windows path separator in wxINSTALL_PREFIX
- 931370f Add NanoSVG setup options
- 62628d4 CMake: Add support for external NanoSVG library
- 3aaeb5a CMake: Enable NanoSVG defines when NanoSVG library is header-only
- 6d91229 CMake: Check all known NanoSVG target names
File Changes
(17 files)
- M build/cmake/functions.cmake (2)
- M build/cmake/lib/CMakeLists.txt (2)
- M build/cmake/lib/core/CMakeLists.txt (16)
- A build/cmake/lib/nanosvg.cmake (40)
- M build/cmake/options.cmake (1)
- M build/cmake/setup.cmake (2)
- M build/cmake/setup.h.in (4)
- M include/wx/android/setup.h (6)
- M include/wx/features.h (2)
- M include/wx/gtk/setup.h (6)
- M include/wx/motif/setup.h (6)
- M include/wx/msw/setup.h (13)
- M include/wx/osx/setup.h (6)
- M include/wx/setup_inc.h (6)
- M include/wx/univ/setup.h (6)
- M setup.h.in (4)
- M src/generic/bmpsvg.cpp (12)
Patch Links:
- https://github.com/wxWidgets/wxWidgets/pull/22407.patch
- https://github.com/wxWidgets/wxWidgets/pull/22407.diff
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- 3ee76d7 Add --with-nanosvg option to configure
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
Thanks, this looks good to me, but I think wxUSE_NANOSVG_EXTERNAL and DISABLE_IMPL should be documented in docs/doxygen/mainpages/const_wxusedef.h as they could be useful for people building wx without CMake too.
In build/cmake/functions.cmake:
> @@ -601,7 +601,7 @@ function(wx_add_thirdparty_library var_name lib_name help_str)
# If the sys library can not be found use builtin
find_package(${lib_name})
string(TOUPPER ${lib_name} lib_name_upper)
- if(NOT ${${lib_name_upper}_FOUND})
+ if(NOT ${lib_name_upper}_FOUND AND NOT ${lib_name}_FOUND)
I just wonder, what is the CMake convention here in general? Is it supposed to always accept both libfoo_FOUND and LIBFOO_FOUND interchangeably?
In configure.in:
> @@ -556,6 +556,7 @@ WX_ARG_WITH(gnomevfs, [ --with-gnomevfs use GNOME VFS for associat WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifications], wxUSE_LIBNOTIFY) WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) +WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG)
I think this should be
⬇️ Suggested change-WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG) +WX_ARG_WITH(nanosvg, [ --without-nanosvg don't use NanoSVG for rasterizing SVG], wxUSE_NANOSVG)
because it is used by default.
In configure.in:
> @@ -2956,6 +2957,14 @@ else
wxUSE_XML=no
fi
+dnl ------------------------------------------------------------------------
+dnl Check for NanoSVG libraries
⬇️ Suggested change
-dnl Check for NanoSVG libraries +dnl Use NanoSVG unless explicitly disabled
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/functions.cmake:
> @@ -601,7 +601,7 @@ function(wx_add_thirdparty_library var_name lib_name help_str)
# If the sys library can not be found use builtin
find_package(${lib_name})
string(TOUPPER ${lib_name} lib_name_upper)
- if(NOT ${${lib_name_upper}_FOUND})
+ if(NOT ${lib_name_upper}_FOUND AND NOT ${lib_name}_FOUND)
It depends on the package name. For example wxWidgets has wxWidgets_FOUND, and jpeg has JPEG_FOUND.
The packages we used so far all have capitalized names, but NanoSVG probably looks like NanoSVG_FOUND.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In build/cmake/functions.cmake:
> @@ -601,7 +601,7 @@ function(wx_add_thirdparty_library var_name lib_name help_str)
# If the sys library can not be found use builtin
find_package(${lib_name})
string(TOUPPER ${lib_name} lib_name_upper)
- if(NOT ${${lib_name_upper}_FOUND})
+ if(NOT ${lib_name_upper}_FOUND AND NOT ${lib_name}_FOUND)
So what you're saying is that there is no real standard/convention?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In configure.in:
> @@ -556,6 +556,7 @@ WX_ARG_WITH(gnomevfs, [ --with-gnomevfs use GNOME VFS for associat WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifications], wxUSE_LIBNOTIFY) WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) +WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG)
All the external libraries above have --with-lib format, and are enabled / searched for by default. Why not keep this the same?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/functions.cmake:
> @@ -601,7 +601,7 @@ function(wx_add_thirdparty_library var_name lib_name help_str)
# If the sys library can not be found use builtin
find_package(${lib_name})
string(TOUPPER ${lib_name} lib_name_upper)
- if(NOT ${${lib_name_upper}_FOUND})
+ if(NOT ${lib_name_upper}_FOUND AND NOT ${lib_name}_FOUND)
I can't find any convention, they use <PackageName>_FOUND found in the documentation.
But I think we could remove ${lib_name_upper}_FOUND since all names are already in the correct capitalization.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
@vadz commented on this pull request.
In configure.in:
> @@ -556,6 +556,7 @@ WX_ARG_WITH(gnomevfs, [ --with-gnomevfs use GNOME VFS for associat WX_ARG_WITH(libnotify, [ --with-libnotify use libnotify for notifications], wxUSE_LIBNOTIFY) WX_ARG_WITH(opengl, [ --with-opengl use OpenGL (or Mesa)], wxUSE_OPENGL) WX_ARG_WITH(xtest, [ --with-xtest use XTest extension], wxUSE_XTEST) +WX_ARG_WITH(nanosvg, [ --with-nanosvg use NanoSVG for rasterizing SVG], wxUSE_NANOSVG)
My reasoning was that we could fail to find these libraries, so for them this could somewhat make sense, but NanoSVG is always available, so it doesn't seem to be very useful to explicitly enable it.
But maybe you're right and it's better to keep it consistent even if it's a bit different. OTOH we do have --without-gtkprint already. Oh well, I guess we can leave it like this if you prefer it, there is no real consistency here anyhow.
On a tangentially related reason, the behaviour that would make most sense to me would be to have --with-foo require foo and fail if it's not available, --without-foo not use it and default (or some --with-foo=auto) behave as --with-foo does now. But this would almost certainly run into compatibility problems and break builds that worked just fine before because some exotic dependency (like libmspack or something) is not available. So in practice the only thing we could do would be to extend WX_ARG_WITH to allow specifying --with-foo=required.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 2 commits.
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks! Should this be merged now?
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Mark Roszko
Can 'unofficial::nanosvg' be added as one of the target candidates? Its what is part of vcpkg now
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Yes, I'll add it.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 1 commit.
- d878165 CMake: Add unofficial::nanosvg target name
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Thanks! Should this be merged now?
Yes please, it is ready.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
Hello! So NanoSVG now has CMake memononen/nanosvg#209
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
@tamasmeszaros commented on this pull request.
In build/cmake/lib/nanosvg.cmake:
> +############################################################################# + +if(wxUSE_NANOSVG STREQUAL "builtin") + set(wxUSE_NANOSVG_EXTERNAL 0 PARENT_SCOPE) +elseif(wxUSE_NANOSVG) + set(wxUSE_NANOSVG_EXTERNAL 1 PARENT_SCOPE) + + set(NANOSVG_LIBRARIES ) + set(NANOSVG_INCLUDE_DIRS ) + + find_package(NanoSVG REQUIRED) + + set(wxUSE_NANOSVG_EXTERNAL_DISABLE_IMPL FALSE) + + foreach(TARGETNAME NanoSVG::nanosvg NanoSVG::nanosvgrast unofficial::nanosvg) + if(NOT TARGETNAME)
This line fails for me but works if I change it to if (NOT TARGET TARGETNAME)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent commented on this pull request.
In build/cmake/lib/nanosvg.cmake:
> +############################################################################# + +if(wxUSE_NANOSVG STREQUAL "builtin") + set(wxUSE_NANOSVG_EXTERNAL 0 PARENT_SCOPE) +elseif(wxUSE_NANOSVG) + set(wxUSE_NANOSVG_EXTERNAL 1 PARENT_SCOPE) + + set(NANOSVG_LIBRARIES ) + set(NANOSVG_INCLUDE_DIRS ) + + find_package(NanoSVG REQUIRED) + + set(wxUSE_NANOSVG_EXTERNAL_DISABLE_IMPL FALSE) + + foreach(TARGETNAME NanoSVG::nanosvg NanoSVG::nanosvgrast unofficial::nanosvg) + if(NOT TARGETNAME)
This is indeed the correct syntax, I don't know why the other one works for me too. I'll update the PR.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 5 commits.
- 5d0428a CMake: Add support for external NanoSVG library
- 071e936 CMake: Support header-only NanoSVG library
- 10e3339 CMake: Check all known NanoSVG target names
- b6e8127 Add --with-nanosvg option to configure
- 817b060 Document the NanoSVG options
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
@MaartenBent pushed 5 commits.
- 603c13a CMake: Add support for external NanoSVG library
- 138d1ab CMake: Support header-only NanoSVG library
- 86b3360 CMake: Check all known NanoSVG target names
- 760bfaa Add --with-nanosvg option to configure
- ce32faa Document the NanoSVG options
—
View it on GitHub or unsubscribe.
You are receiving this because you are subscribed to this thread.
Maarten
Hello! So nanosvg now has CMake memononen/nanosvg#209
Thanks for your work on this. But it is probably useful to enable NANOSVG_ALL_COLOR_KEYWORDS by default in this CMake build, since users won't have the option anymore.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Thanks again, I'm finally merging this.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
VZ
Merged #22407 into master.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
tamasmeszaros
@tamasmeszaros commented on this pull request.
In build/cmake/lib/nanosvg.cmake:
> +############################################################################# + +if(wxUSE_NANOSVG STREQUAL "builtin") + set(wxUSE_NANOSVG_EXTERNAL 0 PARENT_SCOPE) +elseif(wxUSE_NANOSVG) + set(wxUSE_NANOSVG_EXTERNAL 1 PARENT_SCOPE) + + set(NANOSVG_LIBRARIES ) + set(NANOSVG_INCLUDE_DIRS ) + + find_package(NanoSVG REQUIRED) + + set(wxUSE_NANOSVG_EXTERNAL_DISABLE_IMPL FALSE) + + foreach(TARGETNAME NanoSVG::nanosvg NanoSVG::nanosvgrast unofficial::nanosvg) + if(NOT TARGETNAME)
CMake can differ between versions, or who knows :)
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.
Vojtěch Bubník
Great, thanks to all involved!
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.