fix JNI target #26

jothepro · 2021-04-28T16:03:07Z

~~v0.1.0 is not yet available in conan-center-index but I am working on it. This PR updates the documentation according to the new configuration options that will be added in the new version.~~

This PR has been repurposed to propose a solution for the problem that we are currently discussing regarding JNI_OnLoad. Sry for the chaos! 🙈

This needs further discussion. All things not relevant to this change have been moved to #27

a4z

awesome!

jothepro · 2021-04-30T14:09:31Z

docs/install.md


 The library is available at [conan-center](https://conan.io/center/djinni-support-lib):

 ```text
 [requires]
-djinni-support-lib/0.0.1
+djinni-support-lib/0.1.1


I plan to release the canges in this PR as a patch release. To make the usage in cmake without conan possible, two adjustments are needed in the CMakeLists.txt

jothepro · 2021-04-30T14:12:51Z

CMakeLists.txt

@@ -55,6 +55,9 @@ if(DJINNI_STATIC_LIB)
 else()
  add_library(djinni_support_lib SHARED ${SRC_SHARED})
 endif()
+
+add_library(djinni-support-lib::djinni-support-lib ALIAS djinni_support_lib)


This is the name of the target that is defined if the library is installed with the conan cmake_find_package generator. I have added this alias to ensure that the target can always be referenced under the same name, no matter if installed via conan or by embedding the sources in cmake directly (see updated install instructions).

jothepro · 2021-04-30T14:15:54Z

CMakeLists.txt


 include(CTest)
-if (BUILD_TESTING)
+if (BUILD_TESTING AND DJINNI_BUILD_TESTING)


BUILD_TESTING is a global cmake variable that will affect the whole project. If the support-lib is installed by embedding the sources in cmake, setting the variable could potentially disable the tests for the parent project itself, too. In this case the new option DJINNI_BUILD_TESTING can be used to disable tests just for the support-lib.

you created answered while I wrote the question, that's great!

jothepro

I have added two new installation options. I have tested both in a local setup and they seem to work.

CMakeLists.txt

…l be set to define wether the library should be STATIC or SHARED. But if not defined, the global BUILD_SHARED_LIBS cmake flag is followed.

…e depends on it

jothepro · 2021-04-30T22:03:27Z

CMakeLists.txt

+# The target type of the library can be defined by setting `DJINNI_STATIC_LIB`.
+# If `DJINNI_STATIC_LIB` is not defined, the default CMake flag `BUILD_SHARED_LIBS` is followed.
+if(NOT DEFINED DJINNI_STATIC_LIB)
+  if(BUILD_SHARED_LIBS)
+    set(DJINNI_STATIC_LIB OFF)
+  else()
+    set(DJINNI_STATIC_LIB ON)
+  endif()
+endif()
+


ok, I decided to go down another path than what we discussed in slack to resolve #23. It was a priority for me to not break existing setups. And because the variable DJINNI_STATIC_LIB is used twice in the configuration of the target, it has to be set anyways. That's why it seemed the easiest solution to just set it according to BUILD_SHARED_LIB if undefined. This does not use the default CMake mechanism when defining the target, but simulates it.

What do you think about that? I have mixed feelings. The downside of this is that it introduces additional complexity. Reducing complexity would only be possible by introducing a breaking change. I think I have at least figured out a solution that introduces as little additional code as possible.

Btw do you know why the ObjC flavor of the lib is only linked to the Foundation framework if built as shared lib?

if (NOT DJINNI_STATIC_LIB) target_link_libraries(djinni_support_lib "-framework Foundation" ) endif()

The only explanation that I have is: "If this is a static lib the library that will link to this will most likely link the Foundation framework anyways. So screw it here."

I think what you suggested on slack, second way, is the best solution.
If it breaks things, lets break it (with a new version number).
Complexity kills, especially if it is not required, and this is not required since it gives us no great functionality.
For a project like that , where people drop out and in, and you might have a breaks between reading the code where you lose the context, readability and maintainability must be the number 1 priority.

--
If you have a static lib and use it, yourself needs to link against foundation yourself
Your app will only take from the static lib what it needs, and embed it.

If you use a dynamic library nothing gets embedded into your app, it just says I need these symbols and expect them to be found at runtime. The dynamic library does the same with an other dynamic lib it depends on.

But a static library does/can not do that, it says, if you use me, I expect you to provide the external symbol, either via an other static library at link time (than pls get the link order right) or at runtime via an other dynamic library

…e DJINNI_LIBRARY_TYPE

…tic library will always be linked to a shared library.

a4z · 2021-05-01T12:33:12Z

Thanks for the cmake simplification!
Interesting, the Ubuntu build is missing the djinni_main.cpp implementation (what I can see via mobile)

…d & JNI_OnUnload are now part of the public library interface. This means any user can include them in his sources to be able to use the default implementation. djinni-generator v1.0.0 will support this approach by automatically generating a file that includes the JNI_OnLoad implementation from the support-lib.

jothepro

This got out of hand pretty quickly... 🙈

With the new commit I am trying to address the problem that we discussed in slack in the past days: How should the user include/implement a default JNI_OnLoad and JNI_OnUnload implementation.

I realized that this is an issue when I tried to make the Java tests link to the support-lib defined by cmake instead of globbing for the source files.

My strategy is the following:

djinni_jni_main.h is a header file that implements JNI_OnLoad and JNI_OnUnload. It is part of the djinni-support-lib ABI. If a user links a static support-lib, he can include the header file himself to make use of the default JNI_OnLoad and JNI_OnUnload implementation.

To make things easier for the end-user, this PR is complemented by changes in the generator: cross-language-cpp/djinni-generator#49

All of these changes will most likely break some existing setups, thats why I think this is the right time to introduce v1.0.0 for both the generator & support-lib.

jothepro · 2021-05-01T16:10:51Z

djinni/jni/djinni_jni_main.cpp

+// limitations under the License.
+//
+
+#include "djinni_jni_main.hpp"


This new files only purpose is to inclued the JNI_OnLoad implementation from djinni_jni_main.hpp.

jothepro · 2021-05-01T16:13:22Z

djinni/jni/djinni_jni_main.hpp

+// This provides a minimal JNI_OnLoad and JNI_OnUnload implementation.
+// Don't include it if your library does require a custom JNI_OnLoad implementation with custom initialization logic.
+#pragma once


I've transformed djinni_main.cpp to a header file. That way anybody can include it and get the JNI_OnLoad implementation. This only works if the header is included once. Otherwise the linker will complain.

jothepro · 2021-05-01T16:24:51Z

This is expected to fail for JNI. It requires changes in the djinni-generator to work.

CMakeLists.txt

freitass · 2021-05-01T18:07:04Z

djinni/jni/djinni_support.hpp

+#include "djinni/proxy_cache_interface.hpp"
+#include "djinni/djinni_common.hpp"


Nice! I see .. in include directives as a smell (even though we had some weird stuff in my previous PRs 😅)

…Otherwise both would generate a main file & which would result in colliding filenames/symbols

…files

a4z

Thank you, great work!

…ib into update_conan_install_instructions

update conan install instructions

7893d50

jothepro requested a review from a4z April 28, 2021 16:03

a4z approved these changes Apr 28, 2021

View reviewed changes

improve install instructions

158c5be

jothepro requested a review from a4z April 30, 2021 14:08

jothepro commented Apr 30, 2021

View reviewed changes

jothepro requested a review from freitass April 30, 2021 14:09

jothepro commented Apr 30, 2021

View reviewed changes

a4z reviewed Apr 30, 2021

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

a4z approved these changes Apr 30, 2021

View reviewed changes

jothepro added 2 commits April 30, 2021 23:36

remove mandatory DJINNI_STATIC_LIB option. DJINNI_STATIC_LIB can stil…

531d172

…l be set to define wether the library should be STATIC or SHARED. But if not defined, the global BUILD_SHARED_LIBS cmake flag is followed.

make sure BUILD_SHARED_LIB is always set, because the rest of the fil…

daa0e0f

…e depends on it

jothepro commented Apr 30, 2021

View reviewed changes

jothepro added 2 commits May 1, 2021 12:31

reduce complexity of library configuration by introducing the variabl…

098f7e9

…e DJINNI_LIBRARY_TYPE

fix JNI build: always enable -fPIC for JNI, because the resulting sta…

bab9f04

…tic library will always be linked to a shared library.

jothepro marked this pull request as draft May 1, 2021 11:24

jothepro commented May 1, 2021

View reviewed changes

freitass approved these changes May 1, 2021

View reviewed changes

fix JNI build: disable --jni-generate-main for second generator run. …

52bad5c

…Otherwise both would generate a main file & which would result in colliding filenames/symbols

jothepro mentioned this pull request May 2, 2021

Improve install instructions, reduce configuration complexity #27

Merged

jothepro changed the title ~~update conan install instructions~~ fix JNI target May 2, 2021

jothepro added 2 commits May 7, 2021 12:14

update djinni generator version to v1.0.0

4c6f601

fix github actions: add new djinni_jni_main.hpp to list of installed …

cc586eb

…files

a4z approved these changes May 7, 2021

View reviewed changes

Merge branch 'main' of github.com:cross-language-cpp/djinni-support-l…

3e0d40e

…ib into update_conan_install_instructions

jothepro marked this pull request as ready for review May 7, 2021 13:03

jothepro merged commit 34f07aa into cross-language-cpp:main May 7, 2021

jothepro mentioned this pull request May 13, 2021

Use CMake djinni-support-lib target in integration tests #22

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix JNI target #26

fix JNI target #26

jothepro commented Apr 28, 2021 •

edited

Loading

a4z left a comment

jothepro Apr 30, 2021

jothepro Apr 30, 2021 •

edited

Loading

jothepro Apr 30, 2021

a4z Apr 30, 2021

jothepro left a comment

jothepro Apr 30, 2021

jothepro Apr 30, 2021

a4z May 1, 2021

a4z commented May 1, 2021

jothepro left a comment

jothepro May 1, 2021

jothepro May 1, 2021

jothepro commented May 1, 2021

freitass May 1, 2021

a4z left a comment

		#include "djinni/proxy_cache_interface.hpp"
		#include "djinni/djinni_common.hpp"

fix JNI target #26

fix JNI target #26

Conversation

jothepro commented Apr 28, 2021 • edited Loading

a4z left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jothepro Apr 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jothepro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a4z commented May 1, 2021

jothepro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jothepro commented May 1, 2021

Choose a reason for hiding this comment

a4z left a comment

Choose a reason for hiding this comment

jothepro commented Apr 28, 2021 •

edited

Loading

jothepro Apr 30, 2021 •

edited

Loading