Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(1846)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 weeks, 2 days ago by anton
Modified:
4 days, 18 hours ago
CC:
René Jeschke, Felix Dahlke
Visibility:
Public.

Description

Issue 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+98 lines, -8 lines) Patch
M README.md View 1 2 3 4 5 1 chunk +26 lines, -0 lines 0 comments Download
M libadblockplus-android/build.gradle View 1 2 3 4 5 1 chunk +16 lines, -0 lines 1 comment Download
M libadblockplus-android/jni/Android.mk View 1 2 3 4 6 chunks +56 lines, -8 lines 0 comments Download

Messages

Total messages: 26
anton
3 weeks, 2 days ago (2017-09-27 09:14:37 UTC) #1
anton
On 2017/09/27 09:14:37, anton wrote: Updated for multiple libraries
3 weeks, 2 days ago (2017-09-27 12:14:04 UTC) #2
sergei
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 libadblockplus-android/jni/Android.mk:82: $(foreach item,$(SHARED_V8_LIB_FILENAME_LIST),$(eval $(call libv8_define,$(item)))) I find it very good ...
3 weeks, 1 day ago (2017-09-28 08:08:41 UTC) #3
anton
On 2017/09/28 08:08:41, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 > ...
3 weeks, 1 day ago (2017-09-28 08:17:37 UTC) #4
sergei
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 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: ...
3 weeks, 1 day ago (2017-09-28 09:15:28 UTC) #5
anton
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 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: ...
3 weeks, 1 day ago (2017-09-28 11:01:38 UTC) #6
sergei
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 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: ...
3 weeks, 1 day ago (2017-09-28 11:35:00 UTC) #7
anton
On 2017/09/28 11:35:00, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode82 > ...
3 weeks, 1 day ago (2017-09-28 11:42:08 UTC) #8
anton
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-android/jni/Android.mk ...
3 weeks ago (2017-09-29 08:28:54 UTC) #9
sergei
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 ...
3 weeks ago (2017-09-29 08:35:41 UTC) #10
anton
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 ...
3 weeks ago (2017-09-29 08:43:51 UTC) #11
sergei
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 ...
3 weeks ago (2017-09-29 09:21:05 UTC) #12
anton
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 ...
3 weeks ago (2017-09-29 11:45:29 UTC) #13
sergei
In general good, just a couple of questions. https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode104 libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES ...
2 weeks, 4 days ago (2017-10-02 09:12:02 UTC) #14
anton
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode104 libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/10/02 09:12:02, sergei wrote: ...
2 weeks, 4 days ago (2017-10-02 09:22:05 UTC) #15
sergei
https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode104 libadblockplus-android/jni/Android.mk:104: LOCAL_STATIC_LIBRARIES := libadblockplus v8-libplatform On 2017/10/02 09:22:04, anton wrote: ...
2 weeks, 4 days ago (2017-10-02 10:08:01 UTC) #16
anton
On 2017/10/02 10:08:01, sergei wrote: > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29557650/libadblockplus-android/jni/Android.mk#newcode104 > ...
2 weeks, 4 days ago (2017-10-02 10:14:36 UTC) #17
sergei
https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk#newcode11 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 ...
2 weeks, 4 days ago (2017-10-02 12:06:52 UTC) #18
diegocarloslima
https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk File libadblockplus-android/jni/Android.mk (right): https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk#newcode11 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 ...
1 week, 3 days ago (2017-10-10 13:40:51 UTC) #19
anton
On 2017/10/10 13:40:51, diegocarloslima wrote: > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk > File libadblockplus-android/jni/Android.mk (right): > > https://codereview.adblockplus.org/29557565/diff/29559613/libadblockplus-android/jni/Android.mk#newcode11 > ...
1 week, 2 days ago (2017-10-11 05:49:02 UTC) #20
sergei
LGTM
1 week, 2 days ago (2017-10-11 12:27:41 UTC) #21
jens
On 2017/09/27 09:14:37, anton wrote: LGTM
1 week, 2 days ago (2017-10-11 12:29:34 UTC) #22
diegocarloslima
On 2017/10/11 12:29:34, jens wrote: > On 2017/09/27 09:14:37, anton wrote: > > LGTM LGTM
1 week, 1 day ago (2017-10-12 09:05:48 UTC) #23
anton
On 2017/10/12 09:05:48, diegocarloslima wrote: > On 2017/10/11 12:29:34, jens wrote: > > On 2017/09/27 ...
1 week, 1 day ago (2017-10-12 12:31:08 UTC) #24
diegocarloslima
https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-android/build.gradle File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29574613/libadblockplus-android/build.gradle#newcode46 libadblockplus-android/build.gradle:46: excludes = sharedV8LibFilesSet I just think that instead of ...
4 days, 18 hours ago (2017-10-16 10:04:28 UTC) #25
anton
4 days, 18 hours ago (2017-10-16 10:18:52 UTC) #26
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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5