Pugixml fetch only

[cboulay]

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]

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]

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 .

[cboulay]

Converting to draft because there are still some build scenarios where internet access won't be available on the target device.

I'm thinking of the following:

• Developer downloads/clones liblsl, asio, and pugixml and copies them all to the target device next to each other.
• liblsl's cmake code checks to see if it is connected to the internet and if so use the normal approach
• if not then check to see if the dependency code is in a sister folder to liblsl then provide that to FetchContent via SOURCE_DIR
• if not then fail with an informative message

[tstenner]

FWIW the latest ubuntu will package pugixml >=1.15 in three months and conda might already to this.

One other way to make this shorter would be a function/macro that converts a const std::string& to either a string_view or a c_str:

#if defined(PUGIXML_HAS_STRING_VIEW)
	node.append_child(name).append_child(node_pcdata).set_value(value);
#else
	node.append_child(name).append_child(node_pcdata).set_value(value.c_str());
#endif

then becomes

#if PUGIXML_VERSION >= 1150
std::string_view pugi_str(const std::string&) { return value; }
#else
const char* pugi_str(const std::string&) { return value.c_str(); }
#endif

// [..]
node.append_child(name).append_child(node_pcdata).set_value(pugi_str(value));

That way, the #ifdef isn't needed for every call site

[cboulay]

I added a new CMake option LSL_FETCH_PUGIXML which defaults to ON. If the developer wants to use system pugixml then they can set this to OFF.
In case this pulls in pugixml < 1.15, I had to revert (with Tristan's convenience function) the 2 code paths for strings. I exercised this in the ubuntu 22.04 config and all the tests pass.

But I don't want to actually keep it that way (because then some liblsl release artifacts require pugixml and others don't), so I changed the config back to fetch pugixml.