Retrieve required flags from Libnest2D target

[StefanBruens]

Instead of setting include paths and libs manually, just use the imported target.
All required properties are set Libnest2DTargets.cmake.

This also adds the otherwise missing libpthread to the link libraries.

[thopiekar]

@Ghostkeeper This PR is open for some time. Is this one on your board to be reviewed?

[Ghostkeeper]

Yeah, I've seen it. There are still older ones I'd like to handle first.

[StefanBruens]

Can this be merged, and a 4.9.0 release tagged?

[Ghostkeeper]

We're making a build environment that includes this change on our build servers, to properly test it. The changes themselves look good to me (and work locally in my own environment).

4.9.0 and 4.9.1 have been tagged. They are the same. I think this repository wasn't in @jellespijker's script yet when he tagged them.

[Ghostkeeper]

The merge conflict is resolved on the https://github.com/Ultimaker/pynest2d/tree/StefanBruens-cmake_use_imported_target branch.

[Ghostkeeper]

This seems to fail to build, on Windows only. We're getting this result:

-- Found PythonInterp: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/bin/python.exe (found suitable version "3.8.8", minimum required is "3.5") 
-- Found PythonLibs: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/libs/python38.lib (found suitable version "3.8.8", minimum required is "3.5") 
-- Found SIP: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/sip.exe (found version "4.19.24") 
-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
-- Found LIBNEST2D: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include  
-- Configuring done
CMake Error at cmake/SIPMacros.cmake:112 (add_library):
  Target "python_module_pynest2d" links to target
  "Libnest2D::libnest2d_headeronly" but the target was not found.  Perhaps a
  find_package() call is missing for an IMPORTED target, or an ALIAS target
  is missing?
Call Stack (most recent call first):
  CMakeLists.txt:56 (add_sip_python_module)

The package config file does exist, in inst\lib\cmake\Libnest2D\Libnest2DTargets.cmake. And inside it does seem to expect the Libnest2d::libnest2d_headeronly target.

This is the snippet from the log where libnest2d was installed (showing nothing out of the ordinary):

[35m�[1mScanning dependencies of target libnest2d�[0m

[ 32%] �[34m�[1mCreating directories for 'libnest2d'�[0m

[ 32%] �[34m�[1mPerforming download step (git clone) for 'libnest2d'�[0m

Cloning into 'libnest2d'...

Your branch is up-to-date with 'origin/master'.
Already on 'master'

[ 33%] �[34m�[1mPerforming update step for 'libnest2d'�[0m

Current branch master is up to date.
[ 34%] �[34m�[1mPerforming patch step for 'libnest2d'�[0m

[ 35%] �[34m�[1mPerforming configure step for 'libnest2d'�[0m

-- The C compiler identification is MSVC 19.0.24210.0
-- The CXX compiler identification is MSVC 19.0.24210.0
-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe

-- Check for working C compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe -- works
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe

-- Check for working CXX compiler: C:/Program Files (x86)/Microsoft Visual Studio 14.0/VC/bin/amd64/cl.exe -- works
-- Detecting CXX compiler ABI info

-- Detecting CXX compiler ABI info - done
-- Detecting CXX compile features
-- Detecting CXX compile features - done
-- Looking for pthread.h
-- Looking for pthread.h - not found
-- Found Threads: TRUE  

-- Installed Libnest2DConfig.cmake will require packages: Threads Boost Clipper NLopt
-- Configuring done
-- Generating done
-- Build files have been written to: X:/env/CURA-8240_pynest2d_transitive_dependencies/build/libnest2d-prefix/src/libnest2d-build
[ 35%] �[34m�[1mPerforming build step for 'libnest2d'�[0m



Microsoft (R) Program Maintenance Utility Version 14.00.24210.0

Copyright (C) Microsoft Corporation.  All rights reserved.



[ 36%] �[34m�[1mPerforming install step for 'libnest2d'�[0m



Microsoft (R) Program Maintenance Utility Version 14.00.24210.0

Copyright (C) Microsoft Corporation.  All rights reserved.



�[36mInstall the project...�[0m

-- Install configuration: "Release"
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/Libnest2DTargets.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/libnest2d.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/nester.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/geometry_traits.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/geometry_traits_nfp.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/common.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/parallel.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/optimizer.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/metaloop.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/rotfinder.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/rotcalipers.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/bigint.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/rational.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/utils/boost_alg.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/placers/placer_boilerplate.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/placers/bottomleftplacer.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/placers/nfpplacer.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/selections/selection_boilerplate.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/selections/filler.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/selections/firstfit.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/selections/djd_heuristic.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/backends/clipper/geometries.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/backends/clipper/clipper_polygon.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/optimizers/nlopt/simplex.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/optimizers/nlopt/subplex.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/optimizers/nlopt/genetic.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include/libnest2d/optimizers/nlopt/nlopt_boilerplate.hpp
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/Libnest2DConfig.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/Libnest2DConfigVersion.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/FindClipper.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/FindNLopt.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/FindTBB.cmake
-- Installing: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/RPPackageVersions.cmake
[ 37%] �[34m�[1mCompleted 'libnest2d'�[0m

[ 37%] Built target libnest2d

So perhaps this change requires a change to the way libnest2d is found as well?

[StefanBruens]

This seems to fail to build, on Windows only. We're getting this result:

-- Found PythonInterp: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/bin/python.exe (found suitable version "3.8.8", minimum required is "3.5") 
-- Found PythonLibs: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/libs/python38.lib (found suitable version "3.8.8", minimum required is "3.5") 
-- Found SIP: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/sip.exe (found version "4.19.24") 
-- Could NOT find PkgConfig (missing: PKG_CONFIG_EXECUTABLE) 
-- Found LIBNEST2D: X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/include  
-- Configuring done
CMake Error at cmake/SIPMacros.cmake:112 (add_library):
  Target "python_module_pynest2d" links to target
  "Libnest2D::libnest2d_headeronly" but the target was not found.  Perhaps a
  find_package() call is missing for an IMPORTED target, or an ALIAS target
  is missing?
Call Stack (most recent call first):
  CMakeLists.txt:56 (add_sip_python_module)

find_package(Libnest2D REQUIRED) calls cmake/Findlibnest2d.cmake, which then calls find_package_handle_standard_args(...).

On Linux, fphsa is able to find ...lib/cmake/Libnest2D/Libnest2DConfig.cmake, but on Windows apparently it silently fails. Without Libnest2DConfig.cmake, the imported targets are not defined.

Module mode (i.e. Find<FOO>.cmake) is a legacy feature for packages not providing a "Config.cmake" (or "-config.cmake"). The cmake/Findlibnest2d.cmake should be deleted, and you should add X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/ to your search paths, e.g. by adding "-DLibnest2d_ROOT:path=X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/", see https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure

[Ghostkeeper]

Well, that's just replacing the find procedure with a manual hard-coded path in the cura-build-environment source. I don't think that's something we want to merge then, since it wouldn't be cross-platform and would lock the build script to that computer. Some of our requirements are:

• It needs to work on Windows, MacOS and Linux.
• It needs to work on a developer's computer, an external contributor's computer and our build servers.
• It needs to work in the Linux docker container.

The first requirement is currently not met in this PR for this repository. Your suggestion would break the second requirement for cura-build-environment as a whole.

The real solution would be to make sure that the CMake script of libnest2d is properly providing a config file also on Windows. However we're not about to debug that ourselves, since the current solution is working fine and is unlikely to require frequent maintenance. The maintenance burden with our current solution (and why your PR would be better) is only a problem if the dependencies of libnest2d change, which is not likely to happen frequently. We can't justify the time investment to fix the bug in libnest2d, considering the slim benefit we'd get in the reduced maintenance (and somewhat nicer code).

[StefanBruens]

I completely debugged it for you already.

It does not hardcode the path to libnest2d, it provides the path of your build/install prefix. Obviously you have that already set somewhere, otherwise it would not be able to find the headers.

Apparently it already works on Linux and MacOS, as you have confirmed. If an existing Libnest2DConfig.cmake is not found on Windows, then your Windows build setup is broken, and with the custom Findlibnest2d.cmake you are papering over it.

[StefanBruens]

Also, the current setup does not fulfill your second requirement:

The specified link libraries omit Boost, which causes a linker error later, as some symbols from Boost transitively required by libnest2d can not be found.

[Ghostkeeper]

It does not hardcode the path to libnest2d, it provides the path of your build/install prefix. Obviously you have that already set somewhere, otherwise it would not be able to find the headers.

We provide all libraries with the same installation prefix as cura-build-environment itself, where it should be finding libnest2d:

https://github.com/Ultimaker/cura-build-environment/blob/c27ab096b87ffb20d5a63f3f67f0c079599eb64f/projects/pynest2d.cmake#L32

This is ostensibly not enough.

Apparently it already works on Linux and MacOS, as you have confirmed. If an existing Libnest2DConfig.cmake is not found on Windows, then your Windows build setup is broken, and with the custom Findlibnest2d.cmake you are papering over it.

Indeed, I see this as the real problem. Now we could make an attempt to fix libnest2d in a fork. Or we could keep it as is and paper over their bug. Since papering over their bug is already a working workaround, it doesn't need time investment on our side.

[StefanBruens]

It does not hardcode the path to libnest2d, it provides the path of your build/install prefix. Obviously you have that already set somewhere, otherwise it would not be able to find the headers.

We provide all libraries with the same installation prefix as cura-build-environment itself:

https://github.com/Ultimaker/cura-build-environment/blob/c27ab096b87ffb20d5a63f3f67f0c079599eb64f/projects/pynest2d.cmake#L32

This is ostensibly not enough.

X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/lib/cmake/Libnest2D/Libnest2DConfig.cmake

can be broken down to (with CMAKE_INSTALL_PREFIX=X:/env/CURA-8240_pynest2d_transitive_dependencies/inst/):

<prefix>/lib/cmake/<name>/<name>Config.cmake

This matches (https://cmake.org/cmake/help/latest/command/find_package.html#search-procedure):
<prefix>/(lib/<arch>|lib*|share)/cmake/<name>*/ (Unix)

but none of the search paths for Windows (W, W/U).

Apparently it already works on Linux and MacOS, as you have confirmed. If an existing Libnest2DConfig.cmake is not found on Windows, then your Windows build setup is broken, and with the custom Findlibnest2d.cmake you are papering over it.

Indeed, I see this as the real problem. Now we could make an attempt to fix libnest2d in a fork. Or we could keep it as is and paper over their bug. Since papering over their bug is already a working workaround, it doesn't need time investment on our side.

Filed an issue for libnest2d: tamasmeszaros/libnest2d#36

You can either patch libnest2d, or extend the search patch in pynest2ds ExternalProject_Add:

-DCMAKE_PREFIX_PATH="${CMAKE_INSTALL_PREFIX};${CMAKE_INSTALL_PREFIX}/lib/cmake/"

[StefanBruens]

Some of our requirements are:
* It needs to work on Windows, MacOS and Linux.
* It needs to work on a developer's computer, an external contributor's computer and our build servers.
* ...

The current code breaks your second requirement. What are you going to do about it?

[Ghostkeeper]

Since the current code works on a developer's computer and our build servers, I think you mean that it doesn't work on an external contributor's computer. At least, not for you, because we did have two other external contributors to this repository for which it did work.

I think the best approach would be to make a change to libnest2d so that it installs the config file to the proper directory on Windows. But both fixes can be applied for maximum compatibility. Would you like to edit your PR?

[StefanBruens]

I think the problem on Windows is caused by the non-typical installation path. Normally, libnest2d would go into something like C:\Program Files\Libnest2d, with subdirectories lib,include etc. When following this pattern, find_package would be able to find libnest2d without any additional changes.

See https://gitlab.kitware.com/cmake/cmake/-/issues/18453#note_470670 , paragraph "Side note ..."

For curas build, the install directory should be changed from X:.../inst/ to X:.../inst/Libnest2d/.