Android: Make stack tool work for GN builds

Android: Make stack tool work for GN builds

This actually required a couple of changes to the script:
1. Add lib.unstripped to search paths.
2. Respect CHROMIUM_OUTPUT_DIR environment variable.
3. Don't sort by mtime when looking for libraries (resulted in stripped
   version always being selected).

This also stops the script from looking in default symbol locations when
--chrome-symbols-dir is specified.

BUG=543897
Committed: https://crrev.com/cb149b2edc12661f910c9e1c029e91eb6c69e925
Cr-Commit-Position: refs/heads/master@{#355216}

[agrieve, 2015-10-19 19:40:06]

agrieve@chromium.org changed reviewers:
+ sgurun@chromium.org

[sgurun-gerrit only, 2015-10-19 21:15:29]

On 2015/10/19 19:40:06, agrieve wrote:

I am not super familiar with the fine details of stack script, but the changes
look fine to me. Let's ask Yoran to take a look (if not we can ask primiano) and
will be glad to stamp after that.

[sgurun-gerrit only, 2015-10-19 21:16:02]

Description was changed from

==========
Android: Make stack tool work for GN builds

This actually required a couple of changes to the script:
1. Add lib.unstripped to search paths.
2. Respect CHROMIUM_OUTPUT_DIR environment variable.
3. Don't sort by mtime when looking for libraries (resulted in stripped
   version always being selected).

This also stops the script from looking in default symbol locations when
--chrome-symbols-dir is specified.

BUG=543897
==========

to

==========
Android: Make stack tool work for GN builds

This actually required a couple of changes to the script:
1. Add lib.unstripped to search paths.
2. Respect CHROMIUM_OUTPUT_DIR environment variable.
3. Don't sort by mtime when looking for libraries (resulted in stripped
   version always being selected).

This also stops the script from looking in default symbol locations when
--chrome-symbols-dir is specified.

BUG=543897
==========

[sgurun-gerrit only, 2015-10-19 21:16:02]

sgurun@chromium.org changed reviewers:
+ yfriedman@chromium.org

[sgurun-gerrit only, 2015-10-19 21:16:19]

On 2015/10/19 21:15:29, sgurun wrote:
> On 2015/10/19 19:40:06, agrieve wrote:
> 
> I am not super familiar with the fine details of stack script, but the changes
> look fine to me. Let's ask Yoran to take a look (if not we can ask primiano)
and
> will be glad to stamp after that.

Yoran, are you a good reviewer for this?

[Yaron, 2015-10-19 21:33:14]

yfriedman@chromium.org changed reviewers:
+ johnme@chromium.org

[Yaron, 2015-10-19 21:33:14]

I think johnme@ is the only person left on chrome who's worked on this script.

[johnme, 2015-10-20 15:40:20]

Looks generally reasonable.

Please update third_party/android_platform/README.chromium with a description of
your modifications.

Yaron wrote:
> I think johnme@ is the only person left on chrome who's worked on this script.

Oh dear, and I never even worked on this script, just on a related one. Let's
give this a go.

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
File third_party/android_platform/development/scripts/symbol.py (right):

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:208: def
_GetChromeOutputDirCandidates():
This could perhaps reuse existing logic:

sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)),'..',
'..', '..', '..', 'build', 'android'))
from pylib import constants

def _GetChromeOutputDirCandidates():
  if os.environ.get('CHROMIUM_OUTPUT_DIR') or os.environ.get('BUILDTYPE'):
    return [constants.GetOutDirectory()]
  return [constants.GetOutDirectory(build_type='Debug'),
          constants.GetOutDirectory(build_type='Release')]

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:321: # Sorting
doens't work for native libraries because the stripped version is
doesn't

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:322: # always newer
than the unstripped version.
Hmm, but not sorting seems a risky way to get the unstripped version. Especially
since sorting by mtime seems to be the only way this auto-detects whether you
want Debug or Release (fragile though this is).

Why not add a prefer_unstripped parameter to GetCandidates, which just
partitions the candidates (after they are sorted by mtime) as follows:

# Move unstripped to front of list
candidates.sort(key=lambda c: 'unstripped' not in c)

[johnme, 2015-10-20 15:40:20]

Looks generally reasonable.

Please update third_party/android_platform/README.chromium with a description of
your modifications.

Yaron wrote:
> I think johnme@ is the only person left on chrome who's worked on this script.

Oh dear, and I never even worked on this script, just on a related one. Let's
give this a go.

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
File third_party/android_platform/development/scripts/symbol.py (right):

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:208: def
_GetChromeOutputDirCandidates():
This could perhaps reuse existing logic:

sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)),'..',
'..', '..', '..', 'build', 'android'))
from pylib import constants

def _GetChromeOutputDirCandidates():
  if os.environ.get('CHROMIUM_OUTPUT_DIR') or os.environ.get('BUILDTYPE'):
    return [constants.GetOutDirectory()]
  return [constants.GetOutDirectory(build_type='Debug'),
          constants.GetOutDirectory(build_type='Release')]

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:321: # Sorting
doens't work for native libraries because the stripped version is
doesn't

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:322: # always newer
than the unstripped version.
Hmm, but not sorting seems a risky way to get the unstripped version. Especially
since sorting by mtime seems to be the only way this auto-detects whether you
want Debug or Release (fragile though this is).

Why not add a prefer_unstripped parameter to GetCandidates, which just
partitions the candidates (after they are sorted by mtime) as follows:

# Move unstripped to front of list
candidates.sort(key=lambda c: 'unstripped' not in c)

[agrieve, 2015-10-20 18:24:28]

The README file already describes that stack script as being entirely
re-written, so I don't think there's anything to update for this.

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
File third_party/android_platform/development/scripts/symbol.py (right):

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:208: def
_GetChromeOutputDirCandidates():
On 2015/10/20 15:40:20, johnme wrote:
> This could perhaps reuse existing logic:
> 
> sys.path.append(os.path.join(os.path.abspath(os.path.dirname(__file__)),'..',
> '..', '..', '..', 'build', 'android'))
> from pylib import constants
> 
> def _GetChromeOutputDirCandidates():
>   if os.environ.get('CHROMIUM_OUTPUT_DIR') or os.environ.get('BUILDTYPE'):
>     return [constants.GetOutDirectory()]
>   return [constants.GetOutDirectory(build_type='Debug'),
>           constants.GetOutDirectory(build_type='Release')]

Done.

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:321: # Sorting
doens't work for native libraries because the stripped version is
On 2015/10/20 15:40:20, johnme wrote:
> doesn't

Done.

https://codereview.chromium.org/1412293002/diff/1/third_party/android_platfor...
third_party/android_platform/development/scripts/symbol.py:322: # always newer
than the unstripped version.
On 2015/10/20 15:40:20, johnme wrote:
> Hmm, but not sorting seems a risky way to get the unstripped version.
Especially
> since sorting by mtime seems to be the only way this auto-detects whether you
> want Debug or Release (fragile though this is).
> 
> Why not add a prefer_unstripped parameter to GetCandidates, which just
> partitions the candidates (after they are sorted by mtime) as follows:
> 
> # Move unstripped to front of list
> candidates.sort(key=lambda c: 'unstripped' not in c)

Like it. ;)

[johnme, 2015-10-20 18:53:27]

lgtm with comments addressed

https://codereview.chromium.org/1412293002/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/1412293002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:51: DCHECK(false) << "HA";
I take it this is accidental ;)

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
File third_party/android_platform/development/scripts/symbol.py (right):

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:207: for suffix in
suffix_list for prefix in prefix_list ]
Nit: is there still any reason to change this?

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:227: A list of
candidate files filtered by candidate_fun, in the order given.
s/in the order given/newest first/

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:301: return
[CHROME_SYMBOLS_DIR]
This seems to be a change in behaviour. Previously it would try to append 'lib',
'lib.target' etc to CHROME_SYMBOLS_DIR, whereas now it only looks in that exact
path. Is that intentional?

[agrieve, 2015-10-20 19:27:49]

Thanks for the review!

https://codereview.chromium.org/1412293002/diff/20001/chrome/browser/chrome_b...
File chrome/browser/chrome_browser_main_android.cc (right):

https://codereview.chromium.org/1412293002/diff/20001/chrome/browser/chrome_b...
chrome/browser/chrome_browser_main_android.cc:51: DCHECK(false) << "HA";
On 2015/10/20 18:53:27, johnme wrote:
> I take it this is accidental ;)

yeah... about that... :P

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
File third_party/android_platform/development/scripts/symbol.py (right):

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:207: for suffix in
suffix_list for prefix in prefix_list ]
On 2015/10/20 18:53:27, johnme wrote:
> Nit: is there still any reason to change this?

Shouldn't be necessary anymore (it was needed to sort lib.unstripped before
others).
However, I'd like to keep it since it seems like a smarted way to apply
prefix_lists / suffix_list (sort by prefix, then suffix rather than suffix, then
prefix)

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:227: A list of
candidate files filtered by candidate_fun, in the order given.
On 2015/10/20 18:53:27, johnme wrote:
> s/in the order given/newest first/

Done.

https://codereview.chromium.org/1412293002/diff/20001/third_party/android_pla...
third_party/android_platform/development/scripts/symbol.py:301: return
[CHROME_SYMBOLS_DIR]
On 2015/10/20 18:53:27, johnme wrote:
> This seems to be a change in behaviour. Previously it would try to append
'lib',
> 'lib.target' etc to CHROME_SYMBOLS_DIR, whereas now it only looks in that
exact
> path. Is that intentional?

Intentional:

Note that it does *not* work if you set --chrome-symbols-dir to your out/Debug,
but *does* work if you set CHROMIUM_OUTPUT_DIR=out/Debug. This environment
variable is consistent with most other scripts (e.g. adb_gdb).

The reason it doesn't work, is that the stripped .so files exist under
out-gn/Debug, so you'd actually want to use out-gn/Debug/lib.unstripped, and I
couldn't stomach having --chrome-symbols-dir not use the .so files I explicitly
pointed it at.

[agrieve, 2015-10-20 19:30:07]

The CQ bit was checked by agrieve@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-10-20 19:30:59]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412293002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412293002/40001

[commit-bot: I haz the power, 2015-10-20 20:02:55]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-10-20 20:02:55]

Dry run: This issue passed the CQ dry run.

[sgurun-gerrit only, 2015-10-20 20:26:53]

On 2015/10/20 20:02:55, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

thanks John for the review and Andrew for the code.

lgtm

[agrieve, 2015-10-21 01:41:25]

The CQ bit was checked by agrieve@chromium.org

[agrieve, 2015-10-21 01:41:26]

The patchset sent to the CQ was uploaded after l-g-t-m from johnme@chromium.org
Link to the patchset: https://codereview.chromium.org/1412293002/#ps40001
(title: "comments 2")

[commit-bot: I haz the power, 2015-10-21 01:41:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1412293002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1412293002/40001

[commit-bot: I haz the power, 2015-10-21 01:46:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-10-21 01:47:02]

Patchset 3 (id:??) landed as
https://crrev.com/cb149b2edc12661f910c9e1c029e91eb6c69e925
Cr-Commit-Position: refs/heads/master@{#355216}