Introduce --enable-geckodriver build flag

RESOLVED FIXED in Firefox 55

Status

Testing
Marionette
P1
normal
RESOLVED FIXED
11 months ago
11 months ago

People

(Reporter: ato, Assigned: ato)

Tracking

Version 3
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

11 months ago
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.
(Assignee)

Comment 1

11 months ago
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?
(Assignee)

Comment 2

11 months ago
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)

Updated

11 months ago
Assignee: nobody → ato
Status: NEW → ASSIGNED
(Assignee)

Updated

11 months ago
Priority: -- → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
Blocks: 1368264

Comment 6

11 months ago
mozreview-review
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 7

11 months ago
mozreview-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+
(Assignee)

Comment 8

11 months ago
mozreview-review-reply
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".
(Assignee)

Comment 9

11 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 12

11 months ago
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

Comment 13

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ed5505b795ea
https://hg.mozilla.org/mozilla-central/rev/ba93b8451395
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.