Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(913)

Issue 2416073003: Only list direct deps as java libraries

Created:
4 years, 2 months ago by michaelbai
Modified:
3 years, 2 months 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
4 years, 2 months ago (2016-10-14 20:24:25 UTC) #7
michaelbai
4 years, 2 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, ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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: > ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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: > ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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.
4 years, 1 month ago (2016-10-27 19:35:00 UTC) #25
michaelbai
The failure on N probably relates to system linker's namespace
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 ...
4 years, 1 month 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 years, 2 months 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 years, 2 months 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 years, 2 months 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 years, 2 months 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 years, 2 months 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 years, 2 months 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 years, 2 months 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 years, 2 months ago (2017-09-25 20:41:17 UTC) #38
Torne
3 years, 2 months 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