CORRADE_BUILD_STATIC is forced for Android / shared

[bansan85]

So cmake_dependent_option which depend on CORRADE_BUILD_STATIC must be defined after.

I think there is the same problem for magnum.

BTW I don't understand why you don't want shared build for Android. If user thinks it's too boring to use corrade/magnum with shared library, he may set BUILD_STATIC=ON.

[mosra]

Thanks, the change makes sense I think. I'll check locally and do the same for Magnum as well.

I don't understand why you don't want shared build for Android.

It's because -- as far as I know -- one would have to load the shared libraries one by one from the APK virtual filesystem with JNI, which is a lot of extra work as you can't have the app with native-only code anymore. And it doesn't really make sense as you don't gain anything from using them, every app is sandboxed so sharing the libraries for lower memory use isn't possible anyway.

A related topic would be dynamic plugins on Android, which I also didn't look into implementing yet because of similar issues, the apps having no actual filesystem they could detect and load the libraries from.

[bansan85]

The CMakeLists.txt forces to use BUILD_STATIC but I couldn't set BUILD_STATIC_UNIQUE_GLOBALS (which is annoying because I use Magnum from 2 different libraries). Finally, I could set it after moving cmake_dependent_option after set(CORRADE_BUILD_STATIC ON)

My library tree:

jni.so
  + lib1.so
    + libMagnumXXX.a
      + libCorradeXXX.a
  + lib2.so
    + libMagnumXXX.a
      + libCorradeXXX.a

For your information, I tried to disable set(BUILD_STATIC ON) in Corrade/Magnum (I had about 10 libraries) and I just had to loadlibrary(jni.so) in my Android/Java code to make my app work. (I just had to disabled TESTSUITE because DebugTool depend on it and TestSuite.so was not automatically added in the Android package. Maybe a tiny CMake issue but I didn't investigate).

I understand the disinterest in shared library. I maintains an app in shared/static mode. But sometimes, I wonder what the benefit about maintaining them both.

[mosra]

I see, okay, so you punched through the problems already :) Yes then having a static library in this setting is not good due to the symbol duplication. It gives so many problems that it's not worth it, and even with *_BUILD_STATIC_UNIQUE_GLOBALS there are still some broken corner cases, unfortunately.

In any case -- good to know static shared libraries are easier to handle than I thought :) Can you update the PR to make it default to static but possible to override? Something like

if(CMAKE_SYSTEM_NAME STREQUAL Android)
    set(OFF_EXCEPT_ANDROID ON)
else()
    set(OFF_EXCEPT_ANDROID OFF)
endif()
option(CORRADE_BUILD_STATIC "Build static libraries (default are shared)" ${OFF_EXCEPT_ANDROID})

should make your use case work, hopefully? It just won't be a well-supported / tested configuration from my side, for now at least.

I just had to disabled TESTSUITE because DebugTool depend on it and TestSuite.so was not automatically added in the Android package

This is interesting -- can you investigate? What is responsible for adding the dependencies to the Android package? Thanks a lot 🙏

[bansan85]

good to know static libraries are easier

shared libraries, you mean ?

I update my commit.

I didn't force option for emscripten because it looks to support dynamic linking. I will now take a look at DebugTool and TestSuite.

[mosra]

shared libraries, you mean ?

Shared libraries, yes, sorry.

I didn't force option for emscripten because it looks to support dynamic linking

Yes, good idea. It's annoying but possible, so no reason to force it there either.

I will now take a look at DebugTool and TestSuite.

Thanks!

[codecov]

Codecov Report

Patch and project coverage have no change.

Comparison is base (292e565) 98.13% compared to head (164d6a0) 98.13%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #170   +/-   ##
=======================================
  Coverage   98.13%   98.13%           
=======================================
  Files         139      139           
  Lines       11775    11775           
=======================================
  Hits        11555    11555           
  Misses        220      220           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[bansan85]

To fix the problem that Android studio / gradle / cmake doesn't embedded libCorradeTestSuite-d.so in the Android package, I added find_package(Corrade REQUIRED ... TestSuite) and target_link_libraries(myTarget PUBLIC Corrade::TestSuite). Just find_package is not enough.

It's strange I had to add target_link_libraries(... Corrade::TestSuite) since Corrade::TestSuite is declared PUBLIC in target_link_libraries(MagnumDebugTools). But I'm not an Android expert.

By the way, to be able to find_package(Corrade REQUIRED TestSuite), I had to change in FindCorrade.cmake :

find_file(CORRADE_TESTSUITE_ADB_RUNNER AdbRunner.sh
    PATH_SUFFIXES share/corrade/TestSuite)

to

find_file(CORRADE_TESTSUITE_ADB_RUNNER AdbRunner.sh
    PATH_SUFFIXES share/corrade/TestSuite PATHS ${CMAKE_FIND_ROOT_PATH})

It's odd because I thought that CMAKE_FIND_ROOT_PATH is in default path.

[mosra]

I think I know what's the problem with DebugTools and TestSuite. Could you try applying this patch to the FindMagnum.cmake files (all of them, to be sure)? That should properly mark the dependency between the two libraries.

diff --git a/modules/FindMagnum.cmake b/modules/FindMagnum.cmake
index 3e17e0a68..022602673 100644
--- a/modules/FindMagnum.cmake
+++ b/modules/FindMagnum.cmake
@@ -231,10 +231,20 @@ set(_MAGNUM_CORRADE_DEPENDENCIES )
 foreach(_component ${Magnum_FIND_COMPONENTS})
     string(TOUPPER ${_component} _COMPONENT)
 
+    set(_MAGNUM_${_COMPONENT}_CORRADE_DEPENDENCIES )
+
     # Unrolling the transitive dependencies here so this doesn't need to be
     # after resolving inter-component dependencies. Listing also all plugins.
     if(_component MATCHES "^(Audio|DebugTools|MeshTools|Primitives|SceneTools|ShaderTools|Text|TextureTools|Trade|.+Importer|.+ImageConverter|.+Font|.+ShaderConverter)$")
-        set(_MAGNUM_${_COMPONENT}_CORRADE_DEPENDENCIES PluginManager)
+        list(APPEND _MAGNUM_${_COMPONENT}_CORRADE_DEPENDENCIES PluginManager)
+    endif()
+    if(_component STREQUAL DebugTools)
+        # DebugTools depends on TestSuite optionally, so if it's not there
+        # assume it wasn't compiled against it.
+        find_package(Corrade QUIET COMPONENTS TestSuite)
+        if(Corrade_TestSuite_FOUND)
+            list(APPEND _MAGNUM_${_COMPONENT}_CORRADE_DEPENDENCIES TestSuite)
+        endif()
     endif()
 
     list(APPEND _MAGNUM_CORRADE_DEPENDENCIES ${_MAGNUM_${_COMPONENT}_CORRADE_DEPENDENCIES})

Let me know if that's enough to fix it.

Nevertheless, for trimming down the Android package size it's I think better to have TestSuite disabled :)

It's odd because I thought that CMAKE_FIND_ROOT_PATH is in default path.

Exactly 🤔 I suppose you're using NDK's Android toolchain and not the builtin CMake Android support, right? In the latter this worked for me, but maybe it only worked because I supplied this path via CMAKE_INSTALL_PREFIX as well. The Android toolchain situation is a bit of a mess 😅

[bansan85]

It's a bit better with this patch. If WITH_TESTSUITE is enabled, a find_package(Corrade REQUIRED TestSuite) is launched (I needed to patch my find_file like I previously said).
But libCorradeTestSuite-d.so is still missing.

I previously had this problem (UnsatisfiedLinkError / a shared library is not copied in the Android package). It may occurs if :

• with an imported library (add_library(... IMPORTED)), property IMPORTED_LOCATION is not set.
• a library doesn't have a SONAME.

But in your case, it's none of this two cases.

[bansan85]

But there is a problem with your patch.

You call find_package(Corrade QUIET COMPONENTS TestSuite) with QUIET. But in FindCorrade.cmake, there is a

find_package_handle_standard_args(Corrade REQUIRED_VARS
    CORRADE_INCLUDE_DIR
    _CORRADE_MODULE_DIR
    _CORRADE_CONFIGURE_FILE
    ${CORRADE_TESTSUITE_XCTEST_RUNNER_NEEDED}
    ${CORRADE_TESTSUITE_ADB_RUNNER_NEEDED}
    ${CORRADE_TESTSUITE_EMSCRIPTEN_RUNNER_NEEDED}
    HANDLE_COMPONENTS)

CORRADE_TESTSUITE_ADB_RUNNER_NEEDED (for Android) will be set to "CORRADE_TESTSUITE_ADB_RUNNER" because find_file has failed. It will generate a fatal error.

[mosra]

Finally merged this as 45e604a, sorry for the extreme delay.

I also managed to figure out and fix the issue with DebugTools and TestSuite:

• as of 4ecb17b, the TestSuite can be found optionally without causing a fatal error if not found
• as of mosra/magnum@400b799, TestSuite is correctly found as a dependency of DebugTools
• and with mosra/magnum@c4c9df0, it's also correctly linked, which should result in libCorradeTestSuite.so being correctly copied in your case

Though I still recommend building without TestSuite, as it's a functionality you don't need in the context of an Android app.

[bansan85]

Thanks for the fix. I no longer need my fork for this project.