Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1555783002:
Never auto-detect the output directory in adb_gdb (Closed)
|
Created:
4 years, 11 months ago by agrieve Modified:
4 years, 10 months ago Reviewers:
pasko CC:
chromium-reviews, klundberg+watch_chromium.org, mikecase+watch_chromium.org, yfriedman+watch_chromium.org, jbudorick+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@adb_gdb-new-flag Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionNever auto-detect the output directory in adb_gdb
Instead, require one of:
--output-directory
--out-dir
CHROMIUM_OUTPUT_DIR
CHROMIUM_OUT_DIR
CWD=output-dir
This should reduce developer confusion resulting from the wrong output
directory being auto-detected.
TBR=pasko
BUG=573345
Committed: https://crrev.com/1b3c326368e7fc9cb7caae9aba538193780a2ae9
Cr-Commit-Position: refs/heads/master@{#373580}
Patch Set 1 #Patch Set 2 : add check for CWD=output-dir #Patch Set 3 : output-dir -> output-directory #
Total comments: 6
Patch Set 4 : review comments #Messages
Total messages: 19 (7 generated)
Description was changed from ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-dir --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR This should reduce developer confusion resulting from the wrong output directory being auto-detected. BUG=573345 ========== to ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-dir --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR This should reduce developer confusion resulting from the wrong output directory being auto-detected. BUG=573345 ==========
agrieve@chromium.org changed reviewers: + pasko@chromium.org
On 2015/12/31 02:22:44, agrieve wrote: > mailto:agrieve@chromium.org changed reviewers: > + mailto:pasko@chromium.org ☍☍
WDYT about the proposal from thakis@? let's continue the discussion on the bug
On 2016/01/04 10:50:54, pasko wrote: > WDYT about the proposal from thakis@? let's continue the discussion on the bug Updated. PTAL
Description was changed from ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-dir --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR This should reduce developer confusion resulting from the wrong output directory being auto-detected. BUG=573345 ========== to ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. BUG=573345 ==========
On 2016/02/03 02:52:35, agrieve wrote: > On 2016/01/04 10:50:54, pasko wrote: > > WDYT about the proposal from thakis@? let's continue the discussion on the bug > > Updated. PTAL Did we give up on creating wrapper scripts in the output dir? I thought if we want them we should have them first and then disallow the old way. Also, since we have more precision can we please remove some smartness from detect_symbol_dir? I like the KISS principle.
On 2016/02/03 10:11:49, pasko wrote: > On 2016/02/03 02:52:35, agrieve wrote: > > On 2016/01/04 10:50:54, pasko wrote: > > > WDYT about the proposal from thakis@? let's continue the discussion on the > bug > > > > Updated. PTAL > > Did we give up on creating wrapper scripts in the output dir? I thought if we > want them we should have them first and then disallow the old way. > > Also, since we have more precision can we please remove some smartness from > detect_symbol_dir? I like the KISS principle. Didn't give up, just was going to do it next (although I do now agree it makes sense to do it first, really, I'd like to just do them all over the course of a single day). Patch for adding scripts: https://codereview.chromium.org/1663103004 Patch to remove flags that require more complicated search logic: https://codereview.chromium.org/1659413003/
https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb File build/android/adb_gdb (right): https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:317: --out-dir=<path> Specify the out directory (e.g. "out"). why do we support out-dir here and not in other scripts? Wouldn't consistency be better? https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:362: CHROMIUM_OUTPUT_DIR=$(pwd) please use "$PWD" instead it is faster than running a command, has quotes https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:364: panic "Please specify an output directory using one of: nit: s/using/by using/
https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb File build/android/adb_gdb (right): https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:317: --out-dir=<path> Specify the out directory (e.g. "out"). On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > why do we support out-dir here and not in other scripts? Wouldn't consistency be > better? Removed in https://codereview.chromium.org/1659413003/. I think it's just tied to the old ways of specifying output dir and --release / --debug separately rather than just pointing directly to output-directory. https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:362: CHROMIUM_OUTPUT_DIR=$(pwd) On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > please use "$PWD" instead > it is faster than running a command, has quotes Done. https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... build/android/adb_gdb:364: panic "Please specify an output directory using one of: On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > nit: s/using/by using/ Done.
On 2016/02/04 16:20:23, agrieve wrote: > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb > File build/android/adb_gdb (right): > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > build/android/adb_gdb:317: --out-dir=<path> Specify the out directory (e.g. > "out"). > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > why do we support out-dir here and not in other scripts? Wouldn't consistency > be > > better? > > Removed in https://codereview.chromium.org/1659413003/. > > I think it's just tied to the old ways of specifying output dir and --release / > --debug separately rather than just pointing directly to output-directory. Not necessarily anything actionable for this review, but I'd really like to move toward a world where we only allow specifying the full output-directory (vs --out-dir). I think that it's more consistent with how gn & ninja work. > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > build/android/adb_gdb:362: CHROMIUM_OUTPUT_DIR=$(pwd) > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > please use "$PWD" instead > > it is faster than running a command, has quotes > > Done. > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > build/android/adb_gdb:364: panic "Please specify an output directory using one > of: > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > nit: s/using/by using/ > > Done.
Description was changed from ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. BUG=573345 ========== to ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. TBR=pasko BUG=573345 ==========
On 2016/02/04 16:23:25, jbudorick wrote: > On 2016/02/04 16:20:23, agrieve wrote: > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb > > File build/android/adb_gdb (right): > > > > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > > build/android/adb_gdb:317: --out-dir=<path> Specify the out directory > (e.g. > > "out"). > > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > > why do we support out-dir here and not in other scripts? Wouldn't > consistency > > be > > > better? > > > > Removed in https://codereview.chromium.org/1659413003/. > > > > I think it's just tied to the old ways of specifying output dir and --release > / > > --debug separately rather than just pointing directly to output-directory. > > Not necessarily anything actionable for this review, but I'd really like to move > toward a world where we only allow specifying the full output-directory (vs > --out-dir). I think that it's more consistent with how gn & ninja work. > > > > > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > > build/android/adb_gdb:362: CHROMIUM_OUTPUT_DIR=$(pwd) > > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > > please use "$PWD" instead > > > it is faster than running a command, has quotes > > > > Done. > > > > > https://codereview.chromium.org/1555783002/diff/40001/build/android/adb_gdb#n... > > build/android/adb_gdb:364: panic "Please specify an output directory using one > > of: > > On 2016/02/04 12:57:39, pasko (OOO through 2016-02-19) wrote: > > > nit: s/using/by using/ > > > > Done. Judging by pasko's review username, he's out for a couple weeks. TBR'ing, as I think we're pretty much at an lgtm here.
The CQ bit was checked by agrieve@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1555783002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1555783002/60001
Message was sent while issue was closed.
Description was changed from ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. TBR=pasko BUG=573345 ========== to ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. TBR=pasko BUG=573345 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. TBR=pasko BUG=573345 ========== to ========== Never auto-detect the output directory in adb_gdb Instead, require one of: --output-directory --out-dir CHROMIUM_OUTPUT_DIR CHROMIUM_OUT_DIR CWD=output-dir This should reduce developer confusion resulting from the wrong output directory being auto-detected. TBR=pasko BUG=573345 Committed: https://crrev.com/1b3c326368e7fc9cb7caae9aba538193780a2ae9 Cr-Commit-Position: refs/heads/master@{#373580} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/1b3c326368e7fc9cb7caae9aba538193780a2ae9 Cr-Commit-Position: refs/heads/master@{#373580} |
