DEWETRON / OXYGEN-SDK

DEWETRON OXYGEN SDK. OXYGEN API description and example sources.
https://dewetron.github.io/OXYGEN-SDK
MIT License
9 stars 8 forks source link

Add `install` commands to the ODK cmakelists to export targets to the cmake install directory #34

Closed willjschmitt closed 3 months ago

willjschmitt commented 3 months ago

We use a non-cmake build system (Bazel) to manage a monorepo, which means we need a clean wall between the output artifacts of the ODK and our plugin implementations, which will be built using Bazel and our own compiler toolchain configurations. Bazel supports builds depending on cmake-built libraries via rules_foreign_cc, specifically the cmake rule, but the rule depend on artifacts to be written to the install directory, rather than reaching into the intermediate build-directory. I could propose to them to try to support reaching into build-directories, but I figure that will be a bigger reach than additively supporting install commands in this project. I think it should be pretty compatible with the existing cmakelists files, and the pugixml third-party lib offers an example of installing its headers and libraries to the install directory in a generalized and unintrusive way: https://github.com/DEWETRON/OXYGEN-SDK/blob/master/3rdparty/pugixml-1.9/CMakeLists.txt#L67-L72

For now, we're monkey patching in the install commands, which I'm happy to contribute as a PR if it's compatible with how you're thinking about your cmake builds. Here's the patch (with one additional line needing adding where there was a missing header file in the HEADERS variable for the api library, which I'll submit a PR for separately anyways):

diff --git a/odk/api/CMakeLists.txt b/odk/api/CMakeLists.txt
index d180d39..3328a5a 100644
--- a/odk/api/CMakeLists.txt
+++ b/odk/api/CMakeLists.txt
@@ -33,6 +33,7 @@ set(ODK_API_HEADER_FILES
     inc/odkapi_oxygen_queries.h
     inc/odkapi_property_xml.h
     inc/odkapi_property_list_xml.h
+    inc/odkapi_pugixml_fwd.h
     inc/odkapi_software_channel_xml.h
     inc/odkapi_timebase_xml.h
     inc/odkapi_timestamp_xml.h
@@ -92,3 +93,13 @@ endif()
 if (WITH_ODK_TESTS)
   add_subdirectory(unit_tests)
 endif (WITH_ODK_TESTS)
+
+set_target_properties(${LIBNAME}
+  PROPERTIES
+  PUBLIC_HEADER "${ODK_API_HEADER_FILES}"
+)
+install(TARGETS ${LIBNAME}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} 
+  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} 
+)
+
diff --git a/odk/base/CMakeLists.txt b/odk/base/CMakeLists.txt
index 5213240..5a76d7a 100644
--- a/odk/base/CMakeLists.txt
+++ b/odk/base/CMakeLists.txt
@@ -36,3 +36,13 @@ endif()
 if (WITH_ODK_TESTS)
   add_subdirectory(unit_tests)
 endif (WITH_ODK_TESTS)
+
+set_target_properties(${LIBNAME}
+  PROPERTIES
+  PUBLIC_HEADER "${ODK_BASE_HEADER_FILES}"
+)
+install(TARGETS ${LIBNAME}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} 
+  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} 
+)
+
diff --git a/odk/framework/CMakeLists.txt b/odk/framework/CMakeLists.txt
index f7df454..b7ae7f3 100644
--- a/odk/framework/CMakeLists.txt
+++ b/odk/framework/CMakeLists.txt
@@ -88,3 +88,13 @@ endif()
 if (WITH_ODK_TESTS)
   add_subdirectory(unit_tests)
 endif()
+
+set_target_properties(${LIBNAME}
+  PROPERTIES
+  PUBLIC_HEADER "${HEADER_FILES}"
+)
+install(TARGETS ${LIBNAME}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} 
+  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} 
+)
+
diff --git a/odk/uni/CMakeLists.txt b/odk/uni/CMakeLists.txt
index df7fe65..1a745bb 100644
--- a/odk/uni/CMakeLists.txt
+++ b/odk/uni/CMakeLists.txt
@@ -47,3 +47,13 @@ set_target_properties(${LIBNAME} PROPERTIES FOLDER "odk")
 if (CLANG_TIDY_COMMAND)
   set_target_properties(${LIBNAME} PROPERTIES CXX_CLANG_TIDY "${CLANG_TIDY_COMMAND}")
 endif()
+
+set_target_properties(${LIBNAME}
+  PROPERTIES
+  PUBLIC_HEADER "${ODK_UNI_HEADER_FILES}"
+)
+install(TARGETS ${LIBNAME}
+  LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR} 
+  PUBLIC_HEADER DESTINATION ${CMAKE_INSTALL_INCLUDEDIR} 
+)
+
matthiasstraka commented 3 months ago

It makes absolutely sense to have install information in the CMake files. I integrated your proposal into the default branch and merged your #35. Let me know if your issue is resolved now.

willjschmitt commented 3 months ago

Perfect! Thanks so much. This works exactly as desired referencing HEAD, and confirmed end-to-end I'm set up with a successful bazel build (for Windows and Linux) and loaded the plugin successfully into Oxygen (Windows only at this point)

matthiasstraka commented 3 months ago

Thanks for the feedback, I added a small installation change where I put include files in /usr/local/include/odk/* now. I hope this causes no problems for your workflow, but it makes include files better grouped.

willjschmitt commented 3 months ago

That shouldn't impact our workflow, since we can strip the odk/ suffix off the install path in our build, but it does sound like you're hinting at the includes including odk/ as a prefix:

include "odk/odkfw_software_channel_plugin.h"

which is at odds with the current include statements throughout the repo.

Otherwise, I think folks will need to explicitly add the namespaced include path, which isn't a huge deal and doesn't impact our build (since odk/ will be stripped anyways), but just adds more compiler flag configuration for projects compiling against it and sortof defeats installing to the system include paths