Skip to content

build: allow clients to provide their own pugixml#244

Merged
webern merged 3 commits into
webern:mainfrom
rpatters1:external-pugixml-support
Jun 27, 2026
Merged

build: allow clients to provide their own pugixml#244
webern merged 3 commits into
webern:mainfrom
rpatters1:external-pugixml-support

Conversation

@rpatters1

Copy link
Copy Markdown
Contributor

This PR gates the inclusion of the local vendored pugixml on whether another already exists. It allows a client project that also uses pugixml to include mx without creating a conflict.

Resolves #242

@webern webern left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. As written it won’t quite work because I have dumb header paths for the pugi headers. There’s a small fix needed in the Cmake file and the include paths need to change at all the sites where pugi is referenced.

I’m on a mobile phone right now. Later I can add a commit to this PR or you can do it if you get there first:

git remote add upstream https://github.com/webern/mx.git
git fetch upstream pugi-headers
git cherry-pick 99dc0a9

Also the CI failures are something I need to fix. It looks like my comment posting workflows for code coverage don’t work for anyone but me.

@rpatters1

Copy link
Copy Markdown
Contributor Author

It was working for me, weirdly, so I never looked at the include sites. Thank you for the quick response. I updated the PR with your change.

@webern webern changed the title Allow clients to provide their own pugixml build: allow clients to provide their own pugixml Jun 27, 2026
Change the vendored pugixml include directory from ${PRIVATE_DIR} to
${PRIVATE_DIR}/pugixml so that code includes pugixml.hpp directly
(matching upstream pugixml conventions). This lets the if(NOT TARGET
pugixml) guard work seamlessly with find_package(pugixml) or any
other standard pugixml CMake target.
@webern webern force-pushed the external-pugixml-support branch from 99dc0a9 to 8394f86 Compare June 27, 2026 14:40
@webern

webern commented Jun 27, 2026

Copy link
Copy Markdown
Owner

It was working for me, weirdly, so I never looked at the include sites. Thank you for the quick response. I updated the PR with your change.

I think it worked because your version of pugixml and the vendored version were the same, or at least close enough that the headers did not differ in any binary-affecting way. So I think when you were building mx you were reading the vendored headers but linking the cpp's you had built outside.

Anyway, I had forgotten to update one thing so did a force push to that third commit (mine). And checking CI 👍

@webern webern left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, thank you for fixing this!

@webern webern merged commit 799603d into webern:main Jun 27, 2026
5 of 7 checks passed
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.

Pugixml conflict

2 participants