unable to use system pugixml

[sl1pkn07]

Hi

any option for use system pugixml instead of bundled?

when install libsavitar make some file conflicts with pugixml installed in the system

[eli-schwartz]

I'm sort of confused what this even does, since it seemingly includes the pugixml headers in a totally unusable way, which assumes the public libsavitar header /usr/include/Savitar/ThreeMFParser.h can access the header ../pugixml/src/pugixml.hpp on an installed system which makes no sense at all.

also see https://bugs.archlinux.org/task/57800

[nallath]

It's because as with things like this go; Once it works, no-one can be bothered to change it. Especially not if there are tons of people hounding you for new features.

We're not even using the installed headers at all. This is probably why no-one (myself included) ever noticed it. The headers having the hardcoded reference there is needed for building it, which is probably why it stayed there.

[krop]

Any news about this ? I wrote a different patch for my local openSUSE package (just adding an option to use system's pugixml).

[nallath]

I'm not that involved with Cura directly anymore, but I've bumped the issue on our tracker.

[Ghostkeeper]

It's still in our planning as one of the 8 open PRs that we still need to check.

[onitake]

FWIW, we use this patch to reference the system package on Debian: https://salsa.debian.org/3dprinting-team/libsavitar/blob/master/debian/patches/2002-system-pugixml.patch

@eli-schwartz What exactly was your problem with the Debian libpugixml-dev?

I suppose additional work will be needed to fall back to the vendored copy if the system doesn't have pugixml headers installed. YMMV.

[eli-schwartz]

@onitake,

I have no problem with pugixml-dev on Debian, because I do not use Debian nor wish to. :)

However, I have scanned your bugtracker for you and discovered this bug which was recently reported and fixed: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=929792

So it seems that as of today, your distro is no longer distributing a broken version of pugixml, and all is well. More pertinently, you can now use the cleaner, more minimal and better integrated patch from my PR (with links to the PR so that people reading your distro packaging know that the issue has been properly forwarded upstream) rather than your patch which manually searches for libraries and headers.

[eli-schwartz]

I suppose additional work will be needed to fall back to the vendored copy if the system doesn't have pugixml headers installed. YMMV.

There is no additional work needed. If you want to build with a vendored pugixml, you compile it using a tiny shell script which downloads pugixml and installs it into a private install root. Cura already has a more polished version of such a shell script, it is called https://github.com/Ultimaker/cura-build-environment and handles multiple dependencies with cmake's ExternalProject handling thrown in for bonus points..

[onitake]

@eli-schwartz Ah I see where the problem lies. Our patch (which we've been using for quite a while now) didn't rely on a cmake script and instead just searched for the header plus library by find_path and find_library. Thanks for pointing out that the package has a cmake script and that it's now installed to a standard location.

I'll hold off changing the patch for now though, in case there's a decision on how to proceed on this issue.

[eli-schwartz]

The libsavitar developers have stated in #8 (comment) that they are uninterested in permitting a system pugixml to be used. There was a lack of clarity in their part regarding what, exactly, they would accept (if anything), but they did close my PR as "wontfix". Given it's been over a year since then, I'm not sure what decisions you expect to be made any time soon. :)

(I honestly suspect they merely forgot to close this one as well. A WONTFIX for the PR would seem to imply a WONTFIX for the issue too.)

[sl1pkn07]

then patch, patch everywere :V

[onitake]

@Ghostkeeper Your thoughts?

[Ghostkeeper]

Indeed we'd forgotten to close this issue back then. We had closed the PR because it pays off in development time to keep the build system somewhat simpler. It's a bit inconsistent if you ask me, but that's the opinion of Ultimaker and it has merit from our perspective.

Myself I'd prefer to have at least a Git submodule rather than this solution with the code copied and checked out in our repo. That would make it easier to update. But it's not as pressing to me as making the deadline for Cura 4.4. So far we haven't had any issues with Pugixml.

The patch from Eli-schwartz is pretty much exactly how I'd approach this, except that I'd add a FindPugixml.cmake script as well to make sure it can be found on different systems. I think it's a better option than the patch from Onitake.

(I'll keep this issue open for now to let this discussion continue freely.)

[krop]

That's even easier, a FindPugixml.cmake module is not needed.
Pugixml provides a CMake config file with all the information to use it

[eli-schwartz]

The patch from Eli-schwartz is pretty much exactly how I'd approach this, except that I'd add a FindPugixml.cmake script as well to make sure it can be found on different systems. I think it's a better option than the patch from Onitake.

The purpose of my patch is, as @krop pointed out, that the pugixml software always installs pugixml-config.cmake for the exact reason that pugixml wishes to tell people how to link to it. You aren't allowed to install pugixml without installing the .cmake file (though if you wish to and you hate all your users with a hateful passion of hatred, you could delete it after it is installed, but I have no earthly clue why anyone would do such a crazy thing).

Creating your own homebrew FindPugixml.cmake is technologically identical to @onitake's historic patch, because it uses custom, homebrew functions to heuristically scan the filesystem for the library. The only thing it does is refactor it into a separately composable file. It completely misses the point of why one would wish to use find_package in the first place -- which is, to depend on the officially exported API that the project provides for finding and building against it.

Homebrew Find****.cmake files vendored into project trees are IMNSHO the great evil which cmake has inflicted on the software development world. It's one of the leading contenders for my belief that projects which use cmake are fundamentally bad, but I guess I should get off my soapbox now.

[onitake]

Creating your own homebrew FindPugixml.cmake is technologically identical to @onitake's historic patch, because it uses custom, homebrew functions to heuristically scan the filesystem for the library. The only thing it does is refactor it into a separately composable file. It completely misses the point of why one would wish to use find_package in the first place -- which is, to depend on the officially exported API that the project provides for finding and building against it.

No need to get heated up about this - the approach chosen in the Debian patch is a perfectly valid and a common solution for any library that doesn't provide a cmake, cmake or -config script.
But I'm not even arguing about it, any proper solution is fine by me. I just provided the patch for completeness, because I didn't know about the install location bug in the Debian pugixml package before. Thanks for reporting and fixing it.

If UM decides to change CMakeLists.txt to use system libs, I will gladly adopt the chosen solution.
If not, I will keep a patch in the Debian repos that works for Debian policies, but I'll probably change it to use the cmake script.

Either way is fine by me. And I think we should also ask for the opinions of the Fedora and PPA maintainers: @hroncok @thopiekar

[eli-schwartz]

@onitake I'm not getting heated up about your patch :) you're not vendoring a FindFoo.cmake script into a repository when an upstream -config.cmake script is known to exist. My assumption is that you'll move to find_package now that you know it exists, which is as things should be.

I do find it depressing to see people actively say no to known existing find_package providers. There is a reason pkg-config exists. There is also a reason find_package exists (even if that reason is "be like pkg-config except with added NIH syndrome"). And while it is undoubtedly valid to hardcode -l flags and -I flags when you don't know there is something better, it is definitely not valid if you do know there is something better.

My PR represents something better. Something that is guaranteed to work everywhere, without exception (unless someone builds pugixml and then post-processes the installation to rm the -config.cmake scripts, but if anyone hates themselves that much, they deserve the right to have self-inflicted compiler errors and I refuse to feel sorry for them).
So, yes, I may get a bit heated up by the idea of upstream rejecting this in order to write their own FindPugixml.cmake for utterly no reason. It's an anti-pattern.

[Ghostkeeper]

We're not writing our own FindPugixml.cmake file. So far we have no plans to change the status quo, not via Eli-schwartz' method nor any other.

Sorry, I didn't see that they had CMake installation support. Of course it's unnecessary then to create a Find script.

[hroncok]

Fedora maintainer opinion: I'm used to various upstreams* bundling stuff. I'm totally ok patching it out. Having an upstream supported switch would be nice but 🤷‍♂️

Fedora does this:

	
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -15,8 +15,6 @@ if(BUILD_TESTS)
     find_package(Threads QUIET)
 endif()
 
-add_subdirectory(pugixml)
-
 if(BUILD_PYTHON)
     list(APPEND CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake)

And this:

$ rm pugixml -rf
$ sed -i 's|"../pugixml/src/pugixml.hpp"|<pugixml.hpp>|g' src/*.cpp src/*.h

I consider the sed easier to maintain - it doesn't need frequent rebases (unlike a patch). In fact it remained intact since the package was introduced to Fedora in May 2017.

* I was gonna say I kinda feel that upstream projects that are run by companies (whether it's Ultimaker, Prusa or something completely different) who often need to offer integrated downloadable packages/installers are more likely to do this than vendor-independent projects (e.g. Printrun), however this might not be true, as for example (vanilla) Slic3r also bundles a lot of stuff, so I am probably just biased.

[Ghostkeeper]

I was gonna say I kinda feel that upstream projects that are run by companies (whether it's Ultimaker, Prusa or something completely different) who often need to offer integrated downloadable packages/installers are more likely to do this than vendor-independent projects (e.g. Printrun), however this might not be true, as for example (vanilla) Slic3r also bundles a lot of stuff, so I am probably just biased.

I have the same experience actually. From the other side, I can say that it's probably due to greater pressure to sync a release to other product releases, such as Ultimaker's newest printer.

[eli-schwartz]

@hroncok

aside, but I generally find patches are a lot safer than sed, because with sed you cannot tell whether the changes you expected have actually happened -- moreover, I've seen far, far too many sed lines with failing heuristics that resulted in the wildly deleting code that should not be deleted, which did not trigger failing builds but did trigger runtime crashes after you've clicked around a bit. (But C/C++ tends to fail in the compiler, you say? But interpreted languages, I say.)

who often need to offer integrated downloadable packages/installers are more likely to do this than vendor-independent projects

In theory, this is supposed to be why static libraries are good. Also, in practice, I think this is why the Meson build system is good -- because it makes it trivial to toggle dependencies between system providers and building them as a custom subproject wrap.

I'm an optimist -- I believe it's possible to make both sides happy.