1441656 - Build geckodriver for Android builds

Status: NEW

(Firefox Build System :: General, enhancement)

Description

[Nick Alexander :nalexander [he/him]]

This is similar to Bug 1417646, but not identical.  Investigating --enable-geckodriver for Android, I find that:

1) removing the "bzip2" feature from the "zip" dependency avoids Bug 1409276;

2) adding an Android-specific hack for pthread_atfork allows to build.

I don't know if the result actually runs on a device, but I wanted to capture those notes, and see about upstreaming the latter.

Comment 1

[Nick Alexander :nalexander [he/him]]

Attachment: Bug 1441656 - Build geckodriver for Android builds.

This is a proof-of-concept for building geckodriver during Android
(really, all cross compile) builds.  ato, thoughts?  Would we take the
work-around in upstream geckodriver?

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

Comment 2

[Nick Alexander :nalexander [he/him]]

Try build percolating at https://treeherder.mozilla.org/#/jobs?repo=try&revision=ad1ba1bdda2743a1546ba4e5d7d863576d0096a1

Just as a note, my locally built geckodriver binary is 44Mb.  That seems all kinds of wrong.

Comment 3

[Nick Alexander :nalexander [he/him]]

(In reply to Nick Alexander :nalexander from comment #2)
> Try build percolating at
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=ad1ba1bdda2743a1546ba4e5d7d863576d0096a1
> 
> Just as a note, my locally built geckodriver binary is 44Mb.  That seems all
> kinds of wrong.

  8593480334    85736 -rwxr-xr-x    2 nalexander       staff            43896536 Feb 27 12:55 i686-linux-android/release/geckodriver

Comment 4

[Andreas Tolfsen ❲:ato❳]

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

> Just as a note, my locally built geckodriver binary is 44Mb.  That
> seems all kinds of wrong.

We can probably land this and figure out why the binary size is
bloated later.  I noticed it is approximately the same size for a
Linux build:

> % du -shL obj-x86_64-pc-linux-gnu/dist/bin/
> 44M	obj-x86_64-pc-linux-gnu/dist/bin/geckodriver

As opposed to a "cargo build --release" build:

> % du -sh testing/geckodriver/target/release/geckodriver
> 8.7M	testing/geckodriver/target/release/geckodriver

I’ve filed https://bugzil.la/1442253 to follow up on this.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8954513 [details]
Bug 1441656 - Build geckodriver for Android builds.

https://reviewboard.mozilla.org/r/223568/#review230266

Thanks for figuring out the build problems on Android!  Just split
this up into two commits and add a guard for cross compiled macOS,
and I think this is good to land.

::: testing/geckodriver/Cargo.toml:25
(Diff revision 1)
>  mozversion = { path = "../mozbase/rust/mozversion" }
>  regex = "0.2"
>  rustc-serialize = "0.3"
>  uuid = "0.1.18"
>  webdriver = { path = "../webdriver" }
> -zip = "0.3"
> +zip = { version = "0.3", default-features = false, features = ["deflate"] }

We don’t need bzip2 support, only zip, so this is fine, so thanks
for making this change.

But do you mind making the vendoring changes from "./mach vendor
rust" in a separate commit?  For someone examining the commit it is
nearly impossible to tell what has actually changed.

::: testing/geckodriver/src/main.rs:52
(Diff revision 1)
> +pub extern "C" fn pthread_atfork(_prefork: *mut u8,
> +                                 _postfork_parent: *mut u8,
> +                                 _postfork_child: *mut u8)
> +                                 -> i32 {
> +    0
> +}

I don’t want to pretend I understand what this does, but I think it
is worth putting it in if it makes Android compile.

::: toolkit/moz.configure:1107
(Diff revision 1)
>  @depends('--enable-geckodriver',
>           'MOZ_AUTOMATION',
>           compile_environment,
> -         cross_compiling,
>           hazard_analysis)
> -def geckodriver(enable, automation, compile_env, cross_compile, hazard):
> +def geckodriver(enable, automation, compile_env, hazard):
>      """
>      geckodriver is implied on supported platforms when MOZ_AUTOMATION
>      is set, but we also provide the --enable-geckodriver option for
>      developers to use.
>  
>      At the present time, we want individual developers to be able to
>      opt-in to building geckodriver locally, and for it to be enabled by
>      default on supported CI build platforms.
>      """
>      if enable:
>          if not compile_env:
>              die("--enable-geckodriver is not available without a compile "
>                  "environment. A geckodriver binary will be downloaded during "
>                  "an artifact build by default where available.")
>          return True
>  
>      if enable.origin == 'default':
> -        broken_platforms = cross_compile or hazard
> +        broken_platforms = hazard

This makes the cross-compiled macOS build fail because of a missing
linker:

> error: failed to run dsymutil: No such file or directory (os error 2)

I believe this is bug 1417646.

So to get this passing try you would have to add an explicit check
for macOS cross-compiled builds.

Comment 6

[(not currently active) Ted Mielczarek]

(In reply to Andreas Tolfsen ‹:ato› from comment #5)
> This makes the cross-compiled macOS build fail because of a missing
> linker:
> 
> > error: failed to run dsymutil: No such file or directory (os error 2)
> 
> I believe this is bug 1417646.


That error isn't mentioned in bug 1417646 at all, FYI. That error is because cargo explicitly tries to run `dsymutil` after linking a mac binary.

This came up in https://github.com/rust-lang/rust/issues/47240 and acrichto added a rustc option to not do that in https://github.com/rust-lang/rust/pull/47784 which was merged ~2 weeks ago. We could work around this by adding a symlink from `dsymutil` to `$topsrcdir/clang/bin/llvm-dsymutil` somewhere in $PATH.

Comment 7

[(not currently active) Ted Mielczarek]

With my patches from bug 1409276 I have this building on try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3bccdf25dde25d20f63874bee5f2a5b24229ec66&selectedJob=188427910

Although it still breaks with the pthread_atfork issue on android-x86:
https://treeherder.mozilla.org/logviewer.html#?job_id=188427859&repo=try&lineNumber=30154

It looks like the Rust standard library put in a workaround for this, but it only applies when the target contains "androideabi":
https://github.com/rust-lang/rust/blob/50702b28383c15907c29e8f70256cdd439800834/src/liballoc_jemalloc/build.rs#L142

...so that doesn't quite cover "i686-linux-android". I guess we'll still need that local hack for android-x86.

Comment 8

[(not currently active) Ted Mielczarek]

I'm just going to --disable-geckodriver for android-x86, I filed bug 1477340 about it.

Comment 9

[(not currently active) Ted Mielczarek]

Attachment: bug 1441656 - disable geckodriver for android-x86 and macos-cross-ccov.

android-x86 builds can't build geckodriver currently: https://bugzilla.mozilla.org/show_bug.cgi?id=1477340
ccov builds have other issues: https://bugzilla.mozilla.org/show_bug.cgi?id=1477305

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

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8995012 [details]
bug 1441656 - disable geckodriver for android-x86 and macos-cross-ccov.

ted: mozreview gives me “Service unavailable”, but I looked at the
patches and consider this an r+.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8995012 [details]
bug 1441656 - disable geckodriver for android-x86 and macos-cross-ccov.

https://reviewboard.mozilla.org/r/259508/#review266520

Up again now.  Setting r+ here as well.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please note that geckodriver can now be build via cross-compilation for MacOS (bug 1417646). Building for Android is the only remaining platform now.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Ted, I assume you don't work on it. So I will mark the bug as unassigned.

Comment 14

[(not currently active) Ted Mielczarek]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Ted, I assume you don't work on it. So I will mark the bug as unassigned.

I'm not planning on it, no, but I believe this patch could land as-is now that the changes in bug 1409276 landed. However, my patch in bug 1417646 originally enabled geckodriver on Android by default, and I think that got changed before landing, so we'd probably need to fix that.