Skip to content

Conversation

@cboulay
Copy link
Collaborator

@cboulay cboulay commented Jan 18, 2026

Use FetchContent for pugixml; link privately to hide symbols

Note: This is based on #253

  • Fetch pugixml v1.15 via FetchContent instead of bundling or system package
  • Link pugixml statically and privately to prevent symbol leakage
  • Remove LSL_BUNDLED_PUGIXML option and thirdparty/pugixml/
  • Simplify stream_info_impl.cpp by removing #ifdef PUGIXML_HAS_STRING_VIEW guards

Why?

pugixml 1.15 has some desirable updates, but it is not available in ubuntu via apt-get

Are there any objections to this? The cons that I can think of:

  • Increased compile time on configs where we'd otherwise use system pugixml
  • Increased binary size on configs where we could otherwise use system pugixml
  • Internet required at config time
  • Downstream clients with 'install pugixml' as a prerequisite should be updated to remove that step

I think these aren't major concerns, especially because platforms where we might be concerned about resources typically do not have an easy-to-install pugixml anyway.
Is there anything else to be worried about?

@tstenner

@myd7349
Copy link
Contributor

myd7349 commented Jan 18, 2026

From the perspective of some package managers, they tend to avoid using vendored dependencies.
https://learn.microsoft.com/en-us/vcpkg/contributing/maintainer-guide#do-not-use-vendored-dependencies

For example, in vcpkg, all dependencies of liblsl (asio, boost, pugixml) are installed as separate packages:
https://github.com/microsoft/vcpkg/blob/66c0373dc7fca549e5803087b9487edfe3aca0a1/ports/liblsl/vcpkg.json#L8-L23

Therefore, if pugixml is obtained via FetchContent, IMO there should still be an option (just like -DLSL_BUNDLED_PUGIXML=OFF) to allow users to use the pugixml provided by the package manager, or even the system-installed pugixml.

@cboulay
Copy link
Collaborator Author

cboulay commented Jan 18, 2026

Therefore, if pugixml is obtained via FetchContent, IMO there should still be an option (just like -DLSL_BUNDLED_PUGIXML=OFF) to allow users to use the pugixml provided by the package manager, or even the system-installed pugixml.

Unfortunately, pugixml 1.15 is not available in ubuntu package managers so this isn't an option unless we keep the #ifdef PUGIXML_HAS_STRING_VIEW. I'd be willing to restore an option to use system pugixml if/when 1.15 is available from package managers.

they tend to avoid using vendored dependencies

I guess this is a step in the right direction because we're getting rid of the pugixml directory in our source tree. Additionally, some of the concerns raised in that article don't apply here because with this PR we are statically linking pugixml and all of its symbols are hidden.


If we must keep the option of using system pugixml then I'd prefer to delay merging #253 .

…ACE:...> because pugixml and lslobj are not needed in the export set
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.

3 participants