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

Add ability to build with system installed jsoncpp #1178

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lubosz
Copy link

@lubosz lubosz commented Aug 15, 2019

This first patch in this patch set adds the build option USE_SYSTEM_JSONCPP, which instead of using the internal version of jsoncpp.cpp links to a system installed one.

Even though I didn't need the second patch to make it work for me, it removes the local json.h from the CMake include path, which could cause problems on certain systems when building with USE_SYSTEM_JSONCPP. The second patch moves jsoncpp to a thirdparty directory and includes it when the system version should not be used.

This is an attempt to solve #1176 in a upstreamable fashion, keeping the previous behavior when the build flag is not set.

The problem was discovered during the packaging process of xrdesktop for Debian. This patch set will enable conformance with the Debian policy "embedded-library", as seen in https://lintian.debian.org/tags/embedded-library.html

I was able to build the examples (after applying #1177), as well as run xrdesktop in a system wide installation with these patches.

This patch adds the CMake build option `USE_SYSTEM_JSONCPP`,
which instead of using the internal `jsoncpp.cpp` links against
the system installed jsoncpp library.

This results in a `libopenvr_api.so` that is only 115K instead of 301K
on my system.

Distributions like Debian will prefer this behaviour, since it will give
them the possibility to maintain version and security issues for jsoncpp
in one place.

This behaviour can be enabled by

```
cmake -DUSE_SYSTEM_JSONCPP=True .
```

When the flag is not set, the build will behave like it did before.

I tested this patch using the system wide jsoncpp successfully
with xrdesktop.
Even though the previous patch in this patch set enabled the build with
a system wide jsoncpp and worked for me, it still left the internal
jsoncpp includes in the include path, since the includes were placed
in `CMAKE_CURRENT_SOURCE_DIR`. This could cause problems on other systems
when trying to build with a system wide jsoncpp.

In order to remove the internal json.h from the include path,
I moved all jsoncpp files into a thridparty directory amd include
it in the case of `USE_SYSTEM_JSONCPP` not being set.
ChristophHaag added a commit to ChristophHaag/openvr that referenced this pull request Mar 31, 2020
std::istringstream requires this include.
Currently it is accidentally pulled in by including json.h.
Other jsoncpp versions don't provide that include, for example with ValveSoftware#1178
the build breaks on debian stable.
@ArchangeGabriel
Copy link

@lubosz Could you rebase this PR on current master? Else I will just open an updated one.

@ArchangeGabriel
Copy link

Rebased: #1716.

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.

2 participants