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

Do not vendor the pugixml library #8

Closed
wants to merge 1 commit into from

Conversation

eli-schwartz
Copy link

This prevents trying to co-install pugixml with existing system
libraries, and also fixes the includes for pugixml.hpp to actually use
the include path added by find_package/add_subdirectory instead of a
hardcoded relative path.

Fixes #7

This prevents trying to co-install pugixml with existing system
libraries, and also fixes the includes for pugixml.hpp to actually use
the include path added by find_package/add_subdirectory instead of a
hardcoded relative path.
@nallath
Copy link
Member

nallath commented Apr 12, 2018

For our reference CURA-5226

@Ghostkeeper
Copy link
Contributor

Ghostkeeper commented Apr 26, 2018

Something went wrong in Jira there so I had to delete CURA-5226 and create a new ticket. The new ticket is CURA-5300.

I think this is a nice improvement. You rarely see a PR that removes 14k lines of code, replaces it with 7 lines and it still works 😆

MathyV added a commit to MathyV/gentoo that referenced this pull request Jun 14, 2018
Upstream put pugixml inside the distribution and it is built and installed
alongside with the library. This results in file collisions when pugixml is
already installed on the system.

Existing upstream pull request: Ultimaker/libSavitar#8

This commit applies that pull request as a patch. I modified the patch slightly
to not include the actual deletion of the library and only apply the needed
changes to the rest of the library. I did this because otherwise the filesize
was bigger than the portage limit.

Closes: https://bugs.gentoo.org/658112
Package-Manager: Portage[mgorny]-2.3.36.1
@MathyV
Copy link

MathyV commented Jun 14, 2018

Ran into this as you can see from the commits above running Gentoo. Applied this PR and works perfectly. Would be nice if it can be merged in the next version.

gentoo-bot pushed a commit to gentoo/gentoo that referenced this pull request Jun 15, 2018
Upstream put pugixml inside the distribution and it is built and installed
alongside with the library. This results in file collisions when pugixml is
already installed on the system.

Existing upstream pull request: Ultimaker/libSavitar#8

This commit applies that pull request as a patch. I modified the patch slightly
to not include the actual deletion of the library and only apply the needed
changes to the rest of the library. I did this because otherwise the filesize
was bigger than the portage limit.

Closes: https://bugs.gentoo.org/658112
Closes: #8846
Package-Manager: Portage[mgorny]-2.3.36.1
@alekseisasin
Copy link

@MathyV , I tested your pull request and result of it:

  1. I can build "libpugixml-dev" on Linux and Windows and it is good. Only we need to change our Cura build script.

After installing "sudo apt-get install libpugixml-dev" I cannot call cmake because I am missing:
Could not find a package configuration file provided by "pugixml" with any
of the following names:

If I remove find_package(pugixml REQUIRED), I trigger cmake .., "make" command works well without error.
Is suggest to replace the find_package(pugixml REQUIRED) to something else. Maybe CheckIncludeFile(....) if not show error of missing "pugixml". Otherwise the solution will work for you and other might have problem because of "find_package(pugixml REQUIRED)"

@eli-schwartz
Copy link
Author

eli-schwartz commented Jul 4, 2018

Then your pugixml installation sucks and is broken, because it misses the files usr/lib/cmake/pugixml/pugixml-config-release.cmake and usr/lib/cmake/pugixml/pugixml-config.cmake which pugixml automatically installs. Or your cmake installation sucks because it is not picking up the standard cmake files installed by system packages.

(Never mind that cmake should use the standard pkg-config instead of it's homebrew solution...)

Anyway I've got no clue whatsoever whatsoever what you're proposing instead, nor why you're addressing your comment to random people instead of me, the author of this PR.

@eli-schwartz
Copy link
Author

Mystery solved, debian's package is broken: https://packages.debian.org/sid/amd64/libpugixml-dev/filelist

You'll need to tell cmake (using CMAKE_MODULE_PATH to look in this private cmake directory used exclusively by libpugixml-dev on Debian.

@LipuFei
Copy link
Contributor

LipuFei commented Jul 5, 2018

Hi @eli-schwartz , which linux distro are you using?

@eli-schwartz
Copy link
Author

eli-schwartz commented Jul 5, 2018

Arch Linux.

As mentioned in #7 (referenced in the first comment) I found this bug in the process of helping to resolve downstream bug https://bugs.archlinux.org/task/57800
Currently Arch and gentoo both apply this patch...

@Ghostkeeper
Copy link
Contributor

Shall we just add this library to cura-build-environment then?

@Ghostkeeper
Copy link
Contributor

Apparently not. It's been decided that an external dependency makes setting up your development environment for Cura needlessly difficult. We won't be merging this then. Sorry!

@anthraxx
Copy link

anthraxx commented Jul 13, 2018

WAT 😱
yeah that would be unmaintainable rocket science i guess 🙄

@jelly
Copy link

jelly commented Jul 13, 2018

Can't we find a compromise where it can build it from an external lib for distros but use the shipped library for Cura dev env. My cmake fu is too weak to suggest a patch.

@eli-schwartz
Copy link
Author

I would assume that would involve:

Shall we just add this library to cura-build-environment then?

It should be sufficient to tell cmake to install arbitrary (static) libs to some custom build root, and then set the cmake module path.

I'm unsure if @Ghostkeeper's rejection means only something simple enough to be shipped directly in this precise git repository would be accepted.

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

Successfully merging this pull request may close these issues.

unable to use system pugixml
8 participants