Allow building against system libs instead of bundled dependencies

[milianw]

When DIP_BUILD_BUNDLED_DEPENDENCIES=OFF is passed to cmake, we
will rely on find_package to find Eigen3, zlib, jpeg, ics and tiff
libraries. The fftw_api.h header will always be used, as it's not
public API that's included in the fftw3 package on ArchLinux e.g.

The default for this option is still ON, i.e. this is an opt-in
feature.

Change-Id: I4ed9095eefbfd889642d1c3d42b3d9af0dfc15ae

[crisluengo]

I get these configuration errors (see also Travis logs):

CMake Error at src/CMakeLists.txt:192 (add_executable):
  Target "unit_tests" links to target "doctest::doctest" but the target was
  not found.  Perhaps a find_package() call is missing for an IMPORTED
  target, or an ALIAS target is missing?

CMake Error at src/CMakeLists.txt:9 (add_library):
  Target "DIP" links to target "doctest::doctest" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

CMake Error at src/CMakeLists.txt:9 (add_library):
  Target "DIP" links to target "TIFF::TIFF" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?

CMake Error at src/CMakeLists.txt:9 (add_library):
  Target "DIP" links to target "doctest::doctest" but the target was not
  found.  Perhaps a find_package() call is missing for an IMPORTED target, or
  an ALIAS target is missing?

CMake Error at src/CMakeLists.txt:9 (add_library):
  Target "DIP" links to target "TIFF::TIFF" but the target was not found.
  Perhaps a find_package() call is missing for an IMPORTED target, or an
  ALIAS target is missing?

I've made some changes to fix this, see this branch: https://github.com/DIPlib/diplib/tree/wip/cmake-external-dependencies (I wasn't able to push to your branch).

1. libics installs some files that CMake uses to find the library and define the libics target, just doing find_package(libics REQUIRED). So FindICS.cmake is not necessary. Could you verify that this works correctly on Windows as well?

2. Eigen, when installed, creates the file /usr/local/share/cmake/Modules/FindEigen3.cmake. Do we need to provide a copy? Maybe needed on Windows? Maybe useful when installed in a different manner?

3. Zlib is only used to build libics and libtiff, so is not necessary when using externally build libraries.

4. Tests fail when building with external libraries because we have a modified version of pybind11. I suggest we always use the internal one. Not sure if it matters being on the bleeding edge for Python bindings. Do the same safety considerations apply?

[milianw]

Hey @crisluengo - sorry for that, I now see some errors here too, which I seem to have missed previously when I tested this. That said, what configuration did you use exactly to trigger these warnings/errors, as I cannot reproduce all of them here.

Regarding the other points:

1. I tested this on linux, and on archlinux at least the libics package does not ship with any CMake files and thus the find script is required there.
2. For eigen, it is afaik also good practice to ship with a find script, as otherwise you get really arcane errors when it is not installed, or the location of the external find script not within the cmake module path
3. agreed
4. for us this is also about making sure the software is following licenses properly. i.e. by using an external dependency, we can be sure that it is an upstream release. if any patches are required, we can publish them to make sure we are following the licensing terms properly. And generally, it sounds like there is a bug then within the pybind code if it suddenly stops working with a new version? Anyhow, I can try to look into that.

[crisluengo]

1. I guess they don’t use the CMake script to build there. Fine. But why not define the lib target as libics like it’s in that lib’s CMake script? Would be more consistent.

2. OK.

3. Let’s remove that Find command then. And the alias we don’t need.

4. The problem is that our patch changes the functionality in a way that upstream doesn’t like. They’re not going to take this change. See this issue. The patch adds behavior we really want in our Python bindings, and would be a lot of work to implement otherwise. It also breaks behavior we don’t use and never will (we don’t do object pointers as input to public API). There might be a way to write this patch in a way that doesn’t break the tests, but pybind11 is rather complex and I haven’t had the time yet to look into this.

   On the other hand, I don’t understand why there would be a licensing issue. Pybind11 license allows modifications and allows distribution with those modifications.

[milianw]

1. ah, yes indeed, sadly upstream has no alias target itself I now realize.
2. ok, I'll clean that up the next days
3. yes, I'm totally on your side wrt to BSD license allowing it. sadly, sometimes lawyers and management is not that good at understanding that apparently :D I'll disable that part again or keep it behind another extra option. That said - have you tried adding a dip::Image::Image(std::optional<dip::Image> img) constructor for this purpose instead? that could work, the way I read the pybind11 documentation, as that optional would also allow passing via None?

[crisluengo]

Unfortunately adding such a constructor to dip::Image doesn't work. It is each of the functions that must accept None instead of a dip::Image that must take a std::optional<> as input. Or just a pointer to an image.

In the Python interface we have already defined an explicit constructor that takes py::none (Pybind11's representation of Python None) as input, and declare that py::none is implicitly convertible to dip::Image. But the code never gets to use these converters without the small tweak to Pybind11 we did. You can even decorate the argument as py::arg("mask").none() to indicate that None is allowed for this argument, and it's ignored because the argument is not a pointer type.

Though I think I've figured out how to fix Pybind11 for our use case in an acceptable manner.

[crisluengo]

Milian, thanks for bringing this Pybind11 thing to my attention again. The next Pybind11 release will work for our Python bindings without a patch. Yay!
pybind/pybind11#3059