CMake to build djinni support library #303
Conversation
|
Automated message from Dropbox CLA bot @4brunu, it looks like you've already signed the Dropbox CLA. Thanks! |
There was a problem hiding this comment.
I don't use CMake, so my suggestions here might be off-base, but I thought I'd give some high-level suggestions to drive discussion.
I'm fine with this as a way to decouple CMake support from #281. I'm pretty willing to take your word for it, merge this when you say it's ready, and let future discussion/PRs drive more changes if necessary. Since this is a new file it shouldn't impact any existing users (I assume CMake won't be broken by stray unused CMakeLists files, the way that Buck will).
support-lib/CMakeLists.txt
Outdated
|
|
||
| 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}) |
There was a problem hiding this comment.
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.
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.
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.
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.
CMake can't do two target platforms at once.
support-lib/CMakeLists.txt
Outdated
| 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?
I worry a bit about users' wanting their full codebase (including Djinni-generated files, and the support library) to be built with the same standard version, which could be >11
There was a problem hiding this comment.
This is just the default value, I'm using c++14 with std::experimental::optional in the Djinni-generated files without any issues
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.
Let's say most of my app is compiled with C++17, and that's my global CMake setting. Does my global setting override, or does the support-lib build with C++11? If the latter, what happens to an inline function which doesn't get inlined, or to a template instantiation, which may show up in C++11 and C++17 versions in different compilation units. When the final link happens, one of them has to be picked. I'm not confident that they're guaranteed to be binary compatible with each other. Anyway, to be safe I'd want all the code in my app to be built agains the same C++ standard.
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.
support-lib/CMakeLists.txt
Outdated
| 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:/android-ndk/ndk/wiki/Changelog-r14#cmake
There was a problem hiding this comment.
Should I remove the target_compile_options?
support-lib/CMakeLists.txt
Outdated
| 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 jni.h is automatically in the include path, otherwise the FindJNI package comes shipped with CMake and looks in the environment for the file.
|
@artwyman @mknejp do you think the |
|
I would put it in the root so it is immediately visible to users and they know it's there. It not only builds the support lib but also exports a variable that points to the generator. |
|
I'd have said the opposite, and put it in the support-lib directory, so as to keep it separate from other things which it doesn't build, such as the test suite and sample apps. Pointing to the generator is a good point, which is outside of that model, though it doesn't seem unreasonable for the support-lib to point to the generator regardless. I won't be religious about it, though, as I'm happy to let the users of CMake own the best-practices there. |
|
The tests aren't included right now but that can be added later. Having a root CMakeLists.txt helps users identify what to do. I have yet to see a CMake-supporting project that doesn't have a list file in the root. The tests, examples, support-lib etc can all be put in subdirectories and included from the root depending on what options are set. |
|
Indeed all the projects with CMake support that I have seen, have the file in the root directory, and this way, this could expand in the future to support more things. |
|
That seems like a good suggestion, to future-proof the naming. |
…djinni_support_lib
|
I have moved the file to the root directory and rename the target to djinni_support_lib. |
|
👍 |
Since #248 is pending of #281 since July, I created a modified version of #248 for the current Djinni structure, this way, the current users of Djinni can have CMake support for building djinni support library, and when #281 get merged, #248 can replace this PR.
The CMakeLists.txt allows to build the support-lib with CMake. The target name is djinni and it comes with a few options:
DJINNI_WITH_OBJC adds the Objective-C support files to the target
DJINNI_WITH_JNI adds the JNI support files to the target
DJINNI_STATIC_LIB builds Djinni as a static library instead of the default dynamic
Because the CXX_STANDARD property cannot be propagated to consuming targets it needs to be set to >= 11 manually there.
The cache variable DJINNI_RUN_PATH is set to the location of Djinni's run script so it can be passed as argument to add_custom_command()-based generator scripts.
I added the
CMakeList.txtto support-lib directory, since it's job is to build the support library, but I'm not sure what's the best place to put it, in the root directory or support-lib directory.Please let me know what do you think.
All credit's should go to @mknejp I only updated the
CMakeList.txtfor the current structure, and removed the Framework support, since it's not compatible with the current Djinni structure. When#281 get's merged, the Framework support can be added with #248.