1368035 - Introduce --enable-geckodriver build flag

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

Rust compilation is still very slow and as pointed out in https://bugzilla.mozilla.org/show_bug.cgi?id=1340637#c103, it adds “significant overhead to the build”.

Since geckodriver is only at used by the WPT wdspec tests at the present time, we could get away with an --enable-geckodriver build config flag that is turned on by default in CI, but not in local builds so that developers locally wouldn’t have to compile it unless they need it.

This bug is to introduce said build config flag and turn it on for builds in CI.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Quoting rillian over in https://bugzilla.mozilla.org/show_bug.cgi?id=1340637#c107:

> Maybe you could set the default for --enable-geckodriver based
> on MOZ_AUTOMATION_MOZCONFIG so it's enabled in integration builds,
> but not local developer builds?

Comment 2

[Andreas Tolfsen ❲:ato❳]

I had to temporarily disable compilation of testing/geckodriver in https://bugzilla.mozilla.org/show_bug.cgi?id=1368084 due to https://bugzilla.mozilla.org/show_bug.cgi?id=1367995.

We can presumably re-enable it once we make it opt-in, as the CI builds appear to be fine.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1368035 - Correct description of Marionette in toolkit configure;

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

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1368035 - Enable geckodriver building in automation;

geckodriver compilation was disabled by default in
https://bugzilla.mozilla.org/show_bug.cgi?id=1368084 due to issues
building it locally on Windows.

This re-enables building of geckodriver in automation, but gives
developers an option, --enable-geckodriver, to opt-in to building
it locally.

This means geckodriver will be built in CI by default, but not in
developers' local environments.

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

Comment 5

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=39e5b3bf1c68

Comment 6

[David Burns :automatedtester]

Comment on attachment 8872073 [details]
Bug 1368035 - Correct description of Marionette in toolkit configure;

https://reviewboard.mozilla.org/r/143578/#review147480

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 8872074 [details]
Bug 1368035 - Enable geckodriver building in automation;

https://reviewboard.mozilla.org/r/143580/#review147806

::: build/moz.configure/toolchain.configure:174
(Diff revision 1)
>  set_config('MOZ_USING_COMPILER_WRAPPER', using_compiler_wrapper)
>  
>  
> +# GC rooting and hazard analysis.
> +# ==============================================================
> +option(env='MOZ_HAZARD', help='Build for the GC rooting hazard analysis')

Is this change in this patch for any particular reason?

::: toolkit/moz.configure:1110
(Diff revision 1)
> +    """
> +    At the present time, we want individual developers to be able to
> +    opt-in to building geckodriver locally, but for it to be enabled by
> +    default on supported CI build platforms.
> +
> +    Consequently, --enable-geckodriver is not implied when MOZ_AUTOMATION

This read a little confusingly to me at first. It is implied when `MOZ_AUTOMATION` is set, but we *also* provide the `--enable-geckodriver` option for developers to use.

::: toolkit/moz.configure:1117
(Diff revision 1)
> +    on unsupported platforms.
> +    """
> +    if enable:
> +        return True
> +
> +    linux32 = target.kernel == 'Linux' and target.cpu == 'x86'

You want to stick the whole rest of this function in an `if value.origin == 'default:'` conditional, so that if we change things in the future we'll properly handle `--disable-geckodriver`.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8872074 [details]
Bug 1368035 - Enable geckodriver building in automation;

https://reviewboard.mozilla.org/r/143580/#review147806

> Is this change in this patch for any particular reason?

Just to be able to access it through marking the hazard_analysis function as a dependency to the geckodriver function.  I could drop this change and make it depend on "MOZ_HAZARD" directly, which would have the same effect, but I noticed this was a common pattern for other targets.

> This read a little confusingly to me at first. It is implied when `MOZ_AUTOMATION` is set, but we *also* provide the `--enable-geckodriver` option for developers to use.

Yes, I realise this is confusing.  I’m open to better suggestions.

The future we want to get to, when Rust compilation isn’t slow and the cl.exe linking problems have been resolved, is to compile geckodriver on all platforms unconditionally.

I tried to rephrase it as such:

> --- a/toolkit/moz.configure
> +++ b/toolkit/moz.configure
> @@ -1103,13 +1103,13 @@ option('--enable-geckodriver', help='Enable WebDriver implementation')
>           target)
>  def geckodriver(enable, automation, compile_env, cross_compile, hazard, target):
>      """
> +    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, but for it to be enabled by
> +    opt-in to building geckodriver locally, and for it to be enabled by
>      default on supported CI build platforms.
> -
> -    Consequently, --enable-geckodriver is not implied when MOZ_AUTOMATION
> -    is set because, locally, we want developers to be able to build it
> -    on unsupported platforms.
>      """
>      if enable:
>          return True

> You want to stick the whole rest of this function in an `if value.origin == 'default:'` conditional, so that if we change things in the future we'll properly handle `--disable-geckodriver`.

Thanks for letting me know about value.origin == "default".

Comment 9

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8872074 [details]
Bug 1368035 - Enable geckodriver building in automation;

https://reviewboard.mozilla.org/r/143580/#review147806

> Just to be able to access it through marking the hazard_analysis function as a dependency to the geckodriver function.  I could drop this change and make it depend on "MOZ_HAZARD" directly, which would have the same effect, but I noticed this was a common pattern for other targets.

Reverted this change and it now uses MOZ_HAZARD directly.

Comment 10

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8872073 [details]
Bug 1368035 - Correct description of Marionette in toolkit configure;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143578/diff/1-2/

Comment 11

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8872074 [details]
Bug 1368035 - Enable geckodriver building in automation;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143580/diff/1-2/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ed5505b795ea
Correct description of Marionette in toolkit configure; r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ba93b8451395
Enable geckodriver building in automation; r=ted

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ed5505b795ea
https://hg.mozilla.org/mozilla-central/rev/ba93b8451395