Enable pseudolocales on non-official builds [chromium/src : master]
Matt Stark (Gerrit)
Attention is currently required from: Jochen Eisinger.
1 comment:
Patchset:
This is a reland of the previous commit you were approving (there were merge conflicts, so I couldn't just click the reland button).
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Jochen Eisinger (Gerrit)
Attention is currently required from: Matt Stark.
1 comment:
Patchset:
can you please ask folks from the chrome language team to review this?
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Trevor Perrier (Gerrit)
Attention is currently required from: Matt Stark.
6 comments:
Patchset:
This CL is really doing two things:
1. Rename locales_with_fake_bidi to locales_with_pseudolocales.
2. Enable pseudolocales.Most of the files and edits are for the rename rather than enabling pseudolocales. I think it would be a lot cleaner to split this into two CLs. The refactor should be really easy to get an LGTM for.
For the enable pseudolocales CL you should get input from Chromecast owners on how they want to handle the pseudolocales. I think ignoring them in chromecast.gni will be what they want - but we should get an LGTM from them on that too.
File build/config/locales.gni:
Patch Set #1, Line 16:
# - |ios_packed_locales_as_mac_outputs| subset for iOS outputnit: maybe add |locales_with_pseudolocales| to the end of this list.
pseudolocales = [
"ar-XB",
"en-XA",
]
locales_and_pseudolocales = locales + pseudolocalesThe vast majority of edits for this simple refactor which should be safe to do. Can this be split out into a second CL that just renames locales_with_fake_bidi to locales_and_pseudolocales (I think this refactor is really good - it just makes the CL to "Enable pseudolocales on non-official builds" much longer and edit way more files then it needs to).
Patch Set #1, Line 248:
locales += pseudolocalesSo do we know that everywhere |locales| is used to filter pak files the grit build command to make the pack files used |locales_and_pseudolocales|. Based on your testing it seems like this works on Linux and Windows - there weren't any issues with ChromeOS or Android builds?
Something that would really help with this is to rename locales to something like all_platform_locales so that it's easier to find in code search. locales is just too generic of a variable name for something so important.
File chromecast/app/resources/chromecast_settings.grd:
Patch Set #1, Line 127:
allow_pseudo="false"Just to clarify this is saying that even though the pseudolocale pak files are generated they shouldn't be used? Would it be better to introduce a |locales_without_pseudolocales| variable in locales.gni that is guaranteed to never have the pseudolocales even on dev builds?
File chromecast/app/verify_cast_locales.py:
Patch Set #1, Line 26:
Chromecast OWNERS need to know if the list of locales used inWe should check with Chromecast owners what they want to do. I suspect an easier fix for this would be to either have a |locales_without_pseudolocales| variable or subtract out the pseudolocales here[1].
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Matt Stark (Gerrit)
Attention is currently required from: Trevor Perrier.
2 comments:
File build/config/locales.gni:
pseudolocales = [
"ar-XB",
"en-XA",
]
locales_and_pseudolocales = locales + pseudolocales
The vast majority of edits for this simple refactor which should be safe to do. […]
That's a good idea, I'll probably go do that and send it out for review next week.
Patch Set #1, Line 248:
locales += pseudolocales
So do we know that everywhere |locales| is used to filter pak files the grit build command to make t […]
That is part of my concern - this is my first chrome change, and so I'm not actually sure how to test it properly. For now, I've just wrote target_os=[...], and confirmed that I was able to run autoninja -C out/<platform> for each of them. Is that sufficient? I had assumed that CQ would cover all of this, but it didn't seem to.
WRT renaming locales to all_platform locales, a few notes:
- If you look at remoting/android/BUILD.gn, you'll actually note that I managed to break the build by simply importing this file, and had to rename the variable so it didn't try and use this one instead.
- However, because of that very problem, the refactor is very difficult - not all references to locales are actually references to this locales variable.
- Note that all_platform_locales seems like a confusing name. It's more like current_platform_locales, if I'm understanding the code correctly.
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Trevor Perrier (Gerrit)
Attention is currently required from: Matt Stark.
2 comments:
File build/config/locales.gni:
pseudolocales = [
"ar-XB",
"en-XA",
]
locales_and_pseudolocales = locales + pseudolocales
That's a good idea, I'll probably go do that and send it out for review next week.
ACK thanks - sorry for making this more work, but I think that will simplify landing this.
Patch Set #1, Line 248:
locales += pseudolocales
That is part of my concern - this is my first chrome change, and so I'm not actually sure how to tes […]
Re: CQ coverage - yeah, I had similar issues when I was landing the new locales for Android. My understanding is that CQ doesn't build *all* the possible builds/platform configurations - the fact that you've tested them all on local builds should catch some of that. Almost every CL I did would get reverted at least once. There are also some downstream files that use |locales| (at least for Android - not sure if ChromeOS has those too).
I think that's another reason splitting this CL makes sense. You're isolating just the part that might need to get reverted.
Re: rename locales - Yes, that's a better name. I tried to do this last year (here's a very old WIP CL[1]) and used |platform_pak_locales|. But you're right this is a really hard refactor since not all uses of locales mean this variable. I need to work on other things and never got the downstream CLs ready to land this.
However, using that CL as a guide for where |locales| is used I think you've covered all the Android uses I was worried about.
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Matt Stark (Gerrit)
Attention is currently required from: Trevor Perrier.
5 comments:
File build/config/locales.gni:
Patch Set #1, Line 16:
# - |ios_packed_locales_as_mac_outputs| subset for iOS output
nit: maybe add |locales_with_pseudolocales| to the end of this list.
Done
pseudolocales = [
"ar-XB",
"en-XA",
]
locales_and_pseudolocales = locales + pseudolocales
ACK thanks - sorry for making this more work, but I think that will simplify landing this.
Done
Patch Set #1, Line 248:
locales += pseudolocales
Re: CQ coverage - yeah, I had similar issues when I was landing the new locales for Android. […]
Ack
File chromecast/app/resources/chromecast_settings.grd:
Patch Set #1, Line 127:
allow_pseudo="false"
Just to clarify this is saying that even though the pseudolocale pak files are generated they should […]
Upon further investigation, I found https://chromium-review.googlesource.com/c/chromium/src/+/488166/
Based on that, I've simply disabled pseudolocales for chromecast.
File chromecast/app/verify_cast_locales.py:
Patch Set #1, Line 26:
Chromecast OWNERS need to know if the list of locales used in
We should check with Chromecast owners what they want to do. […]
Done
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Trevor Perrier (Gerrit)
Attention is currently required from: Matt Stark.
Patch set 3:Code-Review +1
1 comment:
Patchset:
Great, thanks for the update. This is much easier to reason about now. LGTM from a locales perspective.
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Matt Stark (Gerrit)
Attention is currently required from: Jochen Eisinger.
Patch set 3:Commit-Queue +1
Jochen Eisinger (Gerrit)
Patchset:
lgtm
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Matt Stark (Gerrit)
Patch set 3:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Failed builds:
luci.chromium.try/chromium_presubmit JOB_FAILED https://cr-buildbucket.appspot.com/build/8852649035812426176
#### There are 1 error(s), 1 warning(s), and 0 notifications(s). Here are the errors:
**ERROR**
```
Missing LGTM from an OWNER for these files:
remoting/resources/remoting_strings.grd
```
#### To see notifications and warnings, look at the stdout of the presubmit step.
luci.chromium.try/chromium_presubmit JOB_FAILED https://cr-buildbucket.appspot.com/build/8852648841684430592
#### There are 1 error(s), 1 warning(s), and 0 notifications(s). Here are the errors:
**ERROR**
```
Missing LGTM from an OWNER for these files:
remoting/resources/remoting_strings.grd
```
#### To see notifications and warnings, look at the stdout of the presubmit step.
Gary Kacmarcik (Gerrit)
Attention is currently required from: Matt Stark.
Patch set 3:Code-Review +1
Matt Stark (Gerrit)
Patch set 3:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Failed builds:
luci.chromium.try/chromium_presubmit JOB_FAILED https://cr-buildbucket.appspot.com/build/8852559784661240528
#### There are 1 error(s), 1 warning(s), and 0 notifications(s). Here are the errors:
**ERROR**
```
Missing LGTM from an OWNER for these files:
chromecast/chromecast.gni
```
#### To see notifications and warnings, look at the stdout of the presubmit step.
luci.chromium.try/chromium_presubmit JOB_FAILED https://cr-buildbucket.appspot.com/build/8852559615027733840
#### There are 1 error(s), 1 warning(s), and 0 notifications(s). Here are the errors:
**ERROR**
```
Missing LGTM from an OWNER for these files:
chromecast/chromecast.gni
```
#### To see notifications and warnings, look at the stdout of the presubmit step.
Chromium LUCI CQ (Gerrit)
CQ is trying the patch.
Note: The patchset #4 "Enable pseudolocales on non-official builds" sent to CQ was uploaded after this CL was CR+1-ed.
Reviewer, please verify there is nothing unexpected https://chromium-review.googlesource.com/c/2726015/4
Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/chromium-review.googlesource.com/2726015/4
Bot data: {"action": "start", "triggered_at": "2021-03-18T04:54:08.0Z", "revision": "f6d6089d23cb49c2822e99452646c08f568c248a"}
Matt Stark (Gerrit)
Patch set 4:Commit-Queue +2
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Enable pseudolocales on non-official builds
BUG=chromium:1173812
TEST=Built chrome, this time built a windows version as well, confirmed that pseudolocales work as expected.
Change-Id: I419272154fe25f7df9998b211f874047f27996fc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2726015
Commit-Queue: Matt Stark <ms...@google.com>
Reviewed-by: Trevor Perrier <per...@chromium.org>
Reviewed-by: Jochen Eisinger <joc...@chromium.org>
Reviewed-by: Gary Kacmarcik <gar...@chromium.org>
Cr-Commit-Position: refs/heads/master@{#864130}
---
M build/config/locales.gni
M chrome/android/expectations/chrome_modern_public_bundle.arm64.libs_and_assets.expected
M chrome/android/expectations/monochrome_public_bundle.arm64.libs_and_assets.expected
M chrome/android/expectations/trichrome_chrome_bundle.arm64.libs_and_assets.expected
M remoting/android/BUILD.gn
M remoting/host/win/BUILD.gn
M remoting/remoting_locales.gni
M remoting/resources/remoting_strings.grd
M remoting/tools/build/remoting_localize.gni
9 files changed, 47 insertions(+), 36 deletions(-)
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Findit (Gerrit)
Findit has created a revert of this change.
To view, visit change 2726015. To unsubscribe, or for help writing mail filters, visit settings.
Owen Min (Gerrit)
Owen Min has created a revert of this change.