|
|
Created:
4 years, 10 months ago by Sergey Ulanov Modified:
3 years, 5 months ago Reviewers:
Nico CC:
chromium-reviews, Jamie Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkip missing translations when generating JSON resources.
It doesn't make sense to replace missing translations with english
strings because Chrome can do this automatically when getting
resources.
BUG=369572
Review-Url: https://codereview.chromium.org/1676793002
Cr-Original-Original-Commit-Position: refs/heads/master@{#480994}
Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66bda710f601c0
Review-Url: https://codereview.chromium.org/1676793002
Cr-Original-Commit-Position: refs/heads/master@{#482826}
Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae0b60c45bd9fd
Review-Url: https://codereview.chromium.org/1676793002
Cr-Commit-Position: refs/heads/master@{#483512}
Committed: https://chromium.googlesource.com/chromium/src/+/af0dc05d501cafad6ce25a775ada2559cabd12f0
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : fix fake-bidi support #
Messages
Total messages: 46 (20 generated)
sergeyu@chromium.org changed reviewers: + thakis@chromium.org
thakis: ping
I don't understand this change. Chrome could do this at runtime, but we want to be able to fail the build on missing strings for official builds, else we won't detect missing strings. Doesn't this change make that impossible?
On 2016/02/23 21:44:50, Nico wrote: > I don't understand this change. Chrome could do this at runtime, but we want to > be able to fail the build on missing strings for official builds, else we won't > detect missing strings. Doesn't this change make that impossible? We have a script that's ran as part of the release process for the CRD app. It verifies the JSON files generated by grit to make sure all strings are translated for all languages. It's not part of the build process. I don't think we ever want to fail grit during build time, even for official builds. E.g. it's acceptable to have beta builds with some translations missing.
thakis: ping
On 2016/02/24 18:08:03, Sergey Ulanov wrote: > On 2016/02/23 21:44:50, Nico wrote: > > I don't understand this change. Chrome could do this at runtime, but we want > to > > be able to fail the build on missing strings for official builds, else we > won't > > detect missing strings. Doesn't this change make that impossible? > > We have a script that's ran as part of the release process for the CRD app. It > verifies the JSON files generated by grit to make sure all strings are > translated for all languages. It's not part of the build process. Do all other things that use json output have scripts like this too? Maybe you could put this behavior behind an opt-in flag? > I don't think > we ever want to fail grit during build time, even for official builds. E.g. it's > acceptable to have beta builds with some translations missing.
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
On 2017/05/24 13:48:54, Nico wrote: > Do all other things that use json output have scripts like this too? I don't think so, but all places where json generator is used set fallback_to_english="true", see https://codesearch.chromium.org/search/?q=type%3D%22chrome_messages_json%22+f... . When fallback_to_english flag is set grit doesn't generate any errors for missing strings. > > Maybe you could put this behavior behind an opt-in flag? I've updated this change to keep the old behavior when fallback_to_english=false. It also simplified this change a bit.
thakis: ping!
I have this paged out again, but iirc the state of the discussion was: - there's more than 1 client of this script - this diagnostic is in the way of one client - it's useful for the other clients Is that correct? If so, just removing it seems bad for all but the 1 client.
On 2017/06/15 21:18:13, Nico wrote: > I have this paged out again, but iirc the state of the discussion was: > > - there's more than 1 client of this script > - this diagnostic is in the way of one client > - it's useful for the other clients The last item is incorrect. It makes no difference for other clients because they all set fallback_to_english="true". > > Is that correct? If so, just removing it seems bad for all but the 1 client.
On 2017/06/15 21:36:56, Sergey Ulanov wrote: > On 2017/06/15 21:18:13, Nico wrote: > > I have this paged out again, but iirc the state of the discussion was: > > > > - there's more than 1 client of this script > > - this diagnostic is in the way of one client > > - it's useful for the other clients > > The last item is incorrect. It makes no difference for other clients because > they all set fallback_to_english="true". And also if there were clients that didn't set fallback_to_english="true" then this change would not affect them either, as I'm not changing the current behavior for fallback_to_english="false" (in the last version of this change). > > > > > Is that correct? If so, just removing it seems bad for all but the 1 client.
To clarify it further: 1. Nothing changes when for clients that set fallback_to_english = false. 2. When fallback_to_english=true, currently the fallback is handled by grd. It doesn't report any errors for missing translations in that case. 3. After this change the fallback to english will be handled by chrome instead of by grit when generating .json resources. Currently all grd files that use JSON generator set fallback_to_english="true", i.e. grit will never report missing translations for these files. This change makes it possible to implement this check outside of grit.
thakis: ping
pingly ping
lgtm, thanks for explaining. Sorry about the long holdup.
The CQ bit was checked by sergeyu@chromium.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": 60001, "attempt_start_ts": 1497995514663000, "parent_rev": "d98c08f75e5c26f49a9e05e9d0946a2f6d29d3f0", "commit_rev": "d2f53be161b01a4cf04dfa387b66bda710f601c0"}
Message was sent while issue was closed.
Description was changed from ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 ========== to ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66...
Message was sent while issue was closed.
On 2017/06/20 at 22:30:46, commit-bot wrote: > Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Could this be causing this build failure? https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.win%2FWin_Build... FAILED: gen/remoting/host/win/remoting_host_messages.mc C:/b/depot_tools/python276_bin/python.exe ../../remoting/tools/build/remoting_localize.py --locale_dir gen/remoting/webapp/_locales --template ../../remoting/host/win/host_messages.mc.jinja2 --output gen/remoting/host/win/remoting_host_messages.mc --encoding utf-16 am ar bg bn ca cs da de el en en-GB es et fa fake-bidi fi fil fr gu he hi hr hu id it ja kn ko lt lv ml mr ms nb nl pl pt-PT ro ru sk sl sr sv sw ta te th tr uk vi zh-CN zh-TW pt-BR es-419 Traceback (most recent call last): File "../../remoting/tools/build/remoting_localize.py", line 794, in <module> sys.exit(DoMain(sys.argv[1:])) File "../../remoting/tools/build/remoting_localize.py", line 791, in DoMain return Localize(options.template, locales, options) File "../../remoting/tools/build/remoting_localize.py", line 731, in Localize WriteIfChanged(options.output, template.render(context), options.encoding) File "C:\b\c\b\win\src\third_party\jinja2\environment.py", line 989, in render return self.environment.handle_exception(exc_info, True) File "C:\b\c\b\win\src\third_party\jinja2\environment.py", line 754, in handle_exception reraise(exc_type, exc_value, tb) File "../../remoting/host/win\host_messages.mc.jinja2", line 26, in top-level template code {% trans %}HOST_CATEGORY{% endtrans %} File "../../remoting/tools/build/remoting_localize.py", line 642, in <lambda> return lambda message: self.GetText(message) File "../../remoting/tools/build/remoting_localize.py", line 630, in GetText return self.message_map[self.language][message] KeyError: u'HOST_CATEGORY'
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/2948893002/ by findit-for-me@appspot.gserviceaccount.com. The reason for reverting is: Findit (https://goo.gl/kROfz5) identified CL at revision 480994 as the culprit for failures in the build cycles as shown on: https://findit-for-me.appspot.com/waterfall/culprit?key=ag9zfmZpbmRpdC1mb3Itb....
Message was sent while issue was closed.
Description was changed from ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... ========== to ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... ==========
Fixed the problem that broke build on windows in https://chromium-review.googlesource.com/c/549835/ . Relanding
The CQ bit was checked by sergeyu@chromium.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": 60001, "attempt_start_ts": 1498610261039710, "parent_rev": "d113d16354bdf66f1471a131cac316e4ddb68e4d", "commit_rev": "095dd0be63a91436a1e4ea8b9da502d03d208349"}
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1498610261039710, "parent_rev": "e2856777652263b14611173aea65024db6a27919", "commit_rev": "1deaebd297b7212932be4afe34ae0b60c45bd9fd"}
Message was sent while issue was closed.
Description was changed from ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... ========== to ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#482826} Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae...
Message was sent while issue was closed.
On 2017/06/28 at 00:53:05, commit-bot wrote: > Committed patchset #2 (id:60001) as https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... I think this may be causing the compile to fail again: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_B...
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:60001) has been created in https://codereview.chromium.org/2960853003/ by sergeyu@chromium.org. The reason for reverting is: Broke windows build: https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.linux%2FLinux_B....
Message was sent while issue was closed.
Description was changed from ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#482826} Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... ========== to ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#482826} Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... ==========
The CQ bit was checked by sergeyu@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: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by sergeyu@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/1676793002/#ps80001 (title: "fix fake-bidi support")
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": 80001, "attempt_start_ts": 1498769232051220, "parent_rev": "b394d18f263bb63178bcfee2e248f96d0c593f42", "commit_rev": "af0dc05d501cafad6ce25a775ada2559cabd12f0"}
Message was sent while issue was closed.
Description was changed from ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#482826} Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... ========== to ========== Skip missing translations when generating JSON resources. It doesn't make sense to replace missing translations with english strings because Chrome can do this automatically when getting resources. BUG=369572 Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Original-Commit-Position: refs/heads/master@{#480994} Committed: https://chromium.googlesource.com/chromium/src/+/d2f53be161b01a4cf04dfa387b66... Review-Url: https://codereview.chromium.org/1676793002 Cr-Original-Commit-Position: refs/heads/master@{#482826} Committed: https://chromium.googlesource.com/chromium/src/+/1deaebd297b7212932be4afe34ae... Review-Url: https://codereview.chromium.org/1676793002 Cr-Commit-Position: refs/heads/master@{#483512} Committed: https://chromium.googlesource.com/chromium/src/+/af0dc05d501cafad6ce25a775ada... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:80001) as https://chromium.googlesource.com/chromium/src/+/af0dc05d501cafad6ce25a775ada... |