Some update work that you might find useful for moving forward #14388
Conversation
|
Hello thank you, the CGAL bump looks very useful. I hope we can get to this soon (ish). Why are you patching all the dependencies to require cmake 3.10? |
|
The reason is that policy compatibility to versions older than 3.5 have been removed with CMake 4.0.0 (see https://www.kitware.com/cmake-4-0-0-available-for-download/). Since versions older than 3.10 are deprecated I moved it up to that level to reduce the amount of warnings thrown. Note that CMake 3.10 is already 7 year old and thus older versions are probably not relevant any longer, even for people sitting on older development systems. In other words: Update your cmake to 4.0.0 and you will see why I did that. ;) |
|
By the way, I have internally updated most of the other dependencies as well. Those updates did not contain any API changes and did not require any code changes other than the update itself. Since I can only test things on Linux, I am not sure whether those other trivial updates are worth a pull request. Are you interested in seeing them as well? If so, would you prefer to have separate pull requests, or should I just add them to this pull request as separate commits? |
|
Hi, thank you, makes sense. I would actually prefer to reduce the number of patches in general because honestly it is even more pain to keep together than it already is. The reality is that sometimes (rarely but still...) we do not update a dependency just because there is a conflict with the patch and nobody has the time to look into it (and usually it is sadly very trivial). So if you can update a thing instead of patching it, go for it. Also, to be completely transparent with you, since my last comment I thought about it and it will take a while before we get to merge this in any shape or form. Thank you anyway. |
schiele
force-pushed
the
updates_for_prusa
branch
from
9e7efec to
cca5bd8
Compare
|
As a first step I found another approach to fix the CMake problem that does not require any patches to be applied to dependencies. |
|
Yes, this is way better! Thanks. |
schiele
force-pushed
the
updates_for_prusa
branch
from
cca5bd8 to
f04dd49
Compare
|
Rebased to latest master (2.9.2-rc1). |
|
|
||
| set(OCCT_LIBS | ||
| TKXDESTEP |
There was a problem hiding this comment.
There is only a single lib needed as it pulls in all the required ones, even with 7.6, see:
There was a problem hiding this comment.
This reliance on transitive dependencies works for dynamic linking but not for static linking as Prusa usually does. So we either need to leave the list as it currently is or add some if statements on the version number to select all relevant libraries for each version when libraries change.
| @@ -11,6 +11,8 @@ | |||
| cmake_minimum_required(VERSION 3.13) | |||
| project(PrusaSlicer) | |||
|
|
|||
| set(CMAKE_POLICY_VERSION_MINIMUM 3.5) | |||
There was a problem hiding this comment.
Why not set the minimum cmake requirement to 3.10 everywhere? I don't think 3.5 is really required.
There was a problem hiding this comment.
I've just checked, even RHEL8 has at least cmake 3.11.
There was a problem hiding this comment.
Note that this does not set the minimal version of CMake required but changes the policy behavior. I tested both policy sets, the minimum 3.5 and the minimum 3.10, and they both seem to work. To keep the potential for behavioral changes low, I finally decided to go with the minimum 3.5 since this works with all current versions of CMake and is the least intrusive change. But I have no strong opinion here. If people feel 3.10 would be a better minimum policy version to avoid additional warnings, this should also work. The correct long-term fix is to update the ancient libraries we are still using refering to ancient CMake versions.
There was a problem hiding this comment.
Newer cmake versions will only support 3.10 policy as support for compatibility < 3.10 will be removed!
CMake Deprecation Warning at CMakeLists.txt:2 (cmake_minimum_required):
Compatibility with CMake < 3.10 will be removed from a future version of
CMake.
Update the VERSION argument <min> value. Or, use the <min>...<max> syntax
to tell CMake that the project requires at least <min> but has been updated
to work with policies introduced by <max> or earlier.
| URL https://github.com/CGAL/cgal/archive/refs/tags/v5.4.zip | ||
| URL_HASH SHA256=d7605e0a5a5ca17da7547592f6f6e4a59430a0bc861948974254d0de43eab4c0 | ||
| URL https://github.com/CGAL/cgal/archive/refs/tags/v6.0.1.zip | ||
| URL_HASH SHA256=6aa3837ebcefc39a53a7e6ac8ac08d7695d942e2eaab3709dc43da118cd10bc4 | ||
| ) |
There was a problem hiding this comment.
I think also libigl should be updated as it uses deprecated cgal header files.
There was a problem hiding this comment.
That probably makes sense. I just went with a minimal set of updates to make things work with a modern set of libraries as found in a contemporary system. I just didn't immediately want to open a can of worms since there are lots of other libraries from which deprecated headers are used, like curl.
latest CMake releases dropped support for policies older than CMake 3.5. Since our CMake files seem to work fine with policies of at least CMake 3.5 we accept this as the minimal policy level.
Make the version of OpenCASCADE more flexible by not pulling a specific version. I found the implementation robust enough not to fail with way more recent versions, thus why not allowing those?
The old code materialized the library code into the slicer binary instead of linking the library itself. This does complicate the structure and since I didn't really see the point I changed this to just link the library.
Update to a more recent version 6.0.1.
Update to a more recent version 1.87.0.
There were some API changes with the newer CGAL library. This updates the code accordingly.
There were some API changes with the newer Boost library. This updates the code accordingly.
schiele
force-pushed
the
updates_for_prusa
branch
from
f04dd49 to
1f4f449
Compare
|
Rebased to 2.9.2-rc2 and removed the lib changes for OCCT since the explicit listing of all libs is needed for static linking and as such the list seems correct. Missed that in my original testing since there is currently no build-time check for completenexx of OCCTwrapper in the code. |
When I worked on getting something updated for the current Arch Linux release (https://gitlab.archlinux.org/archlinux/packaging/packages/prusa-slicer/-/merge_requests/2) I solved some issues.
Since you likely will face the same issues when moving forward, I wanted to share this with you, such that you don't have to repeat that work.
Details and rationale for the changes can be found inside the individual commit messages. Feel free to get back to me in case of questions or concerns.