Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2343243002:
tools/coverage: clean up (Closed)
|
Created:
4 years, 3 months ago by hal.canary Modified:
4 years, 2 months ago CC:
reviews_skia.org Target Ref:
refs/heads/master Project:
skia Visibility:
Public. |
Descriptionbin/coverage: a GN version of the coverage script
intended to replace tools/coverage when GYP is gone.
GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002
Committed: https://skia.googlesource.com/skia/+/3948a1bf18c246225f6aa11006e94bb3d396eb62
Patch Set 1 #Patch Set 2 : GN version #
Total comments: 12
Patch Set 3 : comments form klein #Patch Set 4 : rm old version #
Total comments: 8
Patch Set 5 : final comments from mtklein@ #Patch Set 6 : final changes, plus help #Messages
Total messages: 19 (9 generated)
Description was changed from ========== tools/coverage: clean up BUG=skia: ========== to ========== tools/coverage: clean up BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ==========
Description was changed from ========== tools/coverage: clean up BUG=skia: GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ========== to ========== tools/coverage: clean up - Enforce use of GCC. - Only build executable, not all of Skia. - Run from any directory. - Unset SKIA_OUT if needed. - genhtml --ignore-errors source ==========
halcanary@google.com changed reviewers: + mtklein@google.com
PTAL.
mtklein@chromium.org changed reviewers: + mtklein@chromium.org
This is GYP specific. I will not keep this working. If you want to have something that will keep working, please use GN.
On 2016/09/16 18:23:31, mtklein_C wrote: > This is GYP specific. I will not keep this working. If you want to have > something that will keep working, please use GN. I spent a few minutes trying to make that work. I'll try again.
Description was changed from ========== tools/coverage: clean up - Enforce use of GCC. - Only build executable, not all of Skia. - Run from any directory. - Unset SKIA_OUT if needed. - genhtml --ignore-errors source ========== to ========== tools/coverage: clean up - Enforce use of GCC. - Only build executable, not all of Skia. - Run from any directory. - Unset SKIA_OUT if needed. - genhtml --ignore-errors source GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ==========
On 2016/09/16 18:27:04, Hal Canary wrote: > On 2016/09/16 18:23:31, mtklein_C wrote: > > This is GYP specific. I will not keep this working. If you want to have > > something that will keep working, please use GN. > > I spent a few minutes trying to make that work. I'll try again. take a look at patchset 2
Description was changed from ========== tools/coverage: clean up - Enforce use of GCC. - Only build executable, not all of Skia. - Run from any directory. - Unset SKIA_OUT if needed. - genhtml --ignore-errors source GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ========== to ========== bin/coverage: a GN version of the coverage script intended to replace tools/coverage when GYP is gone. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ==========
Better delete the old one? https://codereview.chromium.org/2343243002/diff/20001/bin/coverage File bin/coverage (right): https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode2 bin/coverage:2: # Copyright 2013 Google Inc. :/ https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode17 bin/coverage:17: EXECUTABLE="$1" What's the deal with all the quoting? Is $1 or $@ not safe to use? https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode23 bin/coverage:23: DIR="$(mktemp -d "${TMPDIR:-/tmp}/skia_coverage_XXXXXXXXXX")" Why not out/Coverage or somewhere else that lets you cache between runs? https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode26 bin/coverage:26: bin/sync Are you sure you want this here? Syncing deps can be surprising. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode29 bin/coverage:29: --args='cc="gcc -fprofile-arcs " cxx="g++ -fprofile-arcs" extra_cflags="-ftest-coverage"' \ We used to use --coverage. What's different here?
On 2016/09/16 19:21:43, mtklein_C wrote: > Better delete the old one? Done. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage File bin/coverage (right): https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode2 bin/coverage:2: # Copyright 2013 Google Inc. On 2016/09/16 19:21:43, mtklein_C wrote: > :/ It's based on a copy of a file from 2013. Done. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode17 bin/coverage:17: EXECUTABLE="$1" On 2016/09/16 19:21:43, mtklein_C wrote: > What's the deal with all the quoting? Is $1 or $@ not safe to use? It's always good habit in shell scripting. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode23 bin/coverage:23: DIR="$(mktemp -d "${TMPDIR:-/tmp}/skia_coverage_XXXXXXXXXX")" On 2016/09/16 19:21:43, mtklein_C wrote: > Why not out/Coverage or somewhere else that lets you cache between runs? I had issues with caching while testing. It's fixed now. Done. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode26 bin/coverage:26: bin/sync On 2016/09/16 19:21:43, mtklein_C wrote: > Are you sure you want this here? Syncing deps can be surprising. Suprising how? Not syncing deps before building can cause a lot of problems. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode29 bin/coverage:29: --args='cc="gcc -fprofile-arcs " cxx="g++ -fprofile-arcs" extra_cflags="-ftest-coverage"' \ On 2016/09/16 19:21:43, mtklein_C wrote: > We used to use --coverage. What's different here? not sure why I changed. `--coverage` works. Done.
lgtm, in the sense that it won't hurt anything to land. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage File bin/coverage (right): https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode17 bin/coverage:17: EXECUTABLE="$1" On 2016/09/20 18:19:01, Hal Canary wrote: > On 2016/09/16 19:21:43, mtklein_C wrote: > > What's the deal with all the quoting? Is $1 or $@ not safe to use? > > It's always good habit in shell scripting. I'm not sure that that's true. If the simple option works and you choose the complex option, someone will one day assume the complexity you're adding here matters. It's disquieting to delete code and see no effect. Please delete the unnecessary code now so that we don't feel that disquiet later. https://codereview.chromium.org/2343243002/diff/20001/bin/coverage#newcode26 bin/coverage:26: bin/sync On 2016/09/20 18:19:01, Hal Canary wrote: > On 2016/09/16 19:21:43, mtklein_C wrote: > > Are you sure you want this here? Syncing deps can be surprising. > > Suprising how? > > Not syncing deps before building can cause a lot of problems. It seems like an odd, but perhaps harmless, thing for a script handling code coverage to have to know about syncing deps. https://codereview.chromium.org/2343243002/diff/60001/bin/coverage File bin/coverage (right): https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode19 bin/coverage:19: GCOV="$(realpath tools/gcov_shim)" When you read this code as it's grouped here, do you not think that somehow GCOV depends on $@? Better move it before EXECUTABLE? https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode21 bin/coverage:21: QUIET=-q Why don't we fold this through now? https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode34 bin/coverage:34: gn gen --args="cc=\"${CC} ${FLAGS}\" cxx=\"${CXX} ${FLAGS}\"" "$BUILD" Might be clearer if you set cc, cxx, extra_cflags, and extra_ldflags? https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode53 bin/coverage:53: Might wanna kill off a blank line or two at the end of the file?
https://codereview.chromium.org/2343243002/diff/60001/bin/coverage File bin/coverage (right): https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode19 bin/coverage:19: GCOV="$(realpath tools/gcov_shim)" On 2016/09/20 22:38:53, mtklein wrote: > When you read this code as it's grouped here, do you not think that somehow GCOV > depends on $@? Better move it before EXECUTABLE? Done. https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode21 bin/coverage:21: QUIET=-q On 2016/09/20 22:38:53, mtklein wrote: > Why don't we fold this through now? Done. https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode34 bin/coverage:34: gn gen --args="cc=\"${CC} ${FLAGS}\" cxx=\"${CXX} ${FLAGS}\"" "$BUILD" On 2016/09/20 22:38:53, mtklein wrote: > Might be clearer if you set cc, cxx, extra_cflags, and extra_ldflags? Done. https://codereview.chromium.org/2343243002/diff/60001/bin/coverage#newcode53 bin/coverage:53: On 2016/09/20 22:38:53, mtklein wrote: > Might wanna kill off a blank line or two at the end of the file? Done.
The CQ bit was checked by halcanary@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mtklein@google.com Link to the patchset: https://codereview.chromium.org/2343243002/#ps100001 (title: "final changes, plus help")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== bin/coverage: a GN version of the coverage script intended to replace tools/coverage when GYP is gone. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 ========== to ========== bin/coverage: a GN version of the coverage script intended to replace tools/coverage when GYP is gone. GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=2343243002 Committed: https://skia.googlesource.com/skia/+/3948a1bf18c246225f6aa11006e94bb3d396eb62 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001) as https://skia.googlesource.com/skia/+/3948a1bf18c246225f6aa11006e94bb3d396eb62 |
