Closed Bug 1368035 Opened 2 years ago Closed 2 years ago

Introduce --enable-geckodriver build flag

Categories

(Testing :: Marionette, enhancement, P1)

Version 3
enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(2 files)

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.
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?
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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1368264
Comment on attachment 8872073 [details]
Bug 1368035 - Correct description of Marionette in toolkit configure;

https://reviewboard.mozilla.org/r/143578/#review147480
Attachment #8872073 - Flags: review?(dburns) → review+
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`.
Attachment #8872074 - Flags: review?(ted) → review+
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 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.
Pushed by atolfsen@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/ed5505b795ea
https://hg.mozilla.org/mozilla-central/rev/ba93b8451395
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.