CMake to build djinni support library #303
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,76 @@ | ||
| cmake_minimum_required(VERSION 3.6.0) | ||
|
|
||
| project(Djinni) | ||
|
|
||
| include(GNUInstallDirs) | ||
|
|
||
| set(SRC_SHARED | ||
| "djinni_common.hpp" | ||
| "proxy_cache_interface.hpp" | ||
| "proxy_cache_impl.hpp" | ||
| ) | ||
|
|
||
| set(SRC_JNI | ||
| "jni/djinni_support.hpp" | ||
| "jni/Marshal.hpp" | ||
| "jni/djinni_support.cpp" | ||
| ) | ||
|
|
||
| set(SRC_OBJC | ||
| "objc/DJICppWrapperCache+Private.h" | ||
| "objc/DJIError.h" | ||
| "objc/DJIMarshal+Private.h" | ||
| "objc/DJIObjcWrapperCache+Private.h" | ||
| "objc/DJIError.mm" | ||
| "objc/DJIProxyCaches.mm" | ||
| ) | ||
|
|
||
| option(DJINNI_STATIC_LIB "Build Djinni as a static library instead of dynamic (the default)." off) | ||
| if(DJINNI_STATIC_LIB) | ||
| add_library(djinni STATIC ${SRC_SHARED}) | ||
| else() | ||
| add_library(djinni SHARED ${SRC_SHARED}) | ||
| endif() | ||
| source_group("" FILES ${SRC_SHARED}) | ||
|
|
||
| set_target_properties(djinni PROPERTIES | ||
| CXX_STANDARD 11 | ||
|
There was a problem hiding this comment. Any way to make this a minimum requirement rather than an absolute one? There was a problem hiding this comment. This is just the default value, I'm using c++14 with There was a problem hiding this comment. This only controls what the support library is built with. It has no effect on anything else. There was a problem hiding this comment. I'm not sure if your two answers are contradictory or overlapping. If this is a default value which can be overridden globally, that seems safe enough. If it's a strict value for building the support lib, I think that might not be isolated enough due to the large amount of header-based code in the support-lib, which could get out of sync if the support lib's sources are built with different settings from other sources which use support-lib headers. There was a problem hiding this comment. "not isolated enough" for what? "out of sync" with what? I don't understand the concern. The support lib has to be built with C++11 or newer, same applies to its headers. Both 14 and 17 are backwards compatible to 11. There was a problem hiding this comment. I realize this is a theoretical concern, but I'm just trying to understand the situation. There was a problem hiding this comment. They have to be binary compatible since the language version doesn't affect ABI. If the function has the same token sequence under both compilations (which it does) then it doesn't result in an ODR violation. If that were the case lots and lots of libraries wouldn't work. |
||
| CXX_STANDARD_REQUIRED true | ||
| CXX_EXTENSIONS false | ||
| ) | ||
|
|
||
| # Objective-C support | ||
| option(DJINNI_WITH_OBJC "Include the Objective-C support code in Djinni." off) | ||
| if(DJINNI_WITH_OBJC) | ||
| target_include_directories(djinni PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/objc/>") | ||
| target_sources(djinni PRIVATE ${SRC_OBJC}) | ||
| source_group("objc" FILES ${SRC_OBJC}) | ||
| target_compile_options(djinni PUBLIC "-fobjc-arc") | ||
| endif() | ||
|
|
||
| # JNI support | ||
| option(DJINNI_WITH_JNI "Include the JNI support code in Djinni." off) | ||
| if(DJINNI_WITH_JNI) | ||
| if(NOT DJINNI_STATIC_LIB) | ||
| list(APPEND SRC_JNI "jni/djinni_main.cpp") | ||
| endif() | ||
| target_include_directories(djinni PUBLIC "$<BUILD_INTERFACE:${CMAKE_CURRENT_SOURCE_DIR}/jni/>") | ||
| target_sources(djinni PRIVATE ${SRC_JNI}) | ||
| source_group("jni" FILES ${SRC_JNI}) | ||
| # Do not use the host's jni.h on Android as it is provided automatically by the NDK | ||
| if(NOT ANDROID) | ||
| find_package(JNI REQUIRED QUIET) | ||
|
There was a problem hiding this comment. Is this a standard/published package for CMake on non-Android platforms? There was a problem hiding this comment. I'm not a user of this, but I think so. There was a problem hiding this comment. All platforms CMake runs on are "non-Android". When building with an Android toolchain file (as Android Studio does) then |
||
| target_include_directories(djinni PUBLIC "${JNI_INCLUDE_DIRS}") | ||
| endif() | ||
| # Exceptions and RTTI are disabled in the NDK by default | ||
| if(ANDROID) | ||
| target_compile_options(djinni PRIVATE "-fexceptions" "-frtti") | ||
|
There was a problem hiding this comment. Why are these Android-only? Djinni requires these features to be enabled in all cases. It seems odd to assume that Android is the only platform where they're disabled by default. There was a problem hiding this comment. @mknejp Did I put this in the right place? There was a problem hiding this comment. I'm a bit wary about this. When under Android we know what the compiler is, and that it knows these two flags, outside the guard we no longer know this and someone might very well use MSVC to compile the Java parts. Android is the only toolchain where exceptions are explicitly disabled for backwards compatibility, although it seems they have reverted this for their CMake toolchain and they are no longer disabled, see https://github.com/android-ndk/ndk/wiki/Changelog-r14#cmake There was a problem hiding this comment. Should I remove the |
||
| endif() | ||
| endif() | ||
|
|
||
| if(NOT (DJINNI_WITH_OBJC OR DJINNI_WITH_JNI)) | ||
| message(FATAL_ERROR "At least one of DJINNI_WITH_OBJC or DJINNI_WITH_JNI must be enabled.") | ||
| endif() | ||
|
|
||
| # Store path to the "run" executable so it can be passed as argument to add_custom_command() scripts | ||
| set(DJINNI_RUN_PATH "${CMAKE_CURRENT_SOURCE_DIR}/src/run" CACHE FILEPATH "Path to the Djinni generator executable.") | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not familiar with CMake best-practices, but what's the trade-off of using these config variables to configure building a single library, vs. having distinct library targets for distinct purposes? In the gyp version of this, there are distinct targets for djinni_jni and djinni_objc. The same could be done for static vs. dynamic if desired.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having separate libraries means someone who needs both languages (for example on macOS) will end up with two libraries and ambiguous header files, or needs to prefix include paths and they they're no longer compatible between the two libraries, etc. Having one library is just easier for everyone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a CMake expert, so I'm not sure what's the answer to this, but the projects that I use, tend to do configuration with options.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, the case of a single library with both languages is one I hadn't thought about, since it doesn't come up in our use cases.
I wonder about the converse case with two different libraries by the same name, used on different platforms. I guess in most build systems the platforms might be separated enough on their own to not cause confusion? In our use cases, we use distinct names to avoid confusion in shared files, but since we use different build tools on most platforms, the only real confusion occurs between iOS and Mac, which both use Xcode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CMake can't do two target platforms at once.