Support additional CMake build options #20
Conversation
- 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.
With changes in top-level CMakeLists.txt logic, sdptransform's includes are automatically added since readme-helper links to the sdptransform target.
With changes in top-level CMakeLists.txt logic, sdptransform's includes are automatically added since test links to the sdptransform target.
|
@jmillan can you take a look? |
|
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. |
|
Thanks for taking a look.
Just wanted to clarify - are you referring to |
|
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. |
|
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. |
|
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 |
|
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. |
|
Sure, opened versatica/libmediasoupclient#125. |
|
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? |
|
The It can be added back of course; my understanding however is that it's unnecessary. |
|
@alebcay libsdptransform 1.2.10 has been released and comes with Catch v3. Can you adapt your PR to it? |
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
libsdptransformfor 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.
BUILD_TESTS. Default toON(following existing behavior)BUILD_README_HELPER. Default toON(following existing behavior).BUILD_SHARED_LIBS. Default toOFF(following existing behavior).USE_EXTERNAL_JSON. Default toOFF(following existing behavior).BUILD_TESTSandBUILD_README_HELPERtoggle whether contents in subdirectoriestestandreadme-helperwill be built.BUILD_SHARED_LIBStoggles whether libsdptransform will be built as a static or shared library.add_libraryautomatically checks the value ofBUILD_SHARED_LIBSat 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_PATHcan be set so that CMake can find the nlohmann-json installation.json.hpphas been moved frominclude/to new folderthirdparty/, so that thethirdpartyfolder can be added as an include directory when external nlohmann-json is not in use.include/sdptransform.hppis nowinclude/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#includepath tojson.hppinsdptransform.hpp.