1411802 - Build stylo on Android as default

Status: RESOLVED FIXED

(Firefox Build System :: Android Studio and Gradle Integration, enhancement)

Description

[Makoto Kato [:m_kato]]

Fennec team approves this changes, let's build stylo as default.  But preferece is still turned off.

Comment 1

[Makoto Kato [:m_kato]]

increase is 1.6MB (1,616,842 bytes) with the yesterday's mozilla-central.

Before: 36,686,341 bytes
After:  38,303,183 bytes

Comment 2

[Makoto Kato [:m_kato]]

Attachment: Bug 1411802 - Build stylo on Android as default.

- Building is nightly channel only.  Beta and release for Fennec 58 don't build
  stylo.  It means that the package size for 58 beta/release isn't incremented
  by this change.
- The preference for stylo is still turned off Nightly 58. It will be turned on
  59 after fixing some bugs for crashtests and etc.  Our target to enable stylo
  for Android is 59.
- ./mach bootstrap already installs clang etc to build stylo and bindgen.
  Developers for mobile won't require additional build options for this change.

Review commit: https://reviewboard.mozilla.org/r/193370/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/193370/

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198580

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Are there any Stylo on Android tests that will run in CI?  Or is the assumption that testing Stylo on Linux desktop is good enough for Android too?

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198622

I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to take when this finally rides to Release, but I guess that's what we pay for new technology.  Roll on!

::: taskcluster/ci/build/android.yml:28
(Diff revision 1)
>          secrets: true
>          custom-build-variant-cfg: api-16-debug
>          tooltool-downloads: internal
>      toolchains:
>          - android-sdk-linux
> +        - linux64-clang

Does clang provide the llvm backend for Rust?  I'm confused as to why Stylo, which is Rust, would require a _different_ host C/C++ compiler than we actually build with.  (What we actually build with is the Android NDK, as installed by tooltool.)

::: toolkit/moz.configure:578
(Diff revision 1)
> -@depends('--enable-stylo', '--help', target)
> -def stylo_config(value, _, target):
> +@depends('--enable-stylo', '--help', target, milestone)
> +def stylo_config(value, _, target, milestone):
>      build_stylo = None
>      enable_stylo = None
>  
>      # If nothing is specified, default to building and enabling Stylo where it

This comment isn't accurate anymore.  (Assuming that Stylo works on Android.)

::: toolkit/moz.configure:583
(Diff revision 1)
>      # If nothing is specified, default to building and enabling Stylo where it
>      # is known to work.
>      if value.origin == 'default':
>          if target.os == 'Android':
> -            # Stylo on Android is happening Later(tm).
> -            pass
> +            if milestone.is_nightly:
> +                build_stylo = True

Consider adding `enable_stylo = False` for clarity.

Comment 6

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198622

Oh!  Please ensure that this doesn't break artifact builds, on ARM and x86.  Is there any aarch64 implication for this setting?

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198632

::: taskcluster/ci/build/android.yml:28
(Diff revision 1)
>          secrets: true
>          custom-build-variant-cfg: api-16-debug
>          tooltool-downloads: internal
>      toolchains:
>          - android-sdk-linux
> +        - linux64-clang

I think this is about Stylo's use of bindgen to generate Rust wrappers for C++ code.  bindgen uses clang to do this work.

Comment 8

[Nick Alexander :nalexander [he/him]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #7)
> Comment on attachment 8922318 [details]
> Bug 1411802 - Build stylo on Android as default.
> 
> https://reviewboard.mozilla.org/r/193370/#review198632
> 
> ::: taskcluster/ci/build/android.yml:28
> (Diff revision 1)
> >          secrets: true
> >          custom-build-variant-cfg: api-16-debug
> >          tooltool-downloads: internal
> >      toolchains:
> >          - android-sdk-linux
> > +        - linux64-clang
> 
> I think this is about Stylo's use of bindgen to generate Rust wrappers for
> C++ code.  bindgen uses clang to do this work.

OK, thanks.

Comment 9

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8922318 [details]
Bug 1411802 - Build stylo on Android as default.

https://reviewboard.mozilla.org/r/193370/#review198650

Looks good to me.  I assume there's some CI plan one way or another, so no reason to block this patch on it.

Comment 10

[Richard Newman [:rnewman]]

(In reply to Nick Alexander :nalexander from comment #5)

> I think 1.6Mb is a _huge_ hit to
> take when this finally rides to Release, but I guess that's what we pay for
> new technology.  Roll on!

Indeed, presumably some of this is first-mover cost for some Rust dependencies. The more Rust code we ship — and we will ship more! — the more this cost will be amortized over components.

I've always opposed increasing APK sizes, but in this case I am very happy to be aggressive: the more eagerly we pay these costs, the less resistance will apply to all subsequent uses of Rust, and the more we can leverage the technology to make our products better.

Comment 11

[Chris Peterson [:cpeterson]]

(In reply to Nick Alexander :nalexander from comment #5)
> I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to
> take when this finally rides to Release, but I guess that's what we pay for
> new technology.

The APK size increase will be smaller when we actually ship Stylo on Android because we can remove Gecko's old style system code at that time. During the transition to enabling Stylo by default, we must ship both the old and new style system code.

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Richard Newman [:rnewman] from comment #10)
> (In reply to Nick Alexander :nalexander from comment #5)
> 
> > I think 1.6Mb is a _huge_ hit to
> > take when this finally rides to Release, but I guess that's what we pay for
> > new technology.  Roll on!
> 
> Indeed, presumably some of this is first-mover cost for some Rust
> dependencies. The more Rust code we ship — and we will ship more! — the more
> this cost will be amortized over components.
> 
> I've always opposed increasing APK sizes, but in this case I am very happy
> to be aggressive: the more eagerly we pay these costs, the less resistance
> will apply to all subsequent uses of Rust, and the more we can leverage the
> technology to make our products better.

As someone who has died on this hill once before, I also agree that the size increase is warranted here.

Comment 13

[Mike Hommey [:glandium]]

(In reply to Chris Peterson [:cpeterson] from comment #11)
> (In reply to Nick Alexander :nalexander from comment #5)
> > I have questions, but nothing blocking.  I think 1.6Mb is a _huge_ hit to
> > take when this finally rides to Release, but I guess that's what we pay for
> > new technology.
> 
> The APK size increase will be smaller when we actually ship Stylo on Android
> because we can remove Gecko's old style system code at that time. During the
> transition to enabling Stylo by default, we must ship both the old and new
> style system code.

Speaking of which, maybe it's time to have a build option to disable the gecko style system (and check how big the apk is without it)

Comment 14

[Chris Peterson [:cpeterson]]

(In reply to Mike Hommey [:glandium] from comment #13)
> Speaking of which, maybe it's time to have a build option to disable the
> gecko style system (and check how big the apk is without it)

bholley didn't think adding a build option was good use of time because we plan to remove the Gecko style system code soon (in ~59) when we ship Stylo support for Android and chrome/XUL. He estimated that removing the Gecko code might save about 700 KB.

Comment 15

[Makoto Kato [:m_kato]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #4)
> Are there any Stylo on Android tests that will run in CI?  Or is the
> assumption that testing Stylo on Linux desktop is good enough for Android
> too?

Some tests is run on Android only.  So After landing this, I will add tier 3 jobs such as styloVsGecko (bug 1406290 etc)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1c3b82079a5
Build stylo on Android as default. r=nalexander,snorp,jryans
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e4f10cea5e7
Fix bustage after updating NDK r15c. r=me

Comment 17

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/c1c3b82079a5
https://hg.mozilla.org/mozilla-central/rev/1e4f10cea5e7

Comment 18

[Darkspirit]

Fennec Nightly 58 20171030100130 @ Android 7.0 (Moto G5)
Verified: It's possible to enable stylo and stylo-chrome and they work.