Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1660693002:
Add --output-dir flag to third_party/android_platform/development/scripts/stack (Closed)
DescriptionAdd --output-directory flag to tombstones.py, & friends
Friends:
* third_party/android_platform/development/scripts/stack
* build/android/asan_symbolize.py
Will make it mandatory in stack as in a follow-up.
BUG=573345
Committed: https://crrev.com/32dbc200de3d8d092fe4b83ef36c0fa71885f5eb
Cr-Commit-Position: refs/heads/master@{#373991}
Patch Set 1 #Patch Set 2 : output-dir -> output-directory #Patch Set 3 : git checkout - #
Total comments: 2
Patch Set 4 : add tombstones logic #
Total comments: 5
Patch Set 5 : redo #
Total comments: 7
Patch Set 6 : review comments #Patch Set 7 : Remove CheckOutputDirectory() in stack #Patch Set 8 : tombstones too #Patch Set 9 : fix bot #Patch Set 10 : asan_symbolize #Patch Set 11 : fix fix fix #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 47 (25 generated)
agrieve@chromium.org changed reviewers: + pasko@chromium.org, rmcilroy@chromium.org
(⌣́_⌣̀)
Description was changed from ========== Add --output-dir flag to third_party/android_platform/development/scripts/stack It is always required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) BUG=573345 ========== to ========== Add --output-directory flag to third_party/android_platform/development/scripts/stack It is always required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) BUG=573345 ==========
Description was changed from ========== Add --output-directory flag to third_party/android_platform/development/scripts/stack It is always required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, android_platform/.../stack It is always required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) BUG=573345 ==========
The CQ bit was checked by agrieve@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/02/03 06:44:14, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) The state of stack_tool_with_logcat_dump makes me sad. Should we update the recipes first?
https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... third_party/android_platform/development/scripts/stack:30: Please update src/third_party/android_platform/README.chromium with a line mentioning these changes. https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... third_party/android_platform/development/scripts/stack:34: from pylib import constants I'd rather not do this. Since this script comes from the Android tree I'd rather keep it self contained wherever possible. Could we add this check without importing build/android/constants?
jbudorick@chromium.org changed reviewers: + jbudorick@chromium.org
https://codereview.chromium.org/1660693002/diff/40001/build/android/pylib/con... File build/android/pylib/constants/__init__.py (right): https://codereview.chromium.org/1660693002/diff/40001/build/android/pylib/con... build/android/pylib/constants/__init__.py:237: def GetOutDirectory(build_type=None): I don't think this is the right place to do this validation. I'd strongly prefer doing it in a separate function that scripts call directly at argument parsing time.
Re-worked to use a more explicit Check function and I think it's much better now. Also found that stack's --chrome-apk-dir is GYP-specific, so added a TODO to make it work for GN. https://codereview.chromium.org/1660693002/diff/40001/build/android/pylib/con... File build/android/pylib/constants/__init__.py (right): https://codereview.chromium.org/1660693002/diff/40001/build/android/pylib/con... build/android/pylib/constants/__init__.py:237: def GetOutDirectory(build_type=None): On 2016/02/03 14:22:27, jbudorick wrote: > I don't think this is the right place to do this validation. I'd strongly prefer > doing it in a separate function that scripts call directly at argument parsing > time. Done (and I think it looks much nicer now!) https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... third_party/android_platform/development/scripts/stack:30: On 2016/02/03 12:35:39, rmcilroy wrote: > Please update src/third_party/android_platform/README.chromium with a line > mentioning these changes. Done. https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... third_party/android_platform/development/scripts/stack:34: from pylib import constants On 2016/02/03 12:35:39, rmcilroy wrote: > I'd rather not do this. Since this script comes from the Android tree I'd rather > keep it self contained wherever possible. Could we add this check without > importing build/android/constants? We already import constants within symbol.py, so it makes sense to use the same logic here I think.
LGTM but please wait for jbudorick to be happy too. https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... File third_party/android_platform/development/scripts/stack (right): https://codereview.chromium.org/1660693002/diff/60001/third_party/android_pla... third_party/android_platform/development/scripts/stack:34: from pylib import constants On 2016/02/03 20:27:17, agrieve wrote: > On 2016/02/03 12:35:39, rmcilroy wrote: > > I'd rather not do this. Since this script comes from the Android tree I'd > rather > > keep it self contained wherever possible. Could we add this check without > > importing build/android/constants? > > We already import constants within symbol.py, so it makes sense to use the same > logic here I think. You are right, ok.
lgtm with a couple of nits && jbudorick approves && trybots are happy https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... File build/android/pylib/constants/__init__.py (right): https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... build/android/pylib/constants/__init__.py:247: def CheckOutDirectory(): nit: s/Out/Output/ because it is done after SetOutputDirectory() https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... build/android/pylib/constants/__init__.py:264: raise EnvironmentError('Neither CHROMIUM_OUTPUT_DIR nor CHROMIUM_OUT_DIR ' EnvironmentError is something that keeps a filename and an errno, raising it in this case is causing questions like "how will these be used? would errno be the right one? is it used to be caught where we expect IOError amd OSError?". If this class is not essential, please just 'raise Esxception(...)'. If it is essential, I'd like to know why.
lgtm w/ nit https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... File build/android/pylib/constants/__init__.py (right): https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... build/android/pylib/constants/__init__.py:264: raise EnvironmentError('Neither CHROMIUM_OUTPUT_DIR nor CHROMIUM_OUT_DIR ' On 2016/02/04 12:53:40, pasko (OOO through 2016-02-19) wrote: > EnvironmentError is something that keeps a filename and an errno, raising it in > this case is causing questions like "how will these be used? would errno be the > right one? is it used to be caught where we expect IOError amd OSError?". If > this class is not essential, please just 'raise Esxception(...)'. If it is > essential, I'd like to know why. Agreed. https://codereview.chromium.org/1660693002/diff/80001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/1660693002/diff/80001/build/android/tombstone... build/android/tombstones.py:133: # --output-directory is passed implicitly via the CHROMIUM_OUTPUT_DIR nit: I'd prefer explicitly passing it on the command line.
https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... File build/android/pylib/constants/__init__.py (right): https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... build/android/pylib/constants/__init__.py:247: def CheckOutDirectory(): On 2016/02/04 12:53:40, pasko (OOO through 2016-02-19) wrote: > nit: s/Out/Output/ because it is done after SetOutputDirectory() Done. https://codereview.chromium.org/1660693002/diff/80001/build/android/pylib/con... build/android/pylib/constants/__init__.py:264: raise EnvironmentError('Neither CHROMIUM_OUTPUT_DIR nor CHROMIUM_OUT_DIR ' On 2016/02/04 12:53:40, pasko (OOO through 2016-02-19) wrote: > EnvironmentError is something that keeps a filename and an errno, raising it in > this case is causing questions like "how will these be used? would errno be the > right one? is it used to be caught where we expect IOError amd OSError?". If > this class is not essential, please just 'raise Esxception(...)'. If it is > essential, I'd like to know why. Done. https://codereview.chromium.org/1660693002/diff/80001/build/android/tombstone... File build/android/tombstones.py (right): https://codereview.chromium.org/1660693002/diff/80001/build/android/tombstone... build/android/tombstones.py:133: # --output-directory is passed implicitly via the CHROMIUM_OUTPUT_DIR On 2016/02/04 16:13:45, jbudorick wrote: > nit: I'd prefer explicitly passing it on the command line. Done.
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jbudorick@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1660693002/#ps100001 (title: "review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/02/04 20:58:53, commit-bot: I haz the power wrote: > Try jobs failed on following builders: > linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) We're going to have to jump through a few hoops to land this. Split off a separate CL that adds no-op --output-directory arguments to tombstones.py and stack.
Description was changed from ========== Add --output-directory flag to tombstones.py, android_platform/.../stack It is always required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, android_platform/.../stack Make it mandatory in tombstones.py. Mandatory means: Required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) Will make it mandatory in stack as in a follow-up. BUG=573345 ==========
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jbudorick@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1660693002/#ps120001 (title: "Remove CheckOutputDirectory() in stack")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/120001
The CQ bit was unchecked by agrieve@chromium.org
Description was changed from ========== Add --output-directory flag to tombstones.py, android_platform/.../stack Make it mandatory in tombstones.py. Mandatory means: Required, unless CHROMIUM_OUTPUT_DIR or CHROMIUM_OUT_DIR are set or when the current working directory is an output directory (contains a build.ninja file) Will make it mandatory in stack as in a follow-up. BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, android_platform/.../stack Will make it mandatory in stack as in a follow-up. BUG=573345 ==========
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jbudorick@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1660693002/#ps140001 (title: "tombstones too")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== Add --output-directory flag to tombstones.py, android_platform/.../stack Will make it mandatory in stack as in a follow-up. BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, & friends Friends: * third_party/android_platform/development/scripts/stack * build/android/asan_symbolize.py Will make it mandatory in stack as in a follow-up. BUG=573345 ==========
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jbudorick@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1660693002/#ps180001 (title: "asan_symbolize")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by agrieve@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pasko@chromium.org, jbudorick@chromium.org, rmcilroy@chromium.org Link to the patchset: https://codereview.chromium.org/1660693002/#ps200001 (title: "fix fix fix")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1660693002/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1660693002/200001
Message was sent while issue was closed.
Description was changed from ========== Add --output-directory flag to tombstones.py, & friends Friends: * third_party/android_platform/development/scripts/stack * build/android/asan_symbolize.py Will make it mandatory in stack as in a follow-up. BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, & friends Friends: * third_party/android_platform/development/scripts/stack * build/android/asan_symbolize.py Will make it mandatory in stack as in a follow-up. BUG=573345 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Add --output-directory flag to tombstones.py, & friends Friends: * third_party/android_platform/development/scripts/stack * build/android/asan_symbolize.py Will make it mandatory in stack as in a follow-up. BUG=573345 ========== to ========== Add --output-directory flag to tombstones.py, & friends Friends: * third_party/android_platform/development/scripts/stack * build/android/asan_symbolize.py Will make it mandatory in stack as in a follow-up. BUG=573345 Committed: https://crrev.com/32dbc200de3d8d092fe4b83ef36c0fa71885f5eb Cr-Commit-Position: refs/heads/master@{#373991} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/32dbc200de3d8d092fe4b83ef36c0fa71885f5eb Cr-Commit-Position: refs/heads/master@{#373991} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
