Issue 29557565: Issue 5800 - Add option to specify a list of V8 libraries (Closed)
|
Created:
Sept. 27, 2017, 9:10 a.m. by anton Modified:
Nov. 9, 2017, 7:30 a.m. CC:
René Jeschke, Felix Dahlke Visibility:
Public. |
DescriptionIssue 5800 - Add option to specify a list of V8 libraries
Patch Set 1 #Patch Set 2 : Supporting multiple libraries #
Total comments: 9
Patch Set 3 : Renamed env. variable #
Total comments: 12
Patch Set 4 : Using comma as separator, README changes #
Total comments: 5
Patch Set 5 : Using space as separator, README changes #Patch Set 6 : Excluding v8 shared libs from AAR (Gradle) #
Total comments: 1
Patch Set 7 : moved 'packagingOptions' section to dynamic linking 'if' branch only #
Total comments: 2
MessagesTotal messages: 33
On 2017/09/27 09:14:37, anton wrote: Updated for multiple libraries
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:82: $(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) 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.
On 2017/09/28 08:08:41, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > libadblockplus-android/jni/Android.mk:82: $(foreach > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > 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 Actually i've meant only "shared" libraries as for them i have to use `include $(PREBUILT_SHARED_LIBRARY)`. For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. We can add static libs list support just like i did for shared. > - 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. We could do this way or the way i've described above (separate argument and new "define" section). > - 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`. Why not? We put .a there, what's the difference? One can create symlink instead of actual copying to save the storage. > 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. I see no problem here: we can check if env variable is passed or just adjust local_flags: `LOCAL_LDFLAGS += -L/path` (docs https://developer.android.com/ndk/guides/android_mk.html) 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.
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:82: $(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) On 2017/09/28 08:17:37, anton wrote: > On 2017/09/28 08:08:41, sergei wrote: > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > File libadblockplus-android/jni/Android.mk (right): > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > libadblockplus-android/jni/Android.mk:82: $(foreach > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > > 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 What about replacing LIB_FILENAME by something like LIBRARIES? > > Actually i've meant only "shared" libraries as for them i have to use `include > $(PREBUILT_SHARED_LIBRARY)`. > For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. > We can add static libs list support just like i did for shared. > > > - 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. > > We could do this way or the way i've described above (separate argument and new > "define" section). 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". > > > - 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`. > > Why not? We put .a there, what's the difference? > One can create symlink instead of actual copying to save the storage. 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. > > > 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. > > I see no problem here: we can check if env variable is passed or just adjust > local_flags: > `LOCAL_LDFLAGS += -L/path` > (docs https://developer.android.com/ndk/guides/android_mk.html) > > 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. 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? https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform 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.
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:82: $(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) On 2017/09/28 09:15:27, sergei wrote: > On 2017/09/28 08:17:37, anton wrote: > > On 2017/09/28 08:08:41, sergei wrote: > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > File libadblockplus-android/jni/Android.mk (right): > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > libadblockplus-android/jni/Android.mk:82: $(foreach > > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > > > 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 > What about replacing LIB_FILENAME by something like LIBRARIES? > > > > Actually i've meant only "shared" libraries as for them i have to use `include > > $(PREBUILT_SHARED_LIBRARY)`. > > For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. > > We can add static libs list support just like i did for shared. > > > > > - 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. > > > > We could do this way or the way i've described above (separate argument and > new > > "define" section). > 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". > > > > > - 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`. > > > > Why not? We put .a there, what's the difference? > > One can create symlink instead of actual copying to save the storage. > 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. 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 > libadblockplus-android repository complicates the building process. > > > > > 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. > > > > I see no problem here: we can check if env variable is passed or just adjust > > local_flags: > > `LOCAL_LDFLAGS += -L/path` > > (docs https://developer.android.com/ndk/guides/android_mk.html) > > > > 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. > 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? It's up to android build tool (like Gradle) to do it. The goal of native building is just build libadblockplus-jni.so https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/09/28 09:15:27, sergei wrote: > 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. I tend to agree with it.
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:82: $(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) On 2017/09/28 11:01:38, anton wrote: > On 2017/09/28 09:15:27, sergei wrote: > > On 2017/09/28 08:17:37, anton wrote: > > > On 2017/09/28 08:08:41, sergei wrote: > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > File libadblockplus-android/jni/Android.mk (right): > > > > > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > libadblockplus-android/jni/Android.mk:82: $(foreach > > > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > > > > 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 > > What about replacing LIB_FILENAME by something like LIBRARIES? This is still opened. > > > > > > Actually i've meant only "shared" libraries as for them i have to use > `include > > > $(PREBUILT_SHARED_LIBRARY)`. > > > For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. > > > We can add static libs list support just like i did for shared. > > > > > > > - 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. > > > > > > We could do this way or the way i've described above (separate argument and > > new > > > "define" section). > > 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". > > > > > > > - 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`. > > > > > > Why not? We put .a there, what's the difference? > > > One can create symlink instead of actual copying to save the storage. > > 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. > > 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 OK, let's leave it for present. > > > libadblockplus-android repository complicates the building process. > > > > > > > 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. > > > > > > I see no problem here: we can check if env variable is passed or just adjust > > > local_flags: > > > `LOCAL_LDFLAGS += -L/path` > > > (docs https://developer.android.com/ndk/guides/android_mk.html) > > > > > > 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. > > 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? > It's up to android build tool (like Gradle) to do it. The goal of native > building is just build libadblockplus-jni.so > Should we also update some gradle.build?
On 2017/09/28 11:35:00, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > libadblockplus-android/jni/Android.mk:82: $(foreach > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > On 2017/09/28 11:01:38, anton wrote: > > On 2017/09/28 09:15:27, sergei wrote: > > > On 2017/09/28 08:17:37, anton wrote: > > > > On 2017/09/28 08:08:41, sergei wrote: > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > > File libadblockplus-android/jni/Android.mk (right): > > > > > > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > > libadblockplus-android/jni/Android.mk:82: $(foreach > > > > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call > libv8_define,$(item)))) > > > > > 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 > > > What about replacing LIB_FILENAME by something like LIBRARIES? > This is still opened. > > > > > > > > Actually i've meant only "shared" libraries as for them i have to use > > `include > > > > $(PREBUILT_SHARED_LIBRARY)`. > > > > For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. > > > > We can add static libs list support just like i did for shared. > > > > > > > > > - 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. > > > > > > > > We could do this way or the way i've described above (separate argument > and > > > new > > > > "define" section). > > > 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". > > > > > > > > > - 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`. > > > > > > > > Why not? We put .a there, what's the difference? > > > > One can create symlink instead of actual copying to save the storage. > > > 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. > > > > 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 > OK, let's leave it for present. > > > > > libadblockplus-android repository complicates the building process. > > > > > > > > > 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. > > > > > > > > I see no problem here: we can check if env variable is passed or just > adjust > > > > local_flags: > > > > `LOCAL_LDFLAGS += -L/path` > > > > (docs https://developer.android.com/ndk/guides/android_mk.html) > > > > > > > > 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. > > > 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? > > It's up to android build tool (like Gradle) to do it. The goal of native > > building is just build libadblockplus-jni.so > > > Should we also update some gradle.build? No, just set env variables before build and they will affect how native libs are built. Nothing is changed in android part, the library is loaded as previously. The only thing worth mentioning is that is it's liked with shared libraries they should be loaded before libadblockplus-jni.so similar to https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp...
On 2017/09/28 11:42:08, anton wrote: > On 2017/09/28 11:35:00, sergei wrote: > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > File libadblockplus-android/jni/Android.mk (right): > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > libadblockplus-android/jni/Android.mk:82: $(foreach > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) > > On 2017/09/28 11:01:38, anton wrote: > > > On 2017/09/28 09:15:27, sergei wrote: > > > > On 2017/09/28 08:17:37, anton wrote: > > > > > On 2017/09/28 08:08:41, sergei wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > > > File libadblockplus-android/jni/Android.mk (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > > > > > > libadblockplus-android/jni/Android.mk:82: $(foreach > > > > > > item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call > > libv8_define,$(item)))) > > > > > > 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 > > > > What about replacing LIB_FILENAME by something like LIBRARIES? > > This is still opened. > > > > > > > > > > Actually i've meant only "shared" libraries as for them i have to use > > > `include > > > > > $(PREBUILT_SHARED_LIBRARY)`. > > > > > For static libraries i have to use `include $(PREBUILT_STATIC_LIBRARY)`. > > > > > We can add static libs list support just like i did for shared. > > > > > > > > > > > - 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. > > > > > > > > > > We could do this way or the way i've described above (separate argument > > and > > > > new > > > > > "define" section). > > > > 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". > > > > > > > > > > > - 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`. > > > > > > > > > > Why not? We put .a there, what's the difference? > > > > > One can create symlink instead of actual copying to save the storage. > > > > 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. > > > > > > 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 > > OK, let's leave it for present. > > > > > > > libadblockplus-android repository complicates the building process. > > > > > > > > > > > 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. > > > > > > > > > > I see no problem here: we can check if env variable is passed or just > > adjust > > > > > local_flags: > > > > > `LOCAL_LDFLAGS += -L/path` > > > > > (docs https://developer.android.com/ndk/guides/android_mk.html) > > > > > > > > > > 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. > > > > 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? > > > It's up to android build tool (like Gradle) to do it. The goal of native > > > building is just build libadblockplus-jni.so > > > > > Should we also update some gradle.build? > > No, just set env variables before build and they will affect how native libs are > built. > Nothing is changed in android part, the library is loaded as previously. > The only thing worth mentioning is that is it's liked with shared libraries they > should be loaded before libadblockplus-jni.so similar to > https://github.com/adblockplus/libadblockplus-android/blob/master/libadblockp... Uploaded new patch set
https://codereview.adblockplus.org/29557565/diff/29559555/README.md File README.md (right): https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode96 README.md:96: This can be desired to use product's V8 (let's say Chromium) instead of libadblockplus built-in V8. I would replace "libadblockplus built-in V8" by "V8 shipped with libadblockplus-binaries". https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode98 README.md:98: environment variable before building. You can pass multiple filenames, separated with `:`. Why not separate them by spaces instead of `:`? https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode99 README.md:99: Libadblockplus is required to be linked with that library file(s). actually not "Libadblockplus" but libadblockplus-android.so
https://codereview.adblockplus.org/29557565/diff/29559555/README.md File README.md (right): https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode96 README.md:96: This can be desired to use product's V8 (let's say Chromium) instead of libadblockplus built-in V8. On 2017/09/29 08:35:41, sergei wrote: > I would replace "libadblockplus built-in V8" by "V8 shipped with > libadblockplus-binaries". The user can be not aware of internal structure of our code (core>libadblockplus>libadblockplus-android) so here "libadblockplus" is just "a library that provides an opportunity to use Adblock Plus in 3rd party product". I would leave it as-is. https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode98 README.md:98: environment variable before building. You can pass multiple filenames, separated with `:`. On 2017/09/29 08:35:41, sergei wrote: > Why not separate them by spaces instead of `:`? Does it make sense what separator? ":" standard separator in PATHs and etc. if it's not good for some reason then i'd prefer ",". https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode99 README.md:99: Libadblockplus is required to be linked with that library file(s). On 2017/09/29 08:35:41, sergei wrote: > actually not "Libadblockplus" but libadblockplus-android.so I've meant libadblockplus actually. To be more detailed i've meant "libadblockplus-binaries linked with same dynamic libs".
https://codereview.adblockplus.org/29557565/diff/29559555/README.md File README.md (right): https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode96 README.md:96: This can be desired to use product's V8 (let's say Chromium) instead of libadblockplus built-in V8. On 2017/09/29 08:43:51, anton wrote: > On 2017/09/29 08:35:41, sergei wrote: > > I would replace "libadblockplus built-in V8" by "V8 shipped with > > libadblockplus-binaries". > > The user can be not aware of internal structure of our code > (core>libadblockplus>libadblockplus-android) so here "libadblockplus" is just "a > library that provides an opportunity to use Adblock Plus in 3rd party product". > I would leave it as-is. OK, but V8 is not build-in, libadblockplus.a is not linked with any V8 yet, maybe just remove the word "libadblockplus"? https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode98 README.md:98: environment variable before building. You can pass multiple filenames, separated with `:`. On 2017/09/29 08:43:51, anton wrote: > On 2017/09/29 08:35:41, sergei wrote: > > Why not separate them by spaces instead of `:`? > > Does it make sense what separator? ":" standard separator in PATHs and etc. if > it's not good for some reason then i'd prefer ",". On windows the standard path separator is ; because paths contain :, e.g. c:\projects\. We pass the libraries to the compiler separating them by spaces and using quotes if there a space in the path or in a file name. Therefore I think we should use spaces just don't support names with spaces which is likely a very uncommon case. https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode99 README.md:99: Libadblockplus is required to be linked with that library file(s). On 2017/09/29 08:43:51, anton wrote: > On 2017/09/29 08:35:41, sergei wrote: > > actually not "Libadblockplus" but libadblockplus-android.so > > I've meant libadblockplus actually. To be more detailed i've meant > "libadblockplus-binaries linked with same dynamic libs". Acknowledged. Maybe we should say that this library requires linking with V8?
Uploaded patch set 4 https://codereview.adblockplus.org/29557565/diff/29559555/README.md File README.md (right): https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode96 README.md:96: This can be desired to use product's V8 (let's say Chromium) instead of libadblockplus built-in V8. On 2017/09/29 09:21:05, sergei wrote: > On 2017/09/29 08:43:51, anton wrote: > > On 2017/09/29 08:35:41, sergei wrote: > > > I would replace "libadblockplus built-in V8" by "V8 shipped with > > > libadblockplus-binaries". > > > > The user can be not aware of internal structure of our code > > (core>libadblockplus>libadblockplus-android) so here "libadblockplus" is just > "a > > library that provides an opportunity to use Adblock Plus in 3rd party > product". > > I would leave it as-is. > > OK, but V8 is not build-in, libadblockplus.a is not linked with any V8 yet, > maybe just remove the word "libadblockplus"? Acknowledged. https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode98 README.md:98: environment variable before building. You can pass multiple filenames, separated with `:`. On 2017/09/29 09:21:04, sergei wrote: > On 2017/09/29 08:43:51, anton wrote: > > On 2017/09/29 08:35:41, sergei wrote: > > > Why not separate them by spaces instead of `:`? > > > > Does it make sense what separator? ":" standard separator in PATHs and etc. > if > > it's not good for some reason then i'd prefer ",". > > On windows the standard path separator is ; because paths contain :, e.g. > c:\projects\. We pass the libraries to the compiler separating them by spaces > and using quotes if there a space in the path or in a file name. Therefore I > think we should use spaces just don't support names with spaces which is likely > a very uncommon case. Let's have ',' as working for all solution. https://codereview.adblockplus.org/29557565/diff/29559555/README.md#newcode99 README.md:99: Libadblockplus is required to be linked with that library file(s). On 2017/09/29 09:21:04, sergei wrote: > On 2017/09/29 08:43:51, anton wrote: > > On 2017/09/29 08:35:41, sergei wrote: > > > actually not "Libadblockplus" but libadblockplus-android.so > > > > I've meant libadblockplus actually. To be more detailed i've meant > > "libadblockplus-binaries linked with same dynamic libs". > > Acknowledged. Maybe we should say that this library requires linking with V8? I'm not sure it's required for most users. And it seems to be obvious for advanced users (who want's to link with their product like Chromium).
In general good, just a couple of questions. https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/09/28 11:01:38, anton wrote: > On 2017/09/28 09:15:27, sergei wrote: > > 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. > > I tend to agree with it. So, what is the decision regarding v8-libplatform? https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... libadblockplus-android/jni/Android.mk:10: comma = , should it be called something like shared_v8_lib_separator?
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/10/02 09:12:02, sergei wrote: > On 2017/09/28 11:01:38, anton wrote: > > On 2017/09/28 09:15:27, sergei wrote: > > > 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. > > > > I tend to agree with it. > > So, what is the decision regarding v8-libplatform? 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. https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... libadblockplus-android/jni/Android.mk:10: comma = , On 2017/10/02 09:12:02, sergei wrote: > should it be called something like shared_v8_lib_separator? Does not matter for me. Should i rename?
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/10/02 09:22:04, anton wrote: > On 2017/10/02 09:12:02, sergei wrote: > > On 2017/09/28 11:01:38, anton wrote: > > > On 2017/09/28 09:15:27, sergei wrote: > > > > 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. > > > > > > I tend to agree with it. > > > > So, what is the decision regarding v8-libplatform? > > 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. Acknowledged. https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = $(subst $(comma), ,$(SHARED_V8_LIB_FILENAMES)) Sorry, didn't pay attention to it before. What was the problem with spaces in comparison with comma, I just think that spaces are not only a common way but also can simplify the method by removing calls of subst.
On 2017/10/02 10:08:01, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-andr... > libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := > libadblockplus v8-libplatform > On 2017/10/02 09:22:04, anton wrote: > > On 2017/10/02 09:12:02, sergei wrote: > > > On 2017/09/28 11:01:38, anton wrote: > > > > On 2017/09/28 09:15:27, sergei wrote: > > > > > 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. > > > > > > > > I tend to agree with it. > > > > > > So, what is the decision regarding v8-libplatform? > > > > 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. > > Acknowledged. > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = $(subst > $(comma), ,$(SHARED_V8_LIB_FILENAMES)) > Sorry, didn't pay attention to it before. What was the problem with spaces in > comparison with comma, I just think that spaces are not only a common way but > also can simplify the method by removing calls of subst. Comma is more human friendly. I don't think it worth using space just because we will benefit from avoid 1 line execution.
https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = $(subst $(comma), ,$(SHARED_V8_LIB_FILENAMES)) On 2017/10/02 10:08:01, sergei wrote: > Sorry, didn't pay attention to it before. What was the problem with spaces in > comparison with comma, I just think that spaces are not only a common way but > also can simplify the method by removing calls of subst. > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > > libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = > $(subst > > $(comma), ,$(SHARED_V8_LIB_FILENAMES)) > > Sorry, didn't pay attention to it before. What was the problem with spaces in > > comparison with comma, I just think that spaces are not only a common way but > > also can simplify the method by removing calls of subst. > > Comma is more human friendly. > I don't think it worth using space just because we will benefit from avoid 1 > line execution. It's three times here and there is even no need in SHARED_V8_LIB_FILENAMES_LIST. I think it's better to wait for a third opinion.
https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = $(subst $(comma), ,$(SHARED_V8_LIB_FILENAMES)) On 2017/10/02 12:06:52, sergei wrote: > On 2017/10/02 10:08:01, sergei wrote: > > Sorry, didn't pay attention to it before. What was the problem with spaces in > > comparison with comma, I just think that spaces are not only a common way but > > also can simplify the method by removing calls of subst. > > > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > > > libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = > > $(subst > > > $(comma), ,$(SHARED_V8_LIB_FILENAMES)) > > > Sorry, didn't pay attention to it before. What was the problem with spaces > in > > > comparison with comma, I just think that spaces are not only a common way > but > > > also can simplify the method by removing calls of subst. > > > > Comma is more human friendly. > > I don't think it worth using space just because we will benefit from avoid 1 > > line execution. > It's three times here and there is even no need in SHARED_V8_LIB_FILENAMES_LIST. > I think it's better to wait for a third opinion. Yeah, I agree that for sake of simplicity, just a space would be better.
On 2017/10/10 13:40:51, diegocarloslima wrote: > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = $(subst > $(comma), ,$(SHARED_V8_LIB_FILENAMES)) > On 2017/10/02 12:06:52, sergei wrote: > > On 2017/10/02 10:08:01, sergei wrote: > > > Sorry, didn't pay attention to it before. What was the problem with spaces > in > > > comparison with comma, I just think that spaces are not only a common way > but > > > also can simplify the method by removing calls of subst. > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-andr... > > > > libadblockplus-android/jni/Android.mk:11: SHARED_V8_LIB_FILENAMES_LIST = > > > $(subst > > > > $(comma), ,$(SHARED_V8_LIB_FILENAMES)) > > > > Sorry, didn't pay attention to it before. What was the problem with spaces > > in > > > > comparison with comma, I just think that spaces are not only a common way > > but > > > > also can simplify the method by removing calls of subst. > > > > > > Comma is more human friendly. > > > I don't think it worth using space just because we will benefit from avoid 1 > > > line execution. > > It's three times here and there is even no need in > SHARED_V8_LIB_FILENAMES_LIST. > > I think it's better to wait for a third opinion. > > Yeah, I agree that for sake of simplicity, just a space would be better. Uploaded new patch set
LGTM
On 2017/09/27 09:14:37, anton wrote: LGTM
On 2017/10/11 12:29:34, jens wrote: > On 2017/09/27 09:14:37, anton wrote: > > LGTM LGTM
On 2017/10/12 09:05:48, diegocarloslima wrote: > On 2017/10/11 12:29:34, jens wrote: > > On 2017/09/27 09:14:37, anton wrote: > > > > LGTM > > LGTM Sorry for late patch set #6 (after it's LGTMed), i had to hardcode it and now managed to do it dynamically. We have to exclude shared v8 libs from produced AAR as by default they are packed too. It leads to apk build conflicts as shared v8 files are come from both Chromium and libadblockplus AAR. So now the files are excluded and no conflict happens.
https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... libadblockplus-android/build.gradle:46: excludes = sharedV8LibFilesSet I just think that instead of using the sharedV8LibFilesSet var here, we should make it a call to a method (e.g. getSharedV8LibFilesSet), so we won't tie the packagingOptions closure with the order of execution of the sharedV8LibFilesSet block
On 2017/10/16 10:04:28, diegocarloslima wrote: > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > File libadblockplus-android/build.gradle (right): > > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > libadblockplus-android/build.gradle:46: excludes = sharedV8LibFilesSet > I just think that instead of using the sharedV8LibFilesSet var here, we should > make it a call to a method (e.g. getSharedV8LibFilesSet), so we won't tie the > packagingOptions closure with the order of execution of the sharedV8LibFilesSet > block I've been thinking about it. I decided that `packagingOptions` could be adjusted for both static/dynamic linking (let's say `doNotStrip` option) so i preferred single section for both with empty set for static - for now it's used for dynamic linking only indeed. I can change if you insist or Jens agree with you.
On 2017/10/16 10:18:52, anton wrote: > On 2017/10/16 10:04:28, diegocarloslima wrote: > > > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > > File libadblockplus-android/build.gradle (right): > > > > > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > > libadblockplus-android/build.gradle:46: excludes = sharedV8LibFilesSet > > I just think that instead of using the sharedV8LibFilesSet var here, we should > > make it a call to a method (e.g. getSharedV8LibFilesSet), so we won't tie the > > packagingOptions closure with the order of execution of the > sharedV8LibFilesSet > > block > > I've been thinking about it. I decided that `packagingOptions` could be adjusted > for both static/dynamic linking (let's say `doNotStrip` option) so > i preferred single section for both with empty set for static - for now it's > used for dynamic linking only indeed. > > I can change if you insist or Jens agree with you. +1 for diegos suggestion. But I don't insist to change that right now.
On 2017/11/02 11:13:52, jens wrote: > On 2017/10/16 10:18:52, anton wrote: > > On 2017/10/16 10:04:28, diegocarloslima wrote: > > > > > > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > > > File libadblockplus-android/build.gradle (right): > > > > > > > > > https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-andr... > > > libadblockplus-android/build.gradle:46: excludes = sharedV8LibFilesSet > > > I just think that instead of using the sharedV8LibFilesSet var here, we > should > > > make it a call to a method (e.g. getSharedV8LibFilesSet), so we won't tie > the > > > packagingOptions closure with the order of execution of the > > sharedV8LibFilesSet > > > block > > > > I've been thinking about it. I decided that `packagingOptions` could be > adjusted > > for both static/dynamic linking (let's say `doNotStrip` option) so > > i preferred single section for both with empty set for static - for now it's > > used for dynamic linking only indeed. > > > > I can change if you insist or Jens agree with you. > > +1 for diegos suggestion. But I don't insist to change that right now. Uploaded new patch set
https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet I think we weren't in the same page in my last comment. What I actually meant is that here we could have something like: excludes = getSharedV8LibFileSet(), which would be a call for the method `def getSharedV8LibFileSet() {...}` that would return the array sharedV8LibFilesSet (either with elements or empty)
https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet On 2017/11/08 13:25:55, diegocarloslima wrote: > I think we weren't in the same page in my last comment. What I actually meant is > that here we could have something like: excludes = getSharedV8LibFileSet(), > which would be a call for the method `def getSharedV8LibFileSet() {...}` that > would return the array sharedV8LibFilesSet (either with elements or empty) It's equal to what we had before last patch set. I don't see any reason to introduce methods as it's used once.
On 2017/11/08 13:28:26, anton wrote: > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > File libadblockplus-android/build.gradle (right): > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet > On 2017/11/08 13:25:55, diegocarloslima wrote: > > I think we weren't in the same page in my last comment. What I actually meant > is > > that here we could have something like: excludes = getSharedV8LibFileSet(), > > which would be a call for the method `def getSharedV8LibFileSet() {...}` that > > would return the array sharedV8LibFilesSet (either with elements or empty) > > It's equal to what we had before last patch set. I don't see any reason to > introduce methods as it's used once. We discussed a bit on IRC and we ended up agreeing to keep like this for now. One point was that it wouldn't make much sense to have the printing stuff in the self contained method. If we ever feel the need to change it, we can do it later. Thus, LGTM
On 2017/11/08 13:57:15, diegocarloslima wrote: > On 2017/11/08 13:28:26, anton wrote: > > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > > File libadblockplus-android/build.gradle (right): > > > > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > > libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet > > On 2017/11/08 13:25:55, diegocarloslima wrote: > > > I think we weren't in the same page in my last comment. What I actually > meant > > is > > > that here we could have something like: excludes = getSharedV8LibFileSet(), > > > which would be a call for the method `def getSharedV8LibFileSet() {...}` > that > > > would return the array sharedV8LibFilesSet (either with elements or empty) > > > > It's equal to what we had before last patch set. I don't see any reason to > > introduce methods as it's used once. > > We discussed a bit on IRC and we ended up agreeing to keep like this for now. > One point was that it wouldn't make much sense to have the printing stuff in the > self contained method. If we ever feel the need to change it, we can do it > later. Thus, LGTM LGTM
On 2017/11/08 13:57:15, diegocarloslima wrote: > On 2017/11/08 13:28:26, anton wrote: > > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > > File libadblockplus-android/build.gradle (right): > > > > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-andr... > > libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet > > On 2017/11/08 13:25:55, diegocarloslima wrote: > > > I think we weren't in the same page in my last comment. What I actually > meant > > is > > > that here we could have something like: excludes = getSharedV8LibFileSet(), > > > which would be a call for the method `def getSharedV8LibFileSet() {...}` > that > > > would return the array sharedV8LibFilesSet (either with elements or empty) > > > > It's equal to what we had before last patch set. I don't see any reason to > > introduce methods as it's used once. > > We discussed a bit on IRC and we ended up agreeing to keep like this for now. > One point was that it wouldn't make much sense to have the printing stuff in the > self contained method. If we ever feel the need to change it, we can do it > later. Thus, LGTM Yes, after quick discussing with Diego i tend to agree that having new function like `get...` is more flexible. The question was do we need that flexibility. But at that case we will be unable to output mode (dynamic/static) as the function is not smart enough and outputting of the mode in it does not mean actual applying of the mode. We could probably introduce `enum Mode { Dynamic, Static }`, create new function `detectMode` (which just detects if env var is set), use it in both `get...()` and right before adding of `packagingOptions`. But it makes the code too complex for such a simple thing. So we decided to leave it as it and change when (and if) we really need it. 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
