Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2779663003:
[Chromecast] Require chromecast OWNERS to be aware of locale changes. (Closed)
Description[Chromecast] Require chromecast OWNERS to be aware of locale changes.
Whenever the locale lists declared in //build/config/locales.gni change,
someone from the Chromecast team should be added to the CL as an FYI.
Enforce this via a python script that checks the locales against a fixed
list.
BUG=697090
TEST= modify the locales list, make sure the build complains.
Review-Url: https://codereview.chromium.org/2779663003
Cr-Commit-Position: refs/heads/master@{#460803}
Committed: https://chromium.googlesource.com/chromium/src/+/4f97ba825e075c7660bcb2c4dfc07e2a60d73d48
Patch Set 1 #Patch Set 2 : Prevent superfluous rebuilds. #Patch Set 3 : Updated GN comments. #
Total comments: 8
Patch Set 4 : Address feedback from thakis@ #Messages
Total messages: 22 (15 generated)
slan@chromium.org changed reviewers: + alokp@chromium.org, gfhuang@chromium.org, thakis@chromium.org
Nico, We've had some concern internally about the locales list changing under our feet since crrev.com/2773453003 landed. I think this concern is warranted: if a locale is removed or added from the list in //build/config/locales.gni, our team definitely would like to know about it. (In the case of an added locale, our .grd file would fail to parse, but not for a removed locale). This change essentially enforces that a chromecast OWNER is added to any review changing that list. Hopefully this should not happen often enough to be a burden, and even when it does, this is mostly just an FYI for our team. Please take a look at this change and let me know what you think. Thanks!
The CQ bit was checked by slan@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
The CQ bit was checked by slan@chromium.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.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
wouldn't a WATCHLIST entry for build/config/locales.gni serve the same purpose? If you think you do need this, please add a comment to the .gn file what people are supposed to do when this action fails (that's where I'd look)
On 2017/03/27 19:44:20, Nico (afk until Tue Apr 4) wrote: > wouldn't a WATCHLIST entry for build/config/locales.gni serve the same purpose? > > If you think you do need this, please add a comment to the .gn file what people > are supposed to do when this action fails (that's where I'd look) Done. WATCHLIST would keep us notified of changes, but it does not add a rigorous check to ensure that we're ready for the update. We certainly do not intend to block changes, but we would like to prepare from our end for such events. I don't anticipate that this list is changed frequently enough to slow anybody down. I'm primarily concerned about a locale that we have committed to support silently disappearing from our master branch, from which we will do automatic canary releases. However, being Chrome-y citizens of our codebase is very important to us :) If you would prefer a WATCHLIST, we can take that approach instead and implement this check downstream somewhere. Thanks for the review.
The CQ bit was checked by slan@chromium.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.chromium.or...
lgtm https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/BUILD.gn File chromecast/app/BUILD.gn (right): https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/BUILD.gn... chromecast/app/BUILD.gn:129: # This file will be touched on success, preventing an unecessary rebuild. typo unecessary. These files are usually called "stamp files". https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... File chromecast/app/verify_cast_locales.py (right): https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. nit: add shebang line, make file executable https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:51: return -1 nit: 1 is a more common "failed" exit code https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:52: nit: two blank lines between toplevel things
Thanks! https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/BUILD.gn File chromecast/app/BUILD.gn (right): https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/BUILD.gn... chromecast/app/BUILD.gn:129: # This file will be touched on success, preventing an unecessary rebuild. On 2017/03/30 15:31:47, Nico (afk until Tue Apr 4) wrote: > typo unecessary. These files are usually called "stamp files". Done. Comments and arg updated. https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... File chromecast/app/verify_cast_locales.py (right): https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:1: # Copyright 2017 The Chromium Authors. All rights reserved. On 2017/03/30 15:31:47, Nico (afk until Tue Apr 4) wrote: > nit: add shebang line, make file executable Done. https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:51: return -1 On 2017/03/30 15:31:47, Nico (afk until Tue Apr 4) wrote: > nit: 1 is a more common "failed" exit code Done. https://codereview.chromium.org/2779663003/diff/40001/chromecast/app/verify_c... chromecast/app/verify_cast_locales.py:52: On 2017/03/30 15:31:47, Nico (afk until Tue Apr 4) wrote: > nit: two blank lines between toplevel things Done.
The CQ bit was checked by slan@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thakis@chromium.org Link to the patchset: https://codereview.chromium.org/2779663003/#ps60001 (title: "Address feedback from thakis@")
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": 60001, "attempt_start_ts": 1490889036361170,
"parent_rev": "a78983cb7f095d0947b7bd59c68bea0e68619306", "commit_rev":
"4f97ba825e075c7660bcb2c4dfc07e2a60d73d48"}
Message was sent while issue was closed.
Description was changed from ========== [Chromecast] Require chromecast OWNERS to be aware of locale changes. Whenever the locale lists declared in //build/config/locales.gni change, someone from the Chromecast team should be added to the CL as an FYI. Enforce this via a python script that checks the locales against a fixed list. BUG=697090 TEST= modify the locales list, make sure the build complains. ========== to ========== [Chromecast] Require chromecast OWNERS to be aware of locale changes. Whenever the locale lists declared in //build/config/locales.gni change, someone from the Chromecast team should be added to the CL as an FYI. Enforce this via a python script that checks the locales against a fixed list. BUG=697090 TEST= modify the locales list, make sure the build complains. Review-Url: https://codereview.chromium.org/2779663003 Cr-Commit-Position: refs/heads/master@{#460803} Committed: https://chromium.googlesource.com/chromium/src/+/4f97ba825e075c7660bcb2c4dfc0... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4f97ba825e075c7660bcb2c4dfc0... |
