Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(23)

Issue 2416073003: Only list direct deps as java libraries

Created:
2 years, 1 month ago by michaelbai
Modified:
1 year, 1 month ago
Reviewers:
agrieve, Torne
CC:
chromium-reviews, mikecase+watch_chromium.org, jbudorick+watch_chromium.org, agrieve+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Only list direct deps as java libraries So, only direct deps are listed in NaitveLibraries.LIBRARIES because it is not necessary to list all .so files in NativeLibrariesLIBRARIES, only direct dep shared libraries need to be loaded explicitly. BUG=649445

Patch Set 1 #

Patch Set 2 : add c++_shared #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+76 lines, -69 lines) Patch
M build/android/gyp/write_build_config.py View 1 2 chunks +25 lines, -10 lines 1 comment Download
M build/config/android/internal_rules.gni View 1 1 chunk +7 lines, -10 lines 1 comment Download
M build/config/android/rules.gni View 1 5 chunks +44 lines, -49 lines 0 comments Download

Messages

Total messages: 39 (7 generated)
michaelbai
2 years, 1 month ago (2016-10-14 20:24:25 UTC) #7
michaelbai
2 years, 1 month ago (2016-10-17 19:51:08 UTC) #9
Torne
Not really familiar enough with how the android gn rules work to review this code, ...
2 years ago (2016-10-24 12:10:30 UTC) #10
michaelbai
On 2016/10/24 12:10:30, Torne wrote: > Not really familiar enough with how the android gn ...
2 years ago (2016-10-24 18:04:54 UTC) #11
agrieve
On 2016/10/24 18:04:54, michaelbai wrote: > On 2016/10/24 12:10:30, Torne wrote: > > Not really ...
2 years ago (2016-10-24 18:07:26 UTC) #12
agrieve
On 2016/10/24 18:07:26, agrieve (OOO oct 17-21) wrote: > On 2016/10/24 18:04:54, michaelbai wrote: > ...
2 years ago (2016-10-25 20:26:33 UTC) #13
agrieve
On 2016/10/25 20:26:33, agrieve (OOO oct 17-21) wrote: > On 2016/10/24 18:07:26, agrieve (OOO oct ...
2 years ago (2016-10-25 20:28:52 UTC) #14
agrieve
On 2016/10/25 20:28:52, agrieve (OOO oct 17-21) wrote: > On 2016/10/25 20:26:33, agrieve (OOO oct ...
2 years ago (2016-10-25 20:58:47 UTC) #15
michaelbai
On 2016/10/25 20:58:47, agrieve (OOO oct 17-21) wrote: > On 2016/10/25 20:28:52, agrieve (OOO oct ...
2 years ago (2016-10-25 21:23:55 UTC) #16
michaelbai
On 2016/10/25 21:23:55, michaelbai wrote: > On 2016/10/25 20:58:47, agrieve (OOO oct 17-21) wrote: > ...
2 years ago (2016-10-25 21:37:19 UTC) #17
michaelbai
Please review patchset #2, Note, there are 2 libraries listed in NativeLibraries.java, the additional one ...
2 years ago (2016-10-25 22:15:43 UTC) #18
Torne
I'm not sure I understand what the issue is here, and I don't see anything ...
2 years ago (2016-10-26 10:33:26 UTC) #19
michaelbai
On 2016/10/26 10:33:26, Torne wrote: > I'm not sure I understand what the issue is ...
2 years ago (2016-10-26 16:19:14 UTC) #20
agrieve
lgtm (assuming you've tested that chrome_public_apk_incremental works). https://codereview.chromium.org/2416073003/diff/20001/build/android/gyp/write_build_config.py File build/android/gyp/write_build_config.py (right): https://codereview.chromium.org/2416073003/diff/20001/build/android/gyp/write_build_config.py#newcode201 build/android/gyp/write_build_config.py:201: """Returns sorted ...
2 years ago (2016-10-26 19:46:18 UTC) #21
Torne
On 2016/10/26 16:19:14, michaelbai wrote: > On 2016/10/26 10:33:26, Torne wrote: > > I'm not ...
2 years ago (2016-10-27 10:11:37 UTC) #22
agrieve
On 2016/10/27 10:11:37, Torne wrote: > On 2016/10/26 16:19:14, michaelbai wrote: > > On 2016/10/26 ...
2 years ago (2016-10-27 14:19:16 UTC) #23
michaelbai
Just tried the incremental build, unfortunately, the patch set #2 didn't work with it. I ...
2 years ago (2016-10-27 19:15:57 UTC) #24
michaelbai
Pathset #2 worked in M, it seem N prevent library being loaded from other place.
2 years ago (2016-10-27 19:35:00 UTC) #25
michaelbai
The failure on N probably relates to system linker's namespace
2 years ago (2016-10-28 00:42:41 UTC) #26
Torne
On 2016/10/28 00:42:41, michaelbai wrote: > The failure on N probably relates to system linker's ...
2 years ago (2016-10-28 16:35:51 UTC) #27
Torne
So am I right that the current status here is: 1) with PS#2 everything (including ...
2 years ago (2016-10-28 16:37:43 UTC) #28
agrieve
On 2016/10/28 16:37:43, Torne wrote: > So am I right that the current status here ...
2 years ago (2016-10-28 16:53:40 UTC) #29
Torne
On 2016/10/28 16:53:40, agrieve wrote: > On 2016/10/28 16:37:43, Torne wrote: > > So am ...
2 years ago (2016-10-28 17:15:54 UTC) #30
Torne
Michael, are you still working on this? It seems like we should try to find ...
1 year, 1 month ago (2017-09-25 18:38:35 UTC) #31
michaelbai
On 2017/09/25 18:38:35, Torne wrote: > Michael, are you still working on this? It seems ...
1 year, 1 month ago (2017-09-25 18:54:50 UTC) #32
Torne
On 2017/09/25 18:54:50, michaelbai wrote: > On 2017/09/25 18:38:35, Torne wrote: > > Michael, are ...
1 year, 1 month ago (2017-09-25 19:15:17 UTC) #33
michaelbai
On 2017/09/25 19:15:17, Torne wrote: > On 2017/09/25 18:54:50, michaelbai wrote: > > On 2017/09/25 ...
1 year, 1 month ago (2017-09-25 19:29:50 UTC) #34
Torne
On 2017/09/25 19:29:50, michaelbai wrote: > On 2017/09/25 19:15:17, Torne wrote: > > On 2017/09/25 ...
1 year, 1 month ago (2017-09-25 19:31:05 UTC) #35
michaelbai
On 2017/09/25 19:31:05, Torne wrote: > On 2017/09/25 19:29:50, michaelbai wrote: > > On 2017/09/25 ...
1 year, 1 month ago (2017-09-25 20:36:23 UTC) #36
Torne
On 2017/09/25 20:36:23, michaelbai wrote: > On 2017/09/25 19:31:05, Torne wrote: > > On 2017/09/25 ...
1 year, 1 month ago (2017-09-25 20:37:27 UTC) #37
michaelbai
On 2017/09/25 20:37:27, Torne wrote: > On 2017/09/25 20:36:23, michaelbai wrote: > > On 2017/09/25 ...
1 year, 1 month ago (2017-09-25 20:41:17 UTC) #38
Torne
1 year, 1 month ago (2017-09-28 15:31:46 UTC) #39
On 2017/09/25 20:41:17, michaelbai wrote:
> On 2017/09/25 20:37:27, Torne wrote:
> > On 2017/09/25 20:36:23, michaelbai wrote:
> > > On 2017/09/25 19:31:05, Torne wrote:
> > > > On 2017/09/25 19:29:50, michaelbai wrote:
> > > > > On 2017/09/25 19:15:17, Torne wrote:
> > > > > > On 2017/09/25 18:54:50, michaelbai wrote:
> > > > > > > On 2017/09/25 18:38:35, Torne wrote:
> > > > > > > > Michael, are you still working on this? It seems like we should
> try
> > to
> > > > > find
> > > > > > a
> > > > > > > > way for this to work.
> > > > > > > 
> > > > > > > No, I am not, any ideal about how to make this work?
> > > > > > 
> > > > > > Re-reading the conversation here it sounds like this change as-is is
> > > > correct,
> > > > > > and the only reason we haven't landed it is because it breaks
> > incremental
> > > > > > installs due to the incremental install code not doing the right
thing
> > on
> > > N
> > > > > (and
> > > > > > only happening to work because of this behaviour of explicitly
loading
> > all
> > > > the
> > > > > > libraries one at a time). Fixing the incremental install stuff on N
to
> > set
> > > > the
> > > > > > paths properly seems like the right thing to do.
> > > > > > 
> > > > > > Is there a bug tracking this work specifically? The referenced bug
is
> > just
> > > > > about
> > > > > > monochrome apk differences, which isn't a great summary of this.
> > Rietveld
> > > is
> > > > > > going read-only on Friday so any important information should be
> > captured
> > > in
> > > > a
> > > > > > bug so we can follow up easily.
> > > > > 
> > > > > I have impression that the failure related to N platform, maybe I am
> > wrong,
> > > it
> > > > > would be great if it can be fixed in incremental install.
> > > > 
> > > > The incremental install code is messing around with internals of the
> > platform
> > > > via reflection to do its trickery. That trick isn't working any more on
N
> -
> > > > that's not the platform's fault that we're messing with non-public APIs
to
> > do
> > > > unsupported things :)
> > > 
> > > Sounds good, you can take this over.
> > 
> > I don't want to take this over particularly. Do we have a bug tracking this
> > particular work?
> 
> We don't have bug for this specifically, I found this issue when I
investigated
> the merge failures :(

Can you file a tracking bug for this and summarise the state from this CL (and
link to this so we can find it again later)?

Powered by Google App Engine
This is Rietveld 408576698