Issue 5800 - Add option to specify a list of V8 libraries

libadblockplus-android/jni/Android.mk

Index: libadblockplus-android/jni/Android.mk
diff --git a/libadblockplus-android/jni/Android.mk b/libadblockplus-android/jni/Android.mk
index d152a10bfe00bfc1360a9bef4cbe44cebf132adc..2bef783dab1d6a0334fb476f2f87610972899f62 100755
--- a/libadblockplus-android/jni/Android.mk
+++ b/libadblockplus-android/jni/Android.mk
@@ -1,5 +1,20 @@
 LOCAL_PATH := $(call my-dir)
 
+# Report configuration
+ifeq ($(SHARED_V8_LIB_FILENAME),)
+# static
+$(info [Configuration] Linking statically with built-in v8)
+else
+# dynamic
+
+SHARED_V8_LIB_FILENAME_LIST = $(subst :, ,$(SHARED_V8_LIB_FILENAME))
+define info_define
+    $(info [Configuration] Linking dynamically with shared v8 library ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/$1)
+endef
+$(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call info_define,$(item))))
+endif
+
+# libadblockplus.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := libadblockplus
@@ -7,6 +22,18 @@ LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libadblo
 
 include $(PREBUILT_STATIC_LIBRARY)
 
+# libv8-platform.a
+include $(CLEAR_VARS)
+
+LOCAL_MODULE := v8-libplatform
+LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_libplatform.a
+
+include $(PREBUILT_STATIC_LIBRARY)
+
+ifeq ($(SHARED_V8_LIB_FILENAME),)
+# static
+
+# libv8-libsampler.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := v8-libsampler
@@ -14,6 +41,7 @@ LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_li
 
 include $(PREBUILT_STATIC_LIBRARY)
 
+# libv8-base.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := v8-base
@@ -21,6 +49,7 @@ LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_ba
 
 include $(PREBUILT_STATIC_LIBRARY)
 
+# libv8_libbase.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := v8-libbase
@@ -28,19 +57,31 @@ LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_li
 
 include $(PREBUILT_STATIC_LIBRARY)
 
+# libv8_snapshot.a
 include $(CLEAR_VARS)
 
-LOCAL_MODULE := v8-libplatform
-LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_libplatform.a
+LOCAL_MODULE := v8-snapshot
+LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_snapshot.a
 
 include $(PREBUILT_STATIC_LIBRARY)
 
-include $(CLEAR_VARS)
+else
+# dynamic
 
-LOCAL_MODULE := v8-snapshot
-LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_snapshot.a
+# prebuilt shared libraries v8
 
-include $(PREBUILT_STATIC_LIBRARY)
+SHARED_V8_LIB_FILENAME_LIST = $(subst :, ,$(SHARED_V8_LIB_FILENAME))
+define libv8_define
+    include $(CLEAR_VARS)
+
+    LOCAL_MODULE := $1
+    LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/$1
+
+    include $(PREBUILT_SHARED_LIBRARY)
+endef
+$(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item))))

[sergei, 2017/09/28 08:08:41]

I find it very good that we are looping here through the list of passed
libraries but I have a couple of notes:
- it would be better to rename SHARED_V8_LIB_FILENAME to something carrying that
it's a list of V8 libraries and remove `SHARED_` part because they are not only
shared libraries
- add the condition in the function libv8_define which is defining either a
shared library or a static one depending on the suffix of the library. In
addition to some libv8.so there can be at least libv8_libplatform.a and
libv8_snapshot.a or libv8_nosnapshot.a.
- what about a location of the libraries, it does not look like a good idea to
have them copied into `./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/$1`.
I'm not sure whether it's possible and how difficult it is, so if it does not
require to fight with android makefiles then I would propose to add a
possibility to pass either the library path (-L) or a full/relative path to a v8
library. In order to not delay the progress here, it's totally fine for me to
have something working ASAP and add this feature in another commit.

[sergei, 2017/09/28 09:15:27]

What about replacing LIB_FILENAME by something like LIBRARIES?
The order of libraries can be important, so, I was not sure that we may just
concatenate static and shared libraries. However, anyway we should likely use
LOCAL_SHARED_LIBRARIES and LOCAL_STATIC_LIBRARIES below, in "# dynamic".
It's not about the size, why should one create a symlink or hardlink when it's a
common approach to simply pass a path(s) to libraries. An additional action in
libadblockplus-android repository complicates the building process.
That's right but it would be better to have a working version.

BTW, what about apk, when and what libraries should be included in our apk?

[anton, 2017/09/28 11:01:38]

For Android syntax we pass full (absolute or relative) file paths, not just
filename and libs directories list, see:
LOCAL_SRC_FILES :=
./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_snapshot.a
So i'm not sure we can accept -I and -L list and generate correct `Android.mk`
file
It's up to android build tool (like Gradle) to do it. The goal of native
building is just build libadblockplus-jni.so

[sergei, 2017/09/28 11:35:00]

This is still opened.
OK, let's leave it for present.
Should we also update some gradle.build?

+
+endif
 
 include $(CLEAR_VARS)
 
@@ -55,10 +96,20 @@ LOCAL_SRC_FILES += JniFilterChangeCallback.cpp JniCallbacks.cpp Utils.cpp
 LOCAL_SRC_FILES += JniNotification.cpp JniShowNotificationCallback.cpp
 LOCAL_SRC_FILES += JniIsAllowedConnectionTypeCallback.cpp
 
-LOCAL_CPP_FEATURES := exceptions 
+LOCAL_CPP_FEATURES := exceptions
 LOCAL_CPPFLAGS += -std=c++11
 
 LOCAL_C_INCLUDES := jni/libadblockplus-binaries/include/
-LOCAL_STATIC_LIBRARIES := libadblockplus v8-base v8-snapshot v8-libsampler v8-libplatform v8-libbase
+
+LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform

[sergei, 2017/09/28 09:15:27]

I'm not sure that "v8-libplatform" should be always here, I would rather ask to
pass it in the list of external libraries if the latter exists, even if it's our
library. I think that if there is another platform library then they will
conflict because of redefinition of the same functions.

[anton, 2017/09/28 11:01:38]

I tend to agree with it.

[sergei, 2017/10/02 09:12:02]

So, what is the decision regarding v8-libplatform?

[anton, 2017/10/02 09:22:04]

i'd say for now we should leave it as in review (required in both static and
dynamic linking).
It was required for integrating to Aurora so i believe it's reasonable solution.

Later we will add new arguments or change the way we treat passed libs list and
decide then.

[sergei, 2017/10/02 10:08:01]

Acknowledged.

+
+ifeq ($(SHARED_V8_LIB_FILENAME),)
+# static
+LOCAL_STATIC_LIBRARIES += v8-base v8-snapshot v8-libsampler v8-libbase
+else
+# dynamic
+SHARED_V8_LIB_FILENAME_LIST = $(subst :, ,$(SHARED_V8_LIB_FILENAME))
+LOCAL_STATIC_LIBRARIES += $(SHARED_V8_LIB_FILENAME_LIST)
+endif
 
 include $(BUILD_SHARED_LIBRARY)