Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2267423002:
Added GN target for isac_test. (Closed)
|
Created:
4 years, 4 months ago by ivoc Modified:
4 years, 4 months ago Reviewers:
kjellander_webrtc CC:
webrtc-reviews_webrtc.org, yujie_mao (webrtc), kwiberg-webrtc, hlundin-webrtc, tlegrand-webrtc, tterriberry_mozilla.com, audio-team_agora.io, qiang.lu, niklas.enbom, peah-webrtc, minyue-webrtc Base URL:
https://chromium.googlesource.com/external/webrtc.git@master Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionAdded GN target for isac_test.
BUG=webrtc:6191
NOTRY=True
NOPRESUBMIT=True
Committed: https://crrev.com/e51b41ae4431abeac2eab4e9004172273722dde5
Cr-Commit-Position: refs/heads/master@{#13884}
Patch Set 1 #
Total comments: 10
Patch Set 2 : Addressed review comments. #Patch Set 3 : Sorted order of check_targets. #Patch Set 4 : Rebase. #
Messages
Total messages: 25 (14 generated)
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 ==========
ivoc@webrtc.org changed reviewers: + kjellander@webrtc.org
Hi Henrik, here's another one, PTAL!
https://codereview.webrtc.org/2267423002/diff/1/.gn File .gn (right): https://codereview.webrtc.org/2267423002/diff/1/.gn#newcode27 .gn:27: "//webrtc/modules/audio_coding:isac_test", Sort alphabetically. Nice to see new additions here though! https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1553: "codecs/isac/main/include", Sort alphabetically (please fix GYP version too) https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1555: "../..", This is not needed for GN. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1560: "../../base:rtc_base", Please add "//build/config/sanitizers:deps" as well, it's likely (but not necessarily) needed in order to build with sanitizers enabled. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1560: "../../base:rtc_base", Is rtc_base really needed? rtc_base_approved is usually enough. Can you add that dependency to the GYP target too?
Thanks for the comments, PTAL. https://codereview.webrtc.org/2267423002/diff/1/.gn File .gn (right): https://codereview.webrtc.org/2267423002/diff/1/.gn#newcode27 .gn:27: "//webrtc/modules/audio_coding:isac_test", On 2016/08/23 10:03:31, kjellander_webrtc wrote: > Sort alphabetically. > > Nice to see new additions here though! Done. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... File webrtc/modules/audio_coding/BUILD.gn (right): https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1553: "codecs/isac/main/include", On 2016/08/23 10:03:31, kjellander_webrtc wrote: > Sort alphabetically (please fix GYP version too) Done. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1555: "../..", On 2016/08/23 10:03:32, kjellander_webrtc wrote: > This is not needed for GN. Done. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1560: "../../base:rtc_base", On 2016/08/23 10:03:31, kjellander_webrtc wrote: > Please add "//build/config/sanitizers:deps" as well, it's likely (but not > necessarily) needed in order to build with sanitizers enabled. Done. https://codereview.webrtc.org/2267423002/diff/1/webrtc/modules/audio_coding/B... webrtc/modules/audio_coding/BUILD.gn:1560: "../../base:rtc_base", On 2016/08/23 10:03:32, kjellander_webrtc wrote: > Is rtc_base really needed? rtc_base_approved is usually enough. > > Can you add that dependency to the GYP target too? According to gn check it has to be rtc_base, because webrtc/base/format_macros.h is included. I think that should be fine since this is a standalone test executable. Will add it to GYP as well.
lgtm. Make sure to land with NOTRY=True (you might want to fire a non-gn trybot to be sure).
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=TRUE ==========
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=TRUE ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True ==========
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2267423002/#ps40001 (title: "Sorted order of check_targets.")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7620)
The CQ bit was checked by ivoc@webrtc.org
The patchset sent to the CQ was uploaded after l-g-t-m from kjellander@webrtc.org Link to the patchset: https://codereview.webrtc.org/2267423002/#ps60001 (title: "Rebase.")
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
Try jobs failed on following builders: presubmit on master.tryserver.webrtc (JOB_FAILED, http://build.chromium.org/p/tryserver.webrtc/builders/presubmit/builds/7645)
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True NOPRESUBMIT=True ==========
The CQ bit was checked by ivoc@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.
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True NOPRESUBMIT=True ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True NOPRESUBMIT=True ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True NOPRESUBMIT=True ========== to ========== Added GN target for isac_test. BUG=webrtc:6191 NOTRY=True NOPRESUBMIT=True Committed: https://crrev.com/e51b41ae4431abeac2eab4e9004172273722dde5 Cr-Commit-Position: refs/heads/master@{#13884} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e51b41ae4431abeac2eab4e9004172273722dde5 Cr-Commit-Position: refs/heads/master@{#13884} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
