PrusaSlicer icon indicating copy to clipboard operation
PrusaSlicer copied to clipboard

Some update work that you might find useful for moving forward

Open schiele opened this issue 8 months ago • 2 comments

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.

schiele avatar Apr 01 '25 11:04 schiele

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?

SachCZ avatar Apr 01 '25 13:04 SachCZ

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. ;)

schiele avatar Apr 01 '25 14:04 schiele

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? This would reduce the number of CMake patches again since more current versions of the dependencies have fewer references to ancient CMake versions.

schiele avatar Apr 02 '25 03:04 schiele

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.

SachCZ avatar Apr 02 '25 09:04 SachCZ

As a first step I found another approach to fix the CMake problem that does not require any patches to be applied to dependencies. I tried that approach first but did it slightly wrong and then incorrectly concluded that it does not work --- and only that way went down the cumbersome patch approach. With the modified patches things are looking much better already.

schiele avatar Apr 02 '25 15:04 schiele

Yes, this is way better! Thanks.

SachCZ avatar Apr 02 '25 15:04 SachCZ

Rebased to latest master (2.9.2-rc1).

schiele avatar Apr 04 '25 02:04 schiele

There are several merge requests open which actually do the same as this MR:

https://github.com/prusa3d/PrusaSlicer/pull/13081 https://github.com/prusa3d/PrusaSlicer/pull/11769 https://github.com/prusa3d/PrusaSlicer/pull/13761

For CMake I would require 3.10 as the minimum version, it seems this is available on all distributions in the meantime I found supported including RHEL9.

cryptomilk avatar Apr 05 '25 06:04 cryptomilk

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. Maybe we should add a test that links OCCTwrapper.so to a dummy main function to get feedback from the linker about missing symbols during build time.

schiele avatar Apr 08 '25 12:04 schiele

Would you consider going one step further and replace the function(add_cmake_project) and wrap it around FetchContent? This would make it easier for downstream to use system packages as well. Probably in a different PR though.

Also, the CMAKE_POLICY_VERSION_MINIMUM should not be a replacement for updating the first-party cmake_minimum_required. Please do that.

LecrisUT avatar Aug 02 '25 14:08 LecrisUT