Introduce USE_SYSTEM_LIBS in cmake

[ghost]

No description provided.

[jshort]

Why is this necessary? Does vcpkg not pull it down for you? Is this a follow up to the GCC12 issues from the last PR? Feel free to update the readme with any GCC12 gotchas/exceptions. Or does that latest cxxopts commit not work well with the existing build logic? I built et on MacOS and Linux on the master commit yesterday without issue.

[ghost]

This pull request has nothing to do with GCC 12 at all, so you can relax.

As you have stated, it builds completely fine.

We are trying to "properly" update it (see https://github.com/void-linux/void-packages/pull/41823/files).

From our understanding, a bundled library is used when a distro of choice does not have it packaged or ships incompatible version with your software (i.e. you use newer/older version or have it modified for your own purpose).

In our case you use unmodified (upstream) library which perfectly aligns with the one we use - both share the same version 3.0.0 as a coincidence. The common practice suggests to use the packaged library instead of the bundled one.

I cannot argue with the fact that some systems may not have it packaged or ship incompatible versions, which is why the fallback is left to make the build process less miserable.

[MisterTea]

Hey! Thanks for taking the time to write this PR.

My take on this PR is that it goes against the spirit of vcpkg, which is supposed to handle the dependencies for us. I can see an argument for using the OS version if the package is very heavyweight, but in this case the PR adds the risk of version mismatch without reducing the operational overhead by enough to justify.

[ghost]

but in this case the PR adds the risk of version mismatch

I see. In this case I can limit the version to 3.0.0 (cmake will fall back to the vendored header if <3.0.0 is found) to reduce this risk. Will it be good enough for you?

[eli-schwartz]

My take on this PR is that it goes against the spirit of vcpkg, which is supposed to handle the dependencies for us.

AFAICT it is not using vcpkg at all, just git submodules?

But vcpkg is just a package manager, and a linux distro package manager is also a package manager. Seems odd that using one of two equivalent things counts as being against the spirit of the other.

adds the risk of version mismatch

Naturally, any PR to use a packaged version of a dependency must fulfill the same version constraints that the git submodule based approach requires.

[jshort]

True, cxxopts is not one of the deps managed by vcpkg and instead is simply a submodule.

Regardless, what benefit other than a few seconds of build time savings (if the system already has the desired version of cxxopts) does this change present? Git submodules offer the benefit of being very exact commit hashes that the software has been built and tested with (in the event a project didn't rev a version number and a non-zero number of additional commits were added, maybe one being backwards incompatible with et's usage).

[eli-schwartz]

In general for software reliability it's bad to make use of totally arbitrary git commit hashes that don't correspond to anything that upstream considers stable. Even if you get lucky and it doesn't break.

So you should be using version numbers anyway -- even if those version numbers are codified in a git submodule by commit hash of a versioned tag.

what benefit other than a few seconds of build time savings (if the system already has the desired version of cxxopts) does this change present

It's often a mandatory policy for inclusion in Linux distros, due to a couple reasons including the fact that it makes it much easier to get a good idea of what is in use, which licenses it incorporates, whether any particular software project needs to be rebuilt against a security update of said dependency, or perhaps simply because that dependency is non-portable to obscure operating systems and needed to be patched to compile, and that patch is in the system copy but not in a project's vendor directory.

It's also crucial for building in automated builders that have network sandboxing, which is frequently a global policy to prevent totally insecure use of wget without checksum validation during the build, or obtaining untrusted precompiled binaries. These policies don't special case git submodules and don't either special case "this dependency happens to be source code not precompiled binaries", and may be awkward to work around, then trigger reviews anyway.

[jshort]

@eli-schwartz That's fair. A lot of our external dependencies aren't super actively used, versioned and packaged/distributed projects (easyloggingpp looks dead, dozens of unlooked at PRs). The ones that do move a lot and likely to have CVEs etc are managed by vcpkg (libsodium, zlib, protobuf, openssl). Maybe in the future we'll find replacements for some of the submodule managed deps in external/.

In the short/medium term, @kruceter could you make your change doubly deliberate: check that a make var is explicitly (PRIORITIZE_SYSTEM_DEPS ?) set AND the dep exists on the system already. In your case you very explicitly want to use the system version of cxxopts. Some people might not know their system already has it installed and it might have been a botched release and causes build issues and I'd really like the 'use system lib else use submodule' logic to be truly opt-in. Also could you do it in a generic way (maybe with a list of deps that are in external/ (initially of size 1 with just cxxopts) such that if this scenario arises again we can just add a string to a list and it'll work as desired. Also update the README with a blurb about this build logic.

[ghost]

@jshort, I like your suggestion. I was working on it for some time. I will tidy up my work a little and overwrite existing commits in the branch soon.

[codecov-commenter]

Codecov Report

Base: 72.84% // Head: 73.07% // Increases project coverage by +0.22% 🎉

Coverage data is based on head (f65b72a) compared to base (fde8a7c).
Patch has no changes to coverable lines.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
+ Coverage   72.84%   73.07%   +0.22%     
==========================================
  Files          50       50              
  Lines        3049     3049              
==========================================
+ Hits         2221     2228       +7     
+ Misses        828      821       -7     

Impacted Files	Coverage Δ
src/base/Connection.cpp	84.34% <0.00%> (-0.87%)	⬇️
test/ConnectionTest.cpp	94.94% <0.00%> (+0.50%)	⬆️
src/base/ServerConnection.cpp	77.35% <0.00%> (+6.60%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.