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

Issue 2416073003: Only list direct deps as java libraries

Can't Edit
Can't Publish+Mail
Start Review
Created:
1 year ago by michaelbai
Modified:
2 weeks, 4 days 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
Trybot results: Sign in to try more bots
Commit queue not available (can’t edit this change).

Messages

Total messages: 39 (7 generated)
michaelbai
1 year ago (2016-10-14 20:24:25 UTC) #7
michaelbai
12 months 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, ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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: > ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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: > ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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.
11 months, 3 weeks ago (2016-10-27 19:35:00 UTC) #25
michaelbai
The failure on N probably relates to system linker's namespace
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
11 months, 3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks 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 ...
3 weeks ago (2017-09-25 20:41:17 UTC) #38
Torne
2 weeks, 4 days 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)?
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 81bcdb8aa