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.

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

Use C++14, required for Boost 1.75+ #7

Closed
wants to merge 1 commit into from

Conversation

hroncok
Copy link

@hroncok hroncok commented Jan 29, 2021

No description provided.

@hroncok hroncok marked this pull request as ready for review January 29, 2021 15:56
@Ghostkeeper
Copy link
Contributor

But libnest2d doesn't require 1.75+. By default it takes 1.70. Cura uses v1.67 even. In those versions, the Geometry module doesn't require C++14 yet (it was deprecated from v1.73 onwards).

If Boost 1.75+ requires C++14, it should require that in its CMake script, not in all of its users' CMake scripts. So I'd like to reject this pull request. But if the CMake script to find the Boost headers doesn't properly execute its CMake script there that would be something we need to look at in Ultimaker/libnest2d.

We've been planning for a while to upgrade to C++17 for all of Cura's C++ code, which would also upgrade this code base and allow us to use newer features. From that moment on, pynest2d itself would require C++17.

@Ghostkeeper Ghostkeeper closed this Feb 5, 2021
@hroncok
Copy link
Author

hroncok commented Feb 5, 2021

If Boost 1.75+ requires C++14, it should require that in its CMake script, not in all of its users' CMake scripts. ... But if the CMake script to find the Boost headers doesn't properly execute its CMake script there that would be something we need to look at in Ultimaker/libnest2d.

Would you be able to help me check how this works or why does it not work?

@Ghostkeeper
Copy link
Contributor

Ghostkeeper commented Feb 12, 2021

Yeah, I can help a bit. Though I must say that libnest2d is not written by me or the Cura team. I've forked it for a small modification that Cura needed, but I'm not intimate with how it all works there. I'll try to write out my investigation...

So libnest2d is supposed to have a modular structure where you can swap out the "geometry backend" between Clipper, Boost and Eigen:

https://github.com/Ultimaker/libnest2d/blob/4d6fb4d3919aad653cbae0ae2e03c0e1ac00c64b/CMakeLists.txt#L20

This then adds a subdirectory to the CMake script, depending on the selected backend:

https://github.com/Ultimaker/libnest2d/blob/4d6fb4d3919aad653cbae0ae2e03c0e1ac00c64b/CMakeLists.txt#L79

If Boost is selected, it would add the directory include/libnest2d/backends/boost. That doesn't exist in the source tree though (though Boost is an option here, it doesn't seem to be actually implemented). Instead Clipper is selected as default and that seems to be the only real option too.
However libnest2d adjusts Clipper to its needs and then includes Boost as well:

https://github.com/Ultimaker/libnest2d/blob/4d6fb4d3919aad653cbae0ae2e03c0e1ac00c64b/include/libnest2d/backends/clipper/CMakeLists.txt#L6-L8

The weird thing here is this function require_package(...). It looks a lot like find_package but it's not. It's actually a custom package manager system of sorts (aw hell): https://github.com/Ultimaker/libnest2d/blob/master/cmake_modules/RequirePackage.cmake

It looks like this script intends to find the package on the system first, and if it's not present provide the option to download it automatically. But instead of just a find module and if it's not find an ExternalProject_Add all, it does a whole lot more, including several calls to the CMake executable to separately execute the configure-step of Boost's build system. I must admit that I get a bit lost here since it works with far-from-conventional ways to build this "required" package. But the way I understand it is that it eventually comes down here to find Boost:

https://github.com/Ultimaker/libnest2d/blob/4d6fb4d3919aad653cbae0ae2e03c0e1ac00c64b/cmake_modules/RequirePackage.cmake#L234

I'm not sure what happens there when it finds Boost 1.75 but the Boost 1.75 configure stage requires C++14. Boost doesn't use CMake but their B-Jam thing (or does it have CMake too nowadays?) so perhaps it would only fail when it actually tries to build Boost then. And then this never occurs because we don't build Boost here; we include its headers. If Boost doesn't have CMake to check for C++14 support, ideally this find_package script then looks for only the Boost versions supported by the current compiler, i.e. only the Boost versions that we can use. So if it finds Boost 1.75 with a compiler that doesn't support C++14, it would skip that Boost version and consequently say that it doesn't find Boost. Evidentially, that's not happening. But this is where I would do some testing.
The find_package call effectively then calls on the FindBoost.cmake script which comes built-in with CMake: https://github.com/Kitware/CMake/blob/master/Modules/FindBoost.cmake . This means that your issue could also depend on the CMake version. Older versions of CMake might have FindBoost scripts that don't check for C++14 yet because they don't know about Boost 1.75 yet.

It's quite a mess, isn't it?

@hroncok
Copy link
Author

hroncok commented Feb 12, 2021

Indeed, sound really complicated. Thanks for the analysis.

I've resumed to sed -i 's/CMAKE_CXX_STANDARD 11/CMAKE_CXX_STANDARD 14/' CMakeLists.txt in Fedora and that's what I am fine keeping anyway :)

@Ghostkeeper
Copy link
Contributor

As long as the distro can be assumed to have a C++14 compiler nowadays, that'll be fine.

We are planning to switch to C++17 soon-ish. Several attempts have been made but since our MacOS build server is (intentionally) old, it's a bit of a problem.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants