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

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
2 months, 2 weeks ago by anton
Modified:
1 month 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

Patch Set 7 : moved 'packagingOptions' section to dynamic linking 'if' branch only #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+99 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 6 1 chunk +17 lines, -0 lines 2 comments Download
M libadblockplus-android/jni/Android.mk View 1 2 3 4 6 chunks +56 lines, -8 lines 0 comments Download

Messages

Total messages: 33
anton
2 months, 2 weeks ago (2017-09-27 09:14:37 UTC) #1
anton
On 2017/09/27 09:14:37, anton wrote: Updated for multiple libraries
2 months, 2 weeks 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 ...
2 months, 2 weeks 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 > ...
2 months, 2 weeks 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: ...
2 months, 2 weeks 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: ...
2 months, 2 weeks 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: ...
2 months, 2 weeks 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 > ...
2 months, 2 weeks 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 ...
2 months, 2 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 ...
2 months, 2 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 ...
2 months, 2 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 ...
2 months, 2 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 ...
2 months, 2 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 months, 1 week 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 ...
2 months 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 > ...
2 months ago (2017-10-11 05:49:02 UTC) #20
sergei
LGTM
2 months ago (2017-10-11 12:27:41 UTC) #21
jens
On 2017/09/27 09:14:37, anton wrote: LGTM
2 months 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
2 months 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 ...
2 months 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 ...
1 month, 4 weeks ago (2017-10-16 10:04:28 UTC) #25
anton
On 2017/10/16 10:04:28, diegocarloslima wrote: > 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 > ...
1 month, 4 weeks ago (2017-10-16 10:18:52 UTC) #26
jens
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-android/build.gradle ...
1 month, 1 week ago (2017-11-02 11:13:52 UTC) #27
anton
On 2017/11/02 11:13:52, jens wrote: > On 2017/10/16 10:18:52, anton wrote: > > On 2017/10/16 ...
1 month, 1 week ago (2017-11-02 11:31:31 UTC) #28
diegocarloslima
https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle#newcode43 libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet I think we weren't in the ...
1 month ago (2017-11-08 13:25:55 UTC) #29
anton
https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle File libadblockplus-android/build.gradle (right): https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle#newcode43 libadblockplus-android/build.gradle:43: excludes = sharedV8LibFilesSet On 2017/11/08 13:25:55, diegocarloslima wrote: > ...
1 month ago (2017-11-08 13:28:26 UTC) #30
diegocarloslima
On 2017/11/08 13:28:26, anton wrote: > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle > File libadblockplus-android/build.gradle (right): > > https://codereview.adblockplus.org/29557565/diff/29595584/libadblockplus-android/build.gradle#newcode43 > ...
1 month ago (2017-11-08 13:57:15 UTC) #31
jens
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-android/build.gradle ...
1 month ago (2017-11-08 14:04:26 UTC) #32
anton
1 month ago (2017-11-09 05:45:39 UTC) #33
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.
Sign in to reply to this message.

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