Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2218053002:
Create gn target for peerconnection examples (Closed)
|
Created:
4 years, 4 months ago by kthelgason Modified:
4 years, 4 months ago Reviewers:
tommi CC:
webrtc-reviews_webrtc.org, tterriberry_mozilla.com Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionCreate gn target for peerconnection examples
BUG=
Committed: https://crrev.com/0727f15186fb05f55d69d87ff9633ccdade10258
Cr-Commit-Position: refs/heads/master@{#13673}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Add support for building examples on win #Patch Set 3 : include webrtc.gni for windows also #Patch Set 4 : Silence integer truncation warnings from vs #Patch Set 5 : Actually ignore the correct c number #Patch Set 6 : Add msvs subsystem linker flag #Patch Set 7 : rebase onto master #Patch Set 8 : Use predefined config #
Total comments: 1
Messages
Total messages: 24 (15 generated)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
kthelgason@webrtc.org changed reviewers: + tommi@webrtc.org
https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:24: if (is_linux) { would prefer to include the same platforms as we have in the gyp file https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn#newc... webrtc/examples/BUILD.gn:118: #TODO(kthelgason): Support building on Windows nit: space after #
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_libfuzzer_rel on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/linux_libfuzzer_rel/bui...)
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was checked by kthelgason@webrtc.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_dbg on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/android_dbg/builds/15423)
On 2016/08/05 13:21:32, tommi-webrtc wrote: > https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn > File webrtc/examples/BUILD.gn (right): > > https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn#newc... > webrtc/examples/BUILD.gn:24: if (is_linux) { > would prefer to include the same platforms as we have in the gyp file > > https://codereview.webrtc.org/2218053002/diff/1/webrtc/examples/BUILD.gn#newc... > webrtc/examples/BUILD.gn:118: #TODO(kthelgason): Support building on Windows > nit: space after # AFAICT windows issues have now been resolved and CI is green(ish). I suspect there is some unrelated issue on the one android buildbot thats giving me a hard time. Care to take a look?
https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn File webrtc/examples/BUILD.gn (right): https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn... webrtc/examples/BUILD.gn:384: if (is_linux) { seems like we have several of these checks repeated. I'm not sure what we can do about it though. Does this reflect the checks in the gyp file?
On 2016/08/08 13:17:18, tommi-webrtc wrote: > https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn > File webrtc/examples/BUILD.gn (right): > > https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn... > webrtc/examples/BUILD.gn:384: if (is_linux) { > seems like we have several of these checks repeated. I'm not sure what we can > do about it though. > Does this reflect the checks in the gyp file? lgtm though. Seems like a good start at the very least and improvements can be made incrementally.
On 2016/08/08 13:17:18, tommi-webrtc wrote: > https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn > File webrtc/examples/BUILD.gn (right): > > https://codereview.webrtc.org/2218053002/diff/140001/webrtc/examples/BUILD.gn... > webrtc/examples/BUILD.gn:384: if (is_linux) { > seems like we have several of these checks repeated. I'm not sure what we can > do about it though. Yeah this really bothered me as well, but I don't know gn syntax well enough to find a way around this. > Does this reflect the checks in the gyp file? This was not an issue in the gyp files as the `clang_use_chrome_plugins` was global in gyp but in gn it's effect is just local to a specific file. At least this is my understanding, and may well be very wrong. This way of bypassing the check is used all over the WebRTC codebase, so there is precedent for doing it this way.
The CQ bit was checked by kthelgason@webrtc.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Create gn target for peerconnection examples BUG= ========== to ========== Create gn target for peerconnection examples BUG= Committed: https://crrev.com/0727f15186fb05f55d69d87ff9633ccdade10258 Cr-Commit-Position: refs/heads/master@{#13673} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0727f15186fb05f55d69d87ff9633ccdade10258 Cr-Commit-Position: refs/heads/master@{#13673} |
