Generate and install a cmake configuration file, which can be #447

mmassing · 2023-03-17T15:45:25Z

automatically picked up by downstream cmake projects.

Downstream projects can link to Clipper2 or Clipper2Z by adding 'Clipper2::Clipper2' or 'Clipper2::Clipper2Z' as target dependency, and don't need to implement a FindPackage module.

AngusJohnson · 2023-03-17T21:54:08Z

Hi again Manuel.
Thank you for your PR. Since I'm completely unable to critique your changes (since my knowledge of CMake is negligible), I'm hoping one or two semi-regular Clipper2 contributors who are knowledgeable in CMake might comment before I merge.

sethhillbrand · 2023-03-24T04:49:19Z

@mmassing I think that you might need to include some documentation about how you propose that this be used. Are you suggesting that a downstream package pull Clipper2 as a git submodule and include the CmakeList that way or are you suggesting (as it seems to be) that the downstream project install Clipper2 locally and then include the generated CmakeList file instead of the base one here?

mmassing · 2023-03-28T09:44:42Z

Hi @sethhillbrand, this change will instruct CMake to export and install a cmake config-file. This is a concept similar to pkgconfig, where cmake provides information about exported targets, libraries, and include directories for Clipper2 by writing them to a config file. The patch exports the targets Clipper2::Clipper2 and Clipper2::Clipper2Z (apparently Clipper2Z is now optional, so I will need to adapt the patch for that).

Install step: E.g., under bash, build clipper2 locally, and perform a "system"-wide install:

mkdir -p build-clipper2; cd build-clipper2
cmake ${CLIPPER2_SOURCE} -DCMAKE_BUILD_TYPE=Debug
make -j && sudo make install

Downstream project usage is straightforward (cmake should pick the config file up automatically):

find_package(Clipper2 REQUIRED)

# Create executable which links against clipper2
# Note that include paths / compile flags / transitive dependencies are 
# automatically imported from the target
add_executable(test-clipper2 main.cpp)
target_link_libraries(test-clipper2 PRIVATE Clipper2::Clipper2)

add_executable(test-clipper2z main-with-z.cpp)
target_link_libraries(test-clipper2z PRIVATE Clipper2::Clipper2Z)

sethhillbrand · 2023-03-28T19:00:27Z

I'll be honest here that I can't imagine ever wanting to use Clipper2 in this fashion. That said, maybe someone does?

However, the install appears to pick out a GNU-centric destination that probably will have limited applicability. What is the expected behavior when using this proposed method on Mac or MSVC? Should this be gated?

mmassing · 2023-03-28T20:04:12Z

Hi Seth,

thanks for your feedback. Incorporating a cmake config file follows modern CMake practice and helps to streamline dependency management and integration with other projects. It is more flexible and portable than relying on library-specific cmake-modules (the "old-fashioned" way). You may of course still choose to not use cmake for your downstream projects, or integrate Clipper2 as a subproject.
Although my example used bash command-line syntax, cmake config files are not GNU-centric. The config files work across different OS and architectures, as described in the CMake documentation: https://cmake.org/cmake/help/latest/command/find_package.html#config-mode-search-procedure
CMake config files are particularly useful for building platforms in cross-compilation scenarios, non-system wide installation prefixes, and containerized environments such as Docker (to provide a "builder" platform image).
I hope this provides a clearer understanding of the benefits of the proposed cmake config file patch for Clipper2.

cheers, Manuel

sethhillbrand

Well, as I mentioned, others may find this useful and it doesn't appear to impact normal usage. I've left a couple of comments that you might look over.

sethhillbrand · 2023-03-28T20:08:59Z

CPP/CMakeLists.txt

+include(CMakePackageConfigHelpers)
+include(GNUInstallDirs)
+
+write_basic_package_version_file("${PROJECT_BINARY_DIR}/Clipper2ConfigVersion.cmake" COMPATIBILITY AnyNewerVersion)


Because the API changes between even minor versions of Clipper2, this should be "ExactVersion"

You're correct in principle, but the prevailing practice is to be lenient for the configuration script, since the downstream is free to add additional constraints - e.g. request a specific version using
find_package(Clipper2 1.2.3 EXACT REQUIRED), or restricting to e.g. a specific minor version: find_package(Clipper2 1.2 REQUIRED). SameMajorVersion might be sensible, though.

Fair enough.

sethhillbrand · 2023-03-28T20:09:54Z

CPP/CMakeLists.txt

@@ -212,15 +216,52 @@ endif()
  file(COPY ../Tests/Polygons.txt DESTINATION ${CMAKE_BINARY_DIR} FILE_PERMISSIONS OWNER_READ GROUP_READ WORLD_READ )
 endif()

+# create package config file
+include(CMakePackageConfigHelpers)
+include(GNUInstallDirs)


This shouldn't be used for MSVC or MacOS builds (not sure if there is an alternative available for them

I guess you are concererned about GNUInstallDirs? CMake provides it to establish a convention for install directories (${CMAKE_INSTALL_INCLUDEDIR}, etc.), and contrary to what the name suggests it can be used on all platforms (e.g. Microsofts' own vcpkg uses this convention).

AngusJohnson · 2023-03-28T21:14:43Z

@mmassing in case you're wondering, Seth is very kindly acting as my proxy here (at my request) because of my very limited knowledge of CMake.

LazyDodo · 2024-04-01T23:02:02Z

Stumbled upon this shortly before sending an almost identical PR, this PR seemed well on it's way, and i'm a bit confused on the sudden closure, is this worth dusting off? or were there fundamental issues at hand?

Context: i'm the windows platform dev for blender (CMake based, pkg-config doesn't exist in the stock microsoft tooling), and we're looking to take on a new dependency on manifold which is mostly cmake based, except for where they used pkg-config to find Clipper2 since it exports no cmake config, so i'm looking to fix this issue in Clipper2, so I can fix the issue in manifold, so i ..... yeah... fun times... while I can easily fix this locally with some patches, it's nicer not having to maintain patches and lending a hand upstream.

What is the expected behavior when using this proposed method on Mac or MSVC? Should this be gated?

No need to gate it, CMake config files aren't specific to MSVC, It'll will work across all platforms supported by CMake, I see no problem with generating both formats for all platforms (the msys/mingw based toolchains on windows do have pkg-config, so there will be people that'll appreciate support being there), people can pick and use whatever they prefer.

AngusJohnson · 2024-04-02T05:59:53Z

and i'm a bit confused on the sudden closure, is this worth dusting off? or were there fundamental issues at hand?

I only closed this PR when there seemed little interest in or need for it. (And I have enough problems trying to understand what's happening in CMake without making it more complex. And every time I change something, I potentially break something.) Nevertheless, if enough people will find this helpful and (the changes not too radical), then I'd be happy to accommodate them.

LazyDodo

minor fixes required to build with CLIPPER2_USINGZ=OFF

CPP/CMakeLists.txt

mmassing · 2024-04-03T11:30:10Z

@LazyDodo thanks for picking this back up again, somehow got lost by the wayside! Are you currently working on this? Otherwise I'm happy to update the PR with Seths suggestions, and handle the USINGZ case.

…ically picked up by downstream cmake projects. Downstream projects can link to Clipper2 or Clipper2Z by adding 'Clipper2::Clipper2' or 'Clipper2::Clipper2Z' as target dependency, and don't need to implement a FindPackage module.

AngusJohnson · 2024-04-06T11:26:05Z

If everyone contributing is now happy with the current state of this PR, I'll press the merge button.

sethhillbrand reviewed Mar 28, 2023

View reviewed changes

AngusJohnson closed this Apr 8, 2023

AngusJohnson reopened this Apr 2, 2024

LazyDodo suggested changes Apr 2, 2024

View reviewed changes

CPP/CMakeLists.txt Outdated Show resolved Hide resolved

CPP/CMakeLists.txt Outdated Show resolved Hide resolved

mmassing force-pushed the feature/generate-and-install-cmake-config branch from 4168fb5 to b7f22c6 Compare April 3, 2024 12:43

mmassing force-pushed the feature/generate-and-install-cmake-config branch from b7f22c6 to a3510c6 Compare April 3, 2024 12:43

mmassing added 2 commits April 3, 2024 14:59

Fix package config for Clipper2Z

799cda1

Be a little less lenient when version-checking the configuration file.

3ad9cba

LazyDodo approved these changes Apr 6, 2024

View reviewed changes

AngusJohnson merged commit 0135062 into AngusJohnson:main Apr 6, 2024
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generate and install a cmake configuration file, which can be #447

Generate and install a cmake configuration file, which can be #447

mmassing commented Mar 17, 2023

AngusJohnson commented Mar 17, 2023 •

edited

Loading

sethhillbrand commented Mar 24, 2023

mmassing commented Mar 28, 2023 •

edited

Loading

sethhillbrand commented Mar 28, 2023

mmassing commented Mar 28, 2023

sethhillbrand left a comment

sethhillbrand Mar 28, 2023

mmassing Apr 3, 2024

sethhillbrand Apr 3, 2024

sethhillbrand Mar 28, 2023

mmassing Apr 4, 2024

AngusJohnson commented Mar 28, 2023 •

edited

Loading

LazyDodo commented Apr 1, 2024

AngusJohnson commented Apr 2, 2024 •

edited

Loading

LazyDodo left a comment

mmassing commented Apr 3, 2024

AngusJohnson commented Apr 6, 2024

Generate and install a cmake configuration file, which can be #447

Generate and install a cmake configuration file, which can be #447

Conversation

mmassing commented Mar 17, 2023

AngusJohnson commented Mar 17, 2023 • edited Loading

sethhillbrand commented Mar 24, 2023

mmassing commented Mar 28, 2023 • edited Loading

sethhillbrand commented Mar 28, 2023

mmassing commented Mar 28, 2023

sethhillbrand left a comment

Choose a reason for hiding this comment

sethhillbrand Mar 28, 2023

Choose a reason for hiding this comment

mmassing Apr 3, 2024

Choose a reason for hiding this comment

sethhillbrand Apr 3, 2024

Choose a reason for hiding this comment

sethhillbrand Mar 28, 2023

Choose a reason for hiding this comment

mmassing Apr 4, 2024

Choose a reason for hiding this comment

AngusJohnson commented Mar 28, 2023 • edited Loading

LazyDodo commented Apr 1, 2024

AngusJohnson commented Apr 2, 2024 • edited Loading

LazyDodo left a comment

Choose a reason for hiding this comment

mmassing commented Apr 3, 2024

AngusJohnson commented Apr 6, 2024

AngusJohnson commented Mar 17, 2023 •

edited

Loading

mmassing commented Mar 28, 2023 •

edited

Loading

AngusJohnson commented Mar 28, 2023 •

edited

Loading

AngusJohnson commented Apr 2, 2024 •

edited

Loading