Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1875193002:
Fix coverage build. (Closed)
|
Created:
4 years, 8 months ago by kjellander_webrtc Modified:
4 years, 8 months ago CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com, qiang.lu, niklas.enbom, yujie_mao (webrtc), peah-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionFix coverage build.
NOTICE: The coverage build is not officially supported and may break
at any point.
Patch receieved from johan.ahlers@gmail.com.
BUG=webrtc:5754
NOTRY=True
TBR=henrika@webrtc.org
Committed: https://crrev.com/6e6941f4090a080d6d6b234d9301e424946b6314
Cr-Commit-Position: refs/heads/master@{#12420}
Patch Set 1 #
Total comments: 5
Messages
Total messages: 28 (13 generated)
Description was changed from ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 ========== to ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 ==========
kjellander@webrtc.org changed reviewers: + machenbach@chromium.org
kjellander@webrtc.org changed reviewers: + johan.ahlers@gmail.com
Michael, I don't intend to submit this as it is. What's your recommendation on this? https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... webrtc/build/common.gypi:372: ['coverage==1 and OS=="linux"', { I think we should add clang==0 here to make it clear that it's GCC's coverage. https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... webrtc/build/common.gypi:375: 'ldflags': [ '--coverage' ], Adding this, which seems to be Clang specific, just confuses the user. We should probably adapt something similar to V8's https://chromium.googlesource.com/v8/v8/+/master/build/standalone.gypi instead. See https://bugs.chromium.org/p/chromium/issues/detail?id=568949 for the great work machenbach@ has done for V8's coverage effort.
https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... webrtc/build/common.gypi:372: ['coverage==1 and OS=="linux"', { On 2016/04/11 19:53:08, kjellander (webrtc) wrote: > I think we should add clang==0 here to make it clear that it's GCC's coverage. The cflags are valid for both the clang and the gcc coverage builds. https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... webrtc/build/common.gypi:375: 'ldflags': [ '--coverage' ], On 2016/04/11 19:53:08, kjellander (webrtc) wrote: > Adding this, which seems to be Clang specific, just confuses the user. > We should probably adapt something similar to V8's > https://chromium.googlesource.com/v8/v8/+/master/build/standalone.gypi instead. > > See https://bugs.chromium.org/p/chromium/issues/detail?id=568949 for the great > work machenbach@ has done for V8's coverage effort. This seems to fix the clang case as described e.g. here: http://logan.tw/posts/2015/04/28/check-code-coverage-with-clang-and-lcov/ GCC needs 'ldflags': [ '-fprofile-arcs']. If you want to support both, you need to add clang=1 or clang=0 to the conditions.
On 2016/04/12 08:58:34, Michael Achenbach wrote: > https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi > File webrtc/build/common.gypi (right): > > https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... > webrtc/build/common.gypi:372: ['coverage==1 and OS=="linux"', { > On 2016/04/11 19:53:08, kjellander (webrtc) wrote: > > I think we should add clang==0 here to make it clear that it's GCC's coverage. > I think one could keep this simple. Using --coverage *should* work both for clang and gcc (tested only with minimal example - I have problems with the webrtc clang=0 build variant) From the gcc 4.8 manpage: gcc (Ubuntu 4.8.4-2ubuntu1~14.04.1) 4.8.4 " --coverage This option is used to compile and link code instrumented for coverage analysis. The option is a synonym for -fprofile-arcs -ftest-coverage (when compiling) and -lgcov (when linking). See the documentation for those options for more details." > The cflags are valid for both the clang and the gcc coverage builds. > > https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... > webrtc/build/common.gypi:375: 'ldflags': [ '--coverage' ], > On 2016/04/11 19:53:08, kjellander (webrtc) wrote: > > Adding this, which seems to be Clang specific, just confuses the user. > > We should probably adapt something similar to V8's > > https://chromium.googlesource.com/v8/v8/+/master/build/standalone.gypi > instead. > > > > See https://bugs.chromium.org/p/chromium/issues/detail?id=568949 for the great > > work machenbach@ has done for V8's coverage effort. > > This seems to fix the clang case as described e.g. here: > http://logan.tw/posts/2015/04/28/check-code-coverage-with-clang-and-lcov/ > > GCC needs 'ldflags': [ '-fprofile-arcs']. If you want to support both, you need > to add clang=1 or clang=0 to the conditions.
Let's submit this for now, to unblock Johan. https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi File webrtc/build/common.gypi (right): https://codereview.webrtc.org/1875193002/diff/1/webrtc/build/common.gypi#newc... webrtc/build/common.gypi:375: 'ldflags': [ '--coverage' ], On 2016/04/12 08:58:34, Michael Achenbach wrote: > On 2016/04/11 19:53:08, kjellander (webrtc) wrote: > > Adding this, which seems to be Clang specific, just confuses the user. > > We should probably adapt something similar to V8's > > https://chromium.googlesource.com/v8/v8/+/master/build/standalone.gypi > instead. > > > > See https://bugs.chromium.org/p/chromium/issues/detail?id=568949 for the great > > work machenbach@ has done for V8's coverage effort. > > This seems to fix the clang case as described e.g. here: > http://logan.tw/posts/2015/04/28/check-code-coverage-with-clang-and-lcov/ > > GCC needs 'ldflags': [ '-fprofile-arcs']. If you want to support both, you need > to add clang=1 or clang=0 to the conditions. OK. If it needs more work when/if we decide to setup Clang coverage that's fine with me.
Description was changed from ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 ========== to ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True ==========
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875193002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875193002/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
Doh, lgtm
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875193002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875193002/1
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. Committers are members of the group "project-webrtc-committers". Note that this has nothing to do with OWNERS files.
On 2016/04/19 05:35:29, commit-bot: I haz the power wrote: > No L-G-T-M from a valid reviewer yet. > CQ run can only be started by full committers or once the patch has > received an L-G-T-M from a full committer. > Even if an L-G-T-M may have been provided, it was from a non-committer, > _not_ a full super star committer. > Committers are members of the group "project-webrtc-committers". > Note that this has nothing to do with OWNERS files. Oh, right. I created this CL, not Johan ;) TBR'ing henrika@ for OWNERS
Description was changed from ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True ========== to ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True TBR=henrika@webrtc.org ==========
kjellander@webrtc.org changed reviewers: + henrika@chromium.org
+henrika@ for owners rubberstamp.
The CQ bit was checked by kjellander@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1875193002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1875193002/1
Message was sent while issue was closed.
Description was changed from ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True TBR=henrika@webrtc.org ========== to ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True TBR=henrika@webrtc.org ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True TBR=henrika@webrtc.org ========== to ========== Fix coverage build. NOTICE: The coverage build is not officially supported and may break at any point. Patch receieved from johan.ahlers@gmail.com. BUG=webrtc:5754 NOTRY=True TBR=henrika@webrtc.org Committed: https://crrev.com/6e6941f4090a080d6d6b234d9301e424946b6314 Cr-Commit-Position: refs/heads/master@{#12420} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/6e6941f4090a080d6d6b234d9301e424946b6314 Cr-Commit-Position: refs/heads/master@{#12420}
Message was sent while issue was closed.
LGTM |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
