Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2498603002:
Remove unused build override (Closed)
|
Created:
4 years, 1 month ago by kthelgason_chromium Modified:
4 years, 1 month ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove unused build override
The only downstream project using this used to be WebRTC, where it's no
longer needed.
BUG=webrtc:6431
NOTRY=true
Committed: https://crrev.com/6dd0024530bfa1d1f88840204633788bad70239e
Cr-Commit-Position: refs/heads/master@{#433856}
Patch Set 1 #
Total comments: 2
Patch Set 2 : fix comment #Messages
Total messages: 17 (7 generated)
kthelgason@chromium.org changed reviewers: + dpranke@chromium.org
kjellander@chromium.org changed reviewers: + kjellander@chromium.org
Thanks for this, found a small nit https://codereview.chromium.org/2498603002/diff/1/build_overrides/build.gni File build_overrides/build.gni (right): https://codereview.chromium.org/2498603002/diff/1/build_overrides/build.gni#n... build_overrides/build.gni:5: # Several dependencies of Chrome require a different deployment target I think you should keep "min sdk" instead?
https://codereview.chromium.org/2498603002/diff/1/build_overrides/build.gni File build_overrides/build.gni (right): https://codereview.chromium.org/2498603002/diff/1/build_overrides/build.gni#n... build_overrides/build.gni:5: # Several dependencies of Chrome require a different deployment target On 2016/11/11 12:10:18, kjellander_chromium wrote: > I think you should keep "min sdk" instead? Ah, that was stupid. Fixed!
Description was changed from ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 ========== to ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 NOTRY=true ==========
lgtm
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.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1479825818482860,
"parent_rev": "0e963572de03a32799a5e72fbcef7789bc14bbcf", "commit_rev":
"91f298b574cafe34fce86901297e0aae1f625e53"}
Message was sent while issue was closed.
Description was changed from ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 NOTRY=true ========== to ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 NOTRY=true ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 NOTRY=true ========== to ========== Remove unused build override The only downstream project using this used to be WebRTC, where it's no longer needed. BUG=webrtc:6431 NOTRY=true Committed: https://crrev.com/6dd0024530bfa1d1f88840204633788bad70239e Cr-Commit-Position: refs/heads/master@{#433856} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/6dd0024530bfa1d1f88840204633788bad70239e Cr-Commit-Position: refs/heads/master@{#433856}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2520383002/ by avi@chromium.org. The reason for reverting is: This breaks all the Mac builds. /b/build/slave/ios-simulator/build/src/buildtools/mac/gn gen //out/Debug-iphonesimulator --check -> returned 1 ERROR at //build/config/mac/mac_sdk.gni:18:27: Undefined identifier mac_deployment_target = mac_deployment_target_build_override ^----------------------------------- See //build/toolchain/mac/BUILD.gn:14:1: whence it was imported. import("//build/config/mac/mac_sdk.gni") ^-------------------------------------- See //BUILD.gn:73:1: which caused the file to be included. group("gn_all") { ^---------------- GN gen failed: 1 .
Message was sent while issue was closed.
Why did you do NOTRY=true ? As noted in https://dev.chromium.org/developers/testing/commit-queue this should ONLY be used for reverts, as it has a high probability of breaking the tree, as happened here.
Message was sent while issue was closed.
Sorry about that, seems stupid in retrospect. I guess the intent was to save buildbot resources for something that seemed a trivial change. I wasn't aware of the guidelines you linked to, AFAIK we sometimes use NOTRY for things like this in WebRTC. Won't happen again. On Tue, Nov 22, 2016 at 4:16 PM, <avi@chromium.org> wrote: > Why did you do NOTRY=true ? > > As noted in https://dev.chromium.org/developers/testing/commit-queue this > should > ONLY be used for reverts, as it has a high probability of breaking the > tree, as > happened here. > > https://codereview.chromium.org/2498603002/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
On 2016/11/22 15:19:50, chromium-reviews wrote: > Sorry about that, seems stupid in retrospect. I guess the intent was to > save buildbot resources for something that seemed a trivial change. > > I wasn't aware of the guidelines you linked to, AFAIK we sometimes use > NOTRY for things like this in WebRTC. > Won't happen again. Yeah we're partially guilty to this in WebRTC since we've been scarce on trybot resources for a long time (although that has changed now with swarming). I think we should actually move over to not using NOTRY=True except in extremely trivial cases (like OWNERS-only changes) for WebRTC to align with Chromium and avoid confusion like this. I'll make sure to communicate that to our team. > On Tue, Nov 22, 2016 at 4:16 PM, <mailto:avi@chromium.org> wrote: > > > Why did you do NOTRY=true ? > > > > As noted in https://dev.chromium.org/developers/testing/commit-queue this > > should > > ONLY be used for reverts, as it has a high probability of breaking the > > tree, as > > happened here. > > > > https://codereview.chromium.org/2498603002/ > > |
