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

libadblockplus-android/jni/Android.mk

 LOCAL_PATH := $(call my-dir)
 
 # Report configuration
-ifeq ($(SHARED_V8_LIB_FILENAME),)
+ifeq ($(SHARED_V8_LIB_FILENAMES),)
 # 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))))
+$(foreach item,$(SHARED_V8_LIB_FILENAMES),$(eval $(call info_define,$(item))))
 endif
 
 # libadblockplus.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := libadblockplus
 LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libadblockplus.a
 
 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),)
+ifeq ($(SHARED_V8_LIB_FILENAMES),)
 # static
 
 # libv8-libsampler.a
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := v8-libsampler
 LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_libsampler.a
 
 include $(PREBUILT_STATIC_LIBRARY)
 
@@ skipping 19 matching lines @@
 LOCAL_MODULE := v8-snapshot
 LOCAL_SRC_FILES := ./libadblockplus-binaries/android_$(TARGET_ARCH_ABI)/libv8_snapshot.a
 
 include $(PREBUILT_STATIC_LIBRARY)
 
 else
 # dynamic
 
 # prebuilt shared libraries v8
 
-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?

+$(foreach item,$(SHARED_V8_LIB_FILENAMES),$(eval $(call libv8_define,$(item))))
 
 endif
 
 include $(CLEAR_VARS)
 
 LOCAL_MODULE := libadblockplus-jni
 LOCAL_SRC_FILES := JniLibrary.cpp
 LOCAL_SRC_FILES += JniPlatform.cpp
 LOCAL_SRC_FILES += JniJsEngine.cpp JniFilterEngine.cpp JniJsValue.cpp
 LOCAL_SRC_FILES += JniFilter.cpp JniSubscription.cpp JniEventCallback.cpp
 LOCAL_SRC_FILES += JniLogSystem.cpp JniWebRequest.cpp
 LOCAL_SRC_FILES += JniUpdateAvailableCallback.cpp JniUpdateCheckDoneCallback.cpp
 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_CPPFLAGS += -std=c++11
 
 LOCAL_C_INCLUDES := jni/libadblockplus-binaries/include/
 
 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),)
+ifeq ($(SHARED_V8_LIB_FILENAMES),)
 # 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)
+LOCAL_STATIC_LIBRARIES += $(SHARED_V8_LIB_FILENAMES)
 endif
 
 include $(BUILD_SHARED_LIBRARY)