Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2321963002:
Only expose gflags target in non-Chromium and non-fuzzer builds. (Closed)
|
Created:
4 years, 3 months ago by kjellander_webrtc Modified:
4 years, 3 months ago CC:
webrtc-reviews_webrtc.org Target Ref:
refs/pending/heads/master Project:
webrtc Visibility:
Public. |
DescriptionOnly expose gflags target in non-Chromium and non-fuzzer builds.
Since gflags is not present in Chromium nor the libfuzzer infrastructure,
we have to ensure we don't accidentally depend on it in WebRTC code
that is used in such places.
BUG=chromium:645069
NOTRY=True
Committed: https://crrev.com/9365338db2e0505d6e5f8ea62fa8fe2c45ea8f74
Cr-Commit-Position: refs/heads/master@{#14145}
Patch Set 1 : . #
Total comments: 2
Messages
Total messages: 18 (9 generated)
Description was changed from ========== GN: Add testonly=True attribute to gflags. Since gflags is not present in Chromium, we have to ensure we don't accidentally depend on it in our production code or tools. Since our tests are not included in a Chromium build, setting testonly=True should be a sufficient safeguard (adding check_deps rules to DEPS files seems clumsy and hard to do). BUG=chromium:645069 ========== to ========== Only expose gflags target in non-Chromium builds. Since gflags is not present in Chromium, we have to ensure we don't accidentally depend on it in our production code or tools. BUG=chromium:645069 ==========
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
kjellander@webrtc.org changed reviewers: + ehmaldonado@webrtc.org, henrik.lundin@webrtc.org
I verified this with https://codereview.webrtc.org/2315633002/ locally applied and: gn gen //out/Release --args='ffmpeg_branding="Chrome" rtc_use_h264=true use_libfuzzer=true is_asan=true is_debug=false target_cpu="x64"' The command now fails, which should prevent it breaking at least for fuzzers in the future. https://codereview.webrtc.org/2321963002/diff/40001/third_party/gflags/BUILD.gn File third_party/gflags/BUILD.gn (right): https://codereview.webrtc.org/2321963002/diff/40001/third_party/gflags/BUILD.... third_party/gflags/BUILD.gn:9: import("../../webrtc/build/webrtc.gni") Line 9-13 + 80 are the only added lines, the rest is indented existing code (except for line 43). https://codereview.webrtc.org/2321963002/diff/40001/third_party/gflags/BUILD.... third_party/gflags/BUILD.gn:43: testonly = true I added this too, doesn't hurt.
I verified this with https://codereview.webrtc.org/2315633002/ locally applied and: gn gen //out/Release --args='ffmpeg_branding="Chrome" rtc_use_h264=true use_libfuzzer=true is_asan=true is_debug=false target_cpu="x64"' The command now fails, which should prevent it breaking at least for fuzzers in the future.
I'm not sure I understand exactly what the implications of the changes are, but I'll RS LGTM if ehmaldonado approves too.
On 2016/09/08 14:14:49, hlundin-webrtc wrote: > I'm not sure I understand exactly what the implications of the changes are, but > I'll RS LGTM if ehmaldonado approves too. LGTM
I dunno what I was thinking about when I triggered the Chromium trybots, since this file doesn't even exist in a Chromium checkout :) On 2016/09/08 15:35:42, ehmaldonado_webrtc wrote: > On 2016/09/08 14:14:49, hlundin-webrtc wrote: > > I'm not sure I understand exactly what the implications of the changes are, > but > > I'll RS LGTM if ehmaldonado approves too. It's not a super-elegant solution, but it will for sure prevent anyone writing a fuzzer to depend on gflags, which is what was important here. > LGTM
Description was changed from ========== Only expose gflags target in non-Chromium builds. Since gflags is not present in Chromium, we have to ensure we don't accidentally depend on it in our production code or tools. BUG=chromium:645069 ========== to ========== Only expose gflags target in non-Chromium builds. Since gflags is not present in Chromium, we have to ensure we don't accidentally depend on it in our production code or tools. BUG=chromium:645069 NOTRY=True ==========
Description was changed from ========== Only expose gflags target in non-Chromium builds. Since gflags is not present in Chromium, we have to ensure we don't accidentally depend on it in our production code or tools. BUG=chromium:645069 NOTRY=True ========== to ========== Only expose gflags target in non-Chromium and non-fuzzer builds. Since gflags is not present in Chromium nor the libfuzzer infrastructure, we have to ensure we don't accidentally depend on it in WebRTC code that is used in such places. BUG=chromium:645069 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/v2/patch-status/codereview.webrtc.org/...
Message was sent while issue was closed.
Description was changed from ========== Only expose gflags target in non-Chromium and non-fuzzer builds. Since gflags is not present in Chromium nor the libfuzzer infrastructure, we have to ensure we don't accidentally depend on it in WebRTC code that is used in such places. BUG=chromium:645069 NOTRY=True ========== to ========== Only expose gflags target in non-Chromium and non-fuzzer builds. Since gflags is not present in Chromium nor the libfuzzer infrastructure, we have to ensure we don't accidentally depend on it in WebRTC code that is used in such places. BUG=chromium:645069 NOTRY=True ==========
Message was sent while issue was closed.
Committed patchset #1 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Only expose gflags target in non-Chromium and non-fuzzer builds. Since gflags is not present in Chromium nor the libfuzzer infrastructure, we have to ensure we don't accidentally depend on it in WebRTC code that is used in such places. BUG=chromium:645069 NOTRY=True ========== to ========== Only expose gflags target in non-Chromium and non-fuzzer builds. Since gflags is not present in Chromium nor the libfuzzer infrastructure, we have to ensure we don't accidentally depend on it in WebRTC code that is used in such places. BUG=chromium:645069 NOTRY=True Committed: https://crrev.com/9365338db2e0505d6e5f8ea62fa8fe2c45ea8f74 Cr-Commit-Position: refs/heads/master@{#14145} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/9365338db2e0505d6e5f8ea62fa8fe2c45ea8f74 Cr-Commit-Position: refs/heads/master@{#14145}
Message was sent while issue was closed.
A revert of this CL (patchset #1 id:40001) has been created in https://codereview.webrtc.org/2320723007/ by kjellander@webrtc.org. The reason for reverting is: Eh, I forgot to run the libfuzzer trybot :( broke in the main waterfall.. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
