Support additional CMake build options

[alebcay]

This pull request proposes additional build options to the CMake-based install. Some files have been moved around/touched slightly also to facilitate some of these options/features, which are described below. These options would be helpful for building/shipping libsdptransform for use in package managers and other distributions.

The defaults for these new options have been set to follow the existing behavior at build right now (with no options), so build commands/scripts should work like before and produce the same build outputs.

The changes here are not trivial, so please advise if there's anything that you don't like here and we can hopefully find some happy medium or workaround.

---

• Add option BUILD_TESTS. Default to ON (following existing behavior)
• Add option BUILD_README_HELPER. Default to ON (following existing behavior).
• Add option BUILD_SHARED_LIBS. Default to OFF (following existing behavior).
• Add option USE_EXTERNAL_JSON. Default to OFF (following existing behavior).

BUILD_TESTS and BUILD_README_HELPER toggle whether contents in subdirectories test and readme-helper will be built.

BUILD_SHARED_LIBS toggles whether libsdptransform will be built as a static or shared library. add_library automatically checks the value of BUILD_SHARED_LIBS at configure/build time.

USE_EXTERNAL_JSON toggles whether an external (e.g. system) copy of nlohmann-json should be used instead of the bundled one. If enabled, CMake will automatically search for an installation of nlohmann-json on the system and configure step will fail if not found. If nlohmann-json is installed in a non-standard location, CMAKE_MODULE_PATH can be set so that CMake can find the nlohmann-json installation.

json.hpp has been moved from include/ to new folder thirdparty/, so that the thirdparty folder can be added as an include directory when external nlohmann-json is not in use.

include/sdptransform.hpp is now include/sdptransform.hpp.in. At configure time, the .hpp.in file is preprocessed into the build tree. This step is necessary in order to set the #include path to json.hpp in sdptransform.hpp.

[ibc]

@jmillan can you take a look?

[jmillan]

Hi @alebcay,

I find the changes OK overall, but this PR modifies a file in sdptransform, which is an external dependency that cannot be modified here.

[alebcay]

Thanks for taking a look.

I find the changes OK overall, but this PR modifies a file in sdptransform, which is an external dependency that cannot be modified here.

Just wanted to clarify - are you referring to sdptransform.hpp? And is the issue that I've modified it (touched the line for the #include "json.hpp"), or is the issue that the build now preprocesses this file at configure time (substitutes @JSON_HPP_LOCATION@ with include file path)?

[ibc]

sdptransform.hpp is part of a separate library that we include in libmediasoupclient. We cannot modify it here. Please look for sdptransform project in Versatica GitHub repositories.

[alebcay]

I see, you are referring to https://github.com/versatica/libmediasoupclient/tree/v3/deps/libsdptransform.

Would the correct course of action be to open this PR in that repository instead, to make the changes there first?

[ibc]

I see, you are referring to https://github.com/versatica/libmediasoupclient/tree/v3/deps/libsdptransform.

Would the correct course of action be to open this PR in that repository instead, to make the changes there first?

Exactly :)

Then we release a new version of libsdptransform, and current PR must update it.

[alebcay]

I see, thanks. It was unclear to me whether here or the other repository was the "upstream".

Can I make the CMake-related changes in this PR over there too? The modification to sdptransform.hpp needs to happen together with the CMake-related changes in this PR.

[ibc]

Not sure if I follow. Can you make a PR to libsdptransform, we release a new version and then you update this PR with that new version? See scripts/get-dep.sh in this repo.

[alebcay]

Sure, opened https://github.com/versatica/libmediasoupclient/pull/125.

[jmillan]

Ok. This PR already targets libdsptransform 🤦 , sorry for the confusion @alebcay. We were thinking it was targeting libmediasoupclient.

I just have one question:

- install(TARGETS sdptransform DESTINATION lib)
+ install(TARGETS sdptransform)

why the removal of the DESTINATION?

[alebcay]

The sdptransform target contains a library (libsdptransform), and the implicit/default destination for installing those is lib.

It can be added back of course; my understanding however is that it's unnecessary.

[ibc]

@alebcay libsdptransform 1.2.10 has been released and comes with Catch v3. Can you adapt your PR to it?