Create build target for libaddressinput android widget

Create build target for libaddressinput android widget

BUG=399146
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291169

[Evan Stade, 2014-08-14 21:58:47]

newt and kkimlabs, building this new target causes an exception:

"""
Exception:
/usr/local/google/clank/src/third_party/libaddressinput/src/java/res/layout/address_textview.xml
should use android:layout_marginStart instead of android:layout_marginLeft
For background, see:
http://android-developers.blogspot.com/2013/03/native-rtl-support-in-android-...
If you have a legitimate need for this attribute, discuss with
kkimlabs@chromium.org or newt@chromium.org
"""

My legitimate need is that this is a third party library. How do you recommend I
proceed?

[newt (away), 2014-08-14 23:08:10]

On 2014/08/14 21:58:47, Evan Stade wrote:
> newt and kkimlabs, building this new target causes an exception:
> 
> """
> Exception:
>
/usr/local/google/clank/src/third_party/libaddressinput/src/java/res/layout/address_textview.xml
> should use android:layout_marginStart instead of android:layout_marginLeft
> For background, see:
>
http://android-developers.blogspot.com/2013/03/native-rtl-support-in-android-...
> If you have a legitimate need for this attribute, discuss with
> mailto:kkimlabs@chromium.org or mailto:newt@chromium.org
> """
> 
> My legitimate need is that this is a third party library. How do you recommend
I
> proceed?

If you can modify the library, I'd just replace marginLeft with marginStart,
which is needed to properly support RTL languages. marginLeft is (almost?)
always a mistake, which is why we enabled this check.

Either way, for third party code, you can pass a flag that disables this check:
add a gyp variable 'res_v14_verify_only': 1 when you're including java.gypi.
Example here:
https://chromium.googlesource.com/android_tools/+/master/android_tools.gyp

[Kibeom Kim (inactive), 2014-08-14 23:44:38]

On 2014/08/14 23:08:10, newt wrote:
> On 2014/08/14 21:58:47, Evan Stade wrote:
> > newt and kkimlabs, building this new target causes an exception:
> > 
> > """
> > Exception:
> >
>
/usr/local/google/clank/src/third_party/libaddressinput/src/java/res/layout/address_textview.xml
> > should use android:layout_marginStart instead of android:layout_marginLeft
> > For background, see:
> >
>
http://android-developers.blogspot.com/2013/03/native-rtl-support-in-android-...
> > If you have a legitimate need for this attribute, discuss with
> > mailto:kkimlabs@chromium.org or mailto:newt@chromium.org
> > """
> > 
> > My legitimate need is that this is a third party library. How do you
recommend
> I
> > proceed?
> 
> If you can modify the library, I'd just replace marginLeft with marginStart,
> which is needed to properly support RTL languages. marginLeft is (almost?)
> always a mistake, which is why we enabled this check.
> 
> Either way, for third party code, you can pass a flag that disables this
check:
> add a gyp variable 'res_v14_verify_only': 1 when you're including java.gypi.
> Example here:
> https://chromium.googlesource.com/android_tools/+/master/android_tools.gyp

Context: android:*Left & android:*Right attributes can cause crashes on certain
Samsung devices, b/8351339 b/16661111 . So if that layout resource is actually
used, we must replace them to Start & End.

IIRC, res_v14_verify_only flag still checks it and raises an error, it just
doesn't do behind-the-scene v14 resource generation, i.e., replacing all the
Start&End attributes to Left&Right for pre-v17 devices.

[Kibeom Kim (inactive), 2014-08-15 00:03:50]

> Context: android:*Left & android:*Right attributes can cause crashes on
certain
> Samsung devices, b/8351339 b/16661111 . So if that layout resource is actually
> used, we must replace them to Start & End.
> 

My mistake, correction:

Context: android:*Start & android:*End attributes can cause crashes on certain
Samsung devices, b/8351339 b/16661111 . So if that layout resource is actually
used, we must replace them to Left & Right.

But using Start&End is an encouraged practice, and that's the reason we have the
script to convert Start&End attributes to Left&Right for old devices. Forcing
Start&End in the script was just a bonus, so if Left&Right attributes are really
a desired effect, we can consider adding a way to express exceptions in the
script, but currently the correct way is using android:marginStart instead and
not using res_v14_verify_only flag, unless the issue is already handled by
providing two layout versions manually.

[Evan Stade, 2014-08-15 15:38:17]

If marginStart causes a crash on older devices, wouldn't modifying the
library break other clients of the library (that aren't running the
Start->Left script)?


-- Evan Stade


On Thu, Aug 14, 2014 at 5:03 PM, <kkimlabs@chromium.org> wrote:

> Context: android:*Left & android:*Right attributes can cause crashes on
>>
> certain
>
>> Samsung devices, b/8351339 <https://buganizer.corp.google.com/8351339>
>> b/16661111 <https://buganizer.corp.google.com/16661111> . So if that
>> layout resource is actually
>> used, we must replace them to Start & End.
>>
>
>
> My mistake, correction:
>
> Context: android:*Start & android:*End attributes can cause crashes on
> certain
>
> Samsung devices, b/8351339 <https://buganizer.corp.google.com/8351339>
> b/16661111 <https://buganizer.corp.google.com/16661111> . So if that
> layout resource is actually
> used, we must replace them to Left & Right.
>
> But using Start&End is an encouraged practice, and that's the reason we
> have the
> script to convert Start&End attributes to Left&Right for old devices.
> Forcing
> Start&End in the script was just a bonus, so if Left&Right attributes are
> really
> a desired effect, we can consider adding a way to express exceptions in the
> script, but currently the correct way is using android:marginStart instead
> and
> not using res_v14_verify_only flag, unless the issue is already handled by
> providing two layout versions manually.
>
> https://codereview.chromium.org/461323005/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Evan Stade, 2014-08-15 17:56:00]

Actually I think for now we'll hold off on this change. May revisit it later
though.

[Kibeom Kim (inactive), 2014-08-15 18:04:15]

On 2014/08/15 15:38:17, Evan Stade wrote:
> If marginStart causes a crash on older devices, wouldn't modifying the
> library break other clients of the library (that aren't running the
> Start->Left script)?
> 
> 
> -- Evan Stade
> 

Yes you're right. Then, the library should provide two layouts, e.g. layout/
layout-v17/ , or two styles. Android is doing the latter. And we need to use
res_v14_verify_only gyp flag anyways. I'm OK with using res_v14_verify_only flag
and landing the current one, since we're tracking the issue.

[Evan Stade, 2014-08-19 18:45:34]

alright, I take it back, we do want this change. Use of res_v14_verify_only
lgty?

[newt (away), 2014-08-19 18:55:10]

res_v14_verify_only lgtm

[Evan Stade, 2014-08-19 18:56:23]

+rouslan for review

[Kibeom Kim (inactive), 2014-08-19 18:58:53]

lgtm

Also res_v14_verify_only lgtm. I'll track marginLeft issue here separately.

[please use gerrit instead, 2014-08-19 19:06:34]

lgtm

[Evan Stade, 2014-08-19 19:13:57]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-19 19:14:12]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/461323005/20001

[Evan Stade, 2014-08-19 20:24:09]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-19 20:24:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/461323005/40001

[commit-bot: I haz the power, 2014-08-20 02:26:04]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[commit-bot: I haz the power, 2014-08-20 02:33:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 02:33:49]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Evan Stade, 2014-08-20 19:48:40]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-20 19:50:05]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/461323005/40001

[commit-bot: I haz the power, 2014-08-20 20:02:23]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Evan Stade, 2014-08-20 20:04:52]

+torne, OWNER of android_webview. Any idea why android_aosp is complaining
thusly:

gyp:
/mnt/scratch0/b_used/build/slave/android_aosp/build/android-src/external/chromium_org/third_party/libaddressinput/src/cpp/libaddressinput.gypi
not found (cwd:
/mnt/scratch0/b_used/build/slave/android_aosp/build/android-src/external/chromium_org)
while reading includes of
/mnt/scratch0/b_used/build/slave/android_aosp/build/android-src/external/chromium_org/third_party/libaddressinput/libaddressinput.gyp

[commit-bot: I haz the power, 2014-08-20 20:12:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-08-20 20:12:05]

Try jobs failed on following builders:
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)

[Evan Stade, 2014-08-20 21:44:03]

-torne, +sgurun for better timezone alignment. Selim, any idea why android_aosp
can't find this file?

[sgurun-gerrit only, 2014-08-20 22:06:23]

On 2014/08/20 21:44:03, Evan Stade wrote:
> -torne, +sgurun for better timezone alignment. Selim, any idea why
android_aosp
> can't find this file?


There were some issues earlier about libadddressinput, let me look at this and
see if I can figure out.

[sgurun-gerrit only, 2014-08-20 22:09:38]

On 2014/08/20 22:06:23, sgurun wrote:
> On 2014/08/20 21:44:03, Evan Stade wrote:
> > -torne, +sgurun for better timezone alignment. Selim, any idea why
> android_aosp
> > can't find this file?
> 
> 
> There were some issues earlier about libadddressinput, let me look at this and
> see if I can figure out.

also pulling in Bo since he might be more familiar with the AOSP bot.

[boliu, 2014-08-20 22:20:48]

On 2014/08/20 22:09:38, sgurun wrote:
> On 2014/08/20 22:06:23, sgurun wrote:
> > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > -torne, +sgurun for better timezone alignment. Selim, any idea why
> > android_aosp
> > > can't find this file?
> > 
> > 
> > There were some issues earlier about libadddressinput, let me look at this
and
> > see if I can figure out.
> 
> also pulling in Bo since he might be more familiar with the AOSP bot.

Hmm, this is source in a third party directory
(third_party/libaddressinput/src), but the gyp file is in a *different* third
party dir (third_party/libaddressinput). If they were the same, then I would
tell you to simply add it to self._compile_but_not_snapshot_dependencies in
android_webview/buildbot/deps_whitelist.py.

I'm not sure what should happen when they are different. I think it's a bug in
the bot that it's dropping files from the src/ repo. I suggest try adding them
both? Probably will break downstream stuff, but that's problems for android
webview people, not chromium.

And sorry, and thanks for not ignoring the failure.

[Evan Stade, 2014-08-20 22:24:22]

On 2014/08/20 22:20:48, boliu wrote:
> On 2014/08/20 22:09:38, sgurun wrote:
> > On 2014/08/20 22:06:23, sgurun wrote:
> > > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > > -torne, +sgurun for better timezone alignment. Selim, any idea why
> > > android_aosp
> > > > can't find this file?
> > > 
> > > 
> > > There were some issues earlier about libadddressinput, let me look at this
> and
> > > see if I can figure out.
> > 
> > also pulling in Bo since he might be more familiar with the AOSP bot.
> 
> Hmm, this is source in a third party directory
> (third_party/libaddressinput/src), but the gyp file is in a *different* third
> party dir (third_party/libaddressinput). If they were the same, then I would
> tell you to simply add it to self._compile_but_not_snapshot_dependencies in
> android_webview/buildbot/deps_whitelist.py.
> 
> I'm not sure what should happen when they are different. I think it's a bug in
> the bot that it's dropping files from the src/ repo. I suggest try adding them
> both? 

Sorry, don't follow --- adding both what to what?

> Probably will break downstream stuff, but that's problems for android
> webview people, not chromium.
> 
> And sorry, and thanks for not ignoring the failure.

[boliu, 2014-08-20 22:27:45]

On 2014/08/20 22:24:22, Evan Stade wrote:
> On 2014/08/20 22:20:48, boliu wrote:
> > On 2014/08/20 22:09:38, sgurun wrote:
> > > On 2014/08/20 22:06:23, sgurun wrote:
> > > > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > > > -torne, +sgurun for better timezone alignment. Selim, any idea why
> > > > android_aosp
> > > > > can't find this file?
> > > > 
> > > > 
> > > > There were some issues earlier about libadddressinput, let me look at
this
> > and
> > > > see if I can figure out.
> > > 
> > > also pulling in Bo since he might be more familiar with the AOSP bot.
> > 
> > Hmm, this is source in a third party directory
> > (third_party/libaddressinput/src), but the gyp file is in a *different*
third
> > party dir (third_party/libaddressinput). If they were the same, then I would
> > tell you to simply add it to self._compile_but_not_snapshot_dependencies in
> > android_webview/buildbot/deps_whitelist.py.
> > 
> > I'm not sure what should happen when they are different. I think it's a bug
in
> > the bot that it's dropping files from the src/ repo. I suggest try adding
them
> > both? 
> 
> Sorry, don't follow --- adding both what to what?

In android_webview/buildbot/deps_whitelist.py, delete
'third_party/libaddressinput/src/cpp' and add 'third_party/libaddressinput' and
'third_party/libaddressinput/src'.

> 
> > Probably will break downstream stuff, but that's problems for android
> > webview people, not chromium.
> > 
> > And sorry, and thanks for not ignoring the failure.

[boliu, 2014-08-20 22:36:39]

On 2014/08/20 22:27:45, boliu wrote:
> On 2014/08/20 22:24:22, Evan Stade wrote:
> > On 2014/08/20 22:20:48, boliu wrote:
> > > On 2014/08/20 22:09:38, sgurun wrote:
> > > > On 2014/08/20 22:06:23, sgurun wrote:
> > > > > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > > > > -torne, +sgurun for better timezone alignment. Selim, any idea why
> > > > > android_aosp
> > > > > > can't find this file?
> > > > > 
> > > > > 
> > > > > There were some issues earlier about libadddressinput, let me look at
> this
> > > and
> > > > > see if I can figure out.
> > > > 
> > > > also pulling in Bo since he might be more familiar with the AOSP bot.
> > > 
> > > Hmm, this is source in a third party directory
> > > (third_party/libaddressinput/src), but the gyp file is in a *different*
> third
> > > party dir (third_party/libaddressinput). If they were the same, then I
would
> > > tell you to simply add it to self._compile_but_not_snapshot_dependencies
in
> > > android_webview/buildbot/deps_whitelist.py.
> > > 
> > > I'm not sure what should happen when they are different. I think it's a
bug
> in
> > > the bot that it's dropping files from the src/ repo. I suggest try adding
> them
> > > both? 
> > 
> > Sorry, don't follow --- adding both what to what?
> 
> In android_webview/buildbot/deps_whitelist.py, delete
> 'third_party/libaddressinput/src/cpp' and add 'third_party/libaddressinput'
and
> 'third_party/libaddressinput/src'.

Err, I misread the bot error. I think you only need to drop the "cpp" bit at the
end (ie no need to add "third_party/libaddressinput" like I suggested before)

> 
> > 
> > > Probably will break downstream stuff, but that's problems for android
> > > webview people, not chromium.
> > > 
> > > And sorry, and thanks for not ignoring the failure.

[Evan Stade, 2014-08-20 22:42:15]

On 2014/08/20 22:36:39, boliu wrote:
> On 2014/08/20 22:27:45, boliu wrote:
> > On 2014/08/20 22:24:22, Evan Stade wrote:
> > > On 2014/08/20 22:20:48, boliu wrote:
> > > > On 2014/08/20 22:09:38, sgurun wrote:
> > > > > On 2014/08/20 22:06:23, sgurun wrote:
> > > > > > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > > > > > -torne, +sgurun for better timezone alignment. Selim, any idea why
> > > > > > android_aosp
> > > > > > > can't find this file?
> > > > > > 
> > > > > > 
> > > > > > There were some issues earlier about libadddressinput, let me look
at
> > this
> > > > and
> > > > > > see if I can figure out.
> > > > > 
> > > > > also pulling in Bo since he might be more familiar with the AOSP bot.
> > > > 
> > > > Hmm, this is source in a third party directory
> > > > (third_party/libaddressinput/src), but the gyp file is in a *different*
> > third
> > > > party dir (third_party/libaddressinput). If they were the same, then I
> would
> > > > tell you to simply add it to self._compile_but_not_snapshot_dependencies
> in
> > > > android_webview/buildbot/deps_whitelist.py.
> > > > 
> > > > I'm not sure what should happen when they are different. I think it's a
> bug
> > in
> > > > the bot that it's dropping files from the src/ repo. I suggest try
adding
> > them
> > > > both? 
> > > 
> > > Sorry, don't follow --- adding both what to what?
> > 
> > In android_webview/buildbot/deps_whitelist.py, delete
> > 'third_party/libaddressinput/src/cpp' and add 'third_party/libaddressinput'
> and
> > 'third_party/libaddressinput/src'.
> 
> Err, I misread the bot error. I think you only need to drop the "cpp" bit at
the
> end (ie no need to add "third_party/libaddressinput" like I suggested before)

thanks, I'll give it a shot.

> 
> > 
> > > 
> > > > Probably will break downstream stuff, but that's problems for android
> > > > webview people, not chromium.
> > > > 
> > > > And sorry, and thanks for not ignoring the failure.

[Torne, 2014-08-21 11:03:38]

On 2014/08/20 22:42:15, Evan Stade wrote:
> On 2014/08/20 22:36:39, boliu wrote:
> > On 2014/08/20 22:27:45, boliu wrote:
> > > On 2014/08/20 22:24:22, Evan Stade wrote:
> > > > On 2014/08/20 22:20:48, boliu wrote:
> > > > > On 2014/08/20 22:09:38, sgurun wrote:
> > > > > > On 2014/08/20 22:06:23, sgurun wrote:
> > > > > > > On 2014/08/20 21:44:03, Evan Stade wrote:
> > > > > > > > -torne, +sgurun for better timezone alignment. Selim, any idea
why
> > > > > > > android_aosp
> > > > > > > > can't find this file?
> > > > > > > 
> > > > > > > 
> > > > > > > There were some issues earlier about libadddressinput, let me look
> at
> > > this
> > > > > and
> > > > > > > see if I can figure out.
> > > > > > 
> > > > > > also pulling in Bo since he might be more familiar with the AOSP
bot.
> > > > > 
> > > > > Hmm, this is source in a third party directory
> > > > > (third_party/libaddressinput/src), but the gyp file is in a
*different*
> > > third
> > > > > party dir (third_party/libaddressinput). If they were the same, then I
> > would
> > > > > tell you to simply add it to
self._compile_but_not_snapshot_dependencies
> > in
> > > > > android_webview/buildbot/deps_whitelist.py.
> > > > > 
> > > > > I'm not sure what should happen when they are different. I think it's
a
> > bug
> > > in
> > > > > the bot that it's dropping files from the src/ repo. I suggest try
> adding
> > > them
> > > > > both? 
> > > > 
> > > > Sorry, don't follow --- adding both what to what?
> > > 
> > > In android_webview/buildbot/deps_whitelist.py, delete
> > > 'third_party/libaddressinput/src/cpp' and add
'third_party/libaddressinput'
> > and
> > > 'third_party/libaddressinput/src'.
> > 
> > Err, I misread the bot error. I think you only need to drop the "cpp" bit at
> the
> > end (ie no need to add "third_party/libaddressinput" like I suggested
before)
> 
> thanks, I'll give it a shot.
> 
> > 
> > > 
> > > > 
> > > > > Probably will break downstream stuff, but that's problems for android
> > > > > webview people, not chromium.
> > > > > 
> > > > > And sorry, and thanks for not ignoring the failure.

android_webview LGTM with this change.

This is weird because the android_aosp bot uses the SVN deps, which are what
you're updating here to switch from two separate subdir checkouts to one
parent-dir checkout, but the downstream bot uses the git deps, which already
just use the combined parent dir checkout (see .DEPS.git). So there shouldn't be
a downstream breakage since we already have the whole of
third_party/libaddressinput/src available there.

[Evan Stade, 2014-08-21 17:21:01]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2014-08-21 17:23:14]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/estade@chromium.org/461323005/60001

[commit-bot: I haz the power, 2014-08-21 20:35:36]

Committed patchset #4 (60001) as 291169