1471281 - Include geckodriver in default build

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

When we migrated geckodriver from GitHub into central it was
originally enabled by default in all local builds.  Since this was
one of the very first Rust components to land in central, support
for Rust was a little bit experimental and we discovered a couple
of different problems:

  (1) Not all developers had Rust installed, and support for Rust
      in the tree was experimental.  This caused some compile errors
      in particular on Windows, and at the time developers were not
      happy with requiring Rust for building Firefox.

  (2) The additional build time induced by including a fairly large
      Rust component in the default build was considered too high.

At the time of writing this, Rust support in central has improved
vastly.  We also require Rust for regular Firefox builds, which
addresses part of issue 1.

My working theory is that we now build so much Rust code in central
that many of the dependencies geckodriver relies on and that takes
up a lot of time to build will have been built as dependencies for
other Rust components in the tree, effectively making the additional
time it takes to build geckodriver less prominent than it was when
we originally tried including it in the default build.

This bug will track the work towards including geckodriver in the
default build when you do "./mach build" locally.  We currently
have ac_add_options --enable-geckodriver set for all CI/try builds,
which means that we—at least in theory—should not have any additional
problems enabling it in local builds as well.

Comment 1

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

(In reply to Andreas Tolfsen 「:ato」 from comment #0)
> My working theory is that we now build so much Rust code in central
> that many of the dependencies geckodriver relies on and that takes
> up a lot of time to build will have been built as dependencies for
> other Rust components in the tree, effectively making the additional
> time it takes to build geckodriver less prominent than it was when
> we originally tried including it in the default build.

You could have a look at some build logs to prove that.

Comment 2

[Gregory Szorc [:gps]]

We have "plain" build variations in CI that measure how long it takes to build without caches and using default settings. This is essentially a proxy for local developer builds.

So you can validate your claim that geckodriver won't add much overhead by doing a Try push with the following tasks:

build-linux64-plain/opt
build-linux64-plain/debug
build-win64-plain/opt
build-win64-plain/debug

And then compare the Perfherder build metrics against the base revision. e.g. what https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1682289,1,2&series=autoland,1682265,1,2&series=autoland,1691945,1,2&series=autoland,1682223,1,2&series=autoland,1682281,1,2&series=autoland,1697287,1,2 is rendering.

Comment 3

[Andreas Tolfsen ❲:ato❳]

Some local testing I did showed that enabling geckodriver did not
have any noteworthy impact on build time:

Without geckodriver: 15:37.97 (938r, 399.50s)
With geckodriver:    15:44.74 (945r, 397.83s)

I will verify against the plain builds gps mentioned too.

Comment 4

[Andreas Tolfsen ❲:ato❳]

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #3)

> I will verify against the plain builds gps mentioned too.

Of course we can’t do that, since geckodriver is built with
MOZ_AUTOMATION (on try) but just not locally.

Comment 5

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory

Comment 6

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9003421 [details] [diff] [review]
Make building geckodriver mandatory

I understand artifact builds now fetch a geckodriver binary from
Treeherder, so if I change the default to be to build geckodriver,
how can I make this not fail for those who have --enable-artifact-builds
set in their mozconfig?

Comment 7

[Chris Manchester (limited bugmail, email directly)]

(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #6)
> Comment on attachment 9003421 [details] [diff] [review]
> Make building geckodriver mandatory
> 
> I understand artifact builds now fetch a geckodriver binary from
> Treeherder, so if I change the default to be to build geckodriver,
> how can I make this not fail for those who have --enable-artifact-builds
> set in their mozconfig?

This shouldn't be an issue, we don't build any rust code locally in an artifact build anyway.

Comment 8

[Andreas Tolfsen ❲:ato❳]

Let me clarify.  If I set ac_add_options --enable-artifact-builds
and apply the attached patched to central, I get the following
configure error:

>  0:03.62 Reticulating splines...
>  0:04.34  0:00.74 File already read. Skipping: /home/ato/src/gecko/gfx/angle/targets/angle_common/moz.build
>  0:07.68 Traceback (most recent call last):
>  0:07.68   File "/home/ato/src/gecko/configure.py", line 123, in <module>
>  0:07.68     sys.exit(main(sys.argv))
>  0:07.68   File "/home/ato/src/gecko/configure.py", line 34, in main
>  0:07.68     return config_status(config)
>  0:07.68   File "/home/ato/src/gecko/configure.py", line 118, in config_status
>  0:07.68     return config_status(args=[], **encode(sanitized_config, encoding))
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/config_status.py", line 146, in config_status
>  0:07.68     the_backend.consume(definitions)
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/backend/base.py", line 126, in consume
>  0:07.68     for obj in objs:
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 185, in emit
>  0:07.68     objs = list(emitfn(out))
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 1184, in emit_from_context
>  0:07.68     for obj in self._handle_linkables(context, passthru, generated_files):
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 620, in _handle_linkables
>  0:07.68     self._binaries[program] = cls(context, program, cargo_file)
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/data.py", line 582, in __init__
>  0:07.68     cargo_dir = cargo_output_directory(context, self.TARGET_SUBST_VAR)
>  0:07.68   File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/data.py", line 563, in cargo_output_directory
>  0:07.68     return mozpath.join(context.config.substs[target_var], rust_build_kind)
> 0:07.68 KeyError: u'RUST_TARGET'

This is because I’ve changed the default of the
"geckodriver" rule to build it by default, except for some
unsupported build configurations.  You added a compile_env check in
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/moz.configure#924-926,
which as far as I can see is the only compile_env check in this
configure script.  Do we still need this?

Comment 9

[Chris Manchester (limited bugmail, email directly)]

Right, by removing that check your patch gets us back to the error that motivated it in bug 1370535. We can either hide the option when the compile environment isn't available, as we do with several others (I believe "--disable-gtest-in-build" is an example), or we can dodge the access to that configure variable in the manner we did in https://hg.mozilla.org/mozilla-central/rev/d0fccf681c20 .

Comment 10

[Andreas Tolfsen ❲:ato❳]

Thanks, your comment was pretty useful.  I found this in the same
toolkit/moz.configure file:

> option('--disable-gtest-in-build',
>        help='Force disable building the gtest libxul during the build.',
>        when='--enable-compile-environment')

So the option function takes an optional keyword argument ‘when’
one can use to guard, or “hide”, an option with.

Comment 11

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory.

Enable building of geckodriver by default where we have a compile
environment available.  This makes --enable-geckodriver unavailable
to artifact builds.

geckodriver remains disabled by default on cross compile builds and
hazard builds, with Android pointed out specifically (although it
is cross compiled).

Comment 12

[Andreas Tolfsen ❲:ato❳]

Attachment: Update geckodriver build instructions.

Comment 13

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory.

Enable building of geckodriver by default where we have a compile
environment available.  This makes --enable-geckodriver unavailable
to artifact builds.

Following this change:

  * --enable-geckodriver is implied in supported build configurations,
    but may be used in unsupported build configurations (Android,
    cross compiled, and hazard builds) to force geckodriver to be built.

  * --disable-geckodriver causes geckodriver not to be built.

  * In artifact build mode, a geckodriver binary artifact will
    continue to be downloaded, but it will not be possible to
    specify --enable-geckodriver without a compile environment.

geckodriver remains disabled by default on cross compile builds
and hazard builds, pointing out Android specifically (although it
is cross compiled).

Comment 14

[Andreas Tolfsen ❲:ato❳]

Attachment: Update geckodriver build instructions.

Comment 15

[Andreas Tolfsen ❲:ato❳]

Depending on the length of the review process, I’m likely going to
hold off landing any of the patches until hiro-san’s work on
https://bugzilla.mozilla.org/show_bug.cgi?id=1417646 is completed.

Comment 16

[Andreas Tolfsen ❲:ato❳]

I confirmed through two try runs that the attached patches work as
intended:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd38f8cf1fb070591ccb4d063c36e3d3fa3654aa
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1b997a6d3e807875da27123ae571d5e59689ac

But for them to land on central we first need to land hiro-san’s
patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1417646,
and an extra patch to remove the explicit --enable-geckodriver on
linux32 that glandium added in
https://bugzilla.mozilla.org/show_bug.cgi?id=1486652 along with the
changeset in this bug.

In total this will make geckodriver work in cross-compile environments
(except Android) and enable geckodriver by default, which will
render the --enable-geckodriver flag unnecessary in
build/unix/mozconfig.linux32, and this in turn will make artifact
builds happy since the flag isn’t available to them.

Comment 17

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory.

Enable building of geckodriver by default where we have a compile
environment available.  This makes --enable-geckodriver unavailable
to artifact builds.

Following this change:

  * --enable-geckodriver is implied in supported build configurations,
    but may be used in unsupported build configurations (Android,
    cross compiled, and hazard builds) to force geckodriver to be built.

  * --disable-geckodriver causes geckodriver not to be built.

  * In artifact build mode, a geckodriver binary artifact will
    continue to be downloaded, but it will not be possible to
    specify --enable-geckodriver without a compile environment.

geckodriver remains disabled by default on cross compile builds
and hazard builds, pointing out Android specifically (although it
is cross compiled).

Comment 18

[Andreas Tolfsen ❲:ato❳]

Attachment: Update geckodriver build instructions.

Comment 19

[Andreas Tolfsen ❲:ato❳]

Attachment: Remove --enable-geckodriver override for linux32.

Now that geckodriver is enabled by default in all builds and
works in cross-compile environments, we can drop the explicit
--enable-geckodriver flag from the linux32 build config since it is
no longer inhibited geckodriver not working in cross-compile environments.

Comment 20

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 9005649 [details] [diff] [review]
Make building geckodriver mandatory.

I don't think I'm going to be able to look at this before PTO, forwarding to the shared queue.

Comment 21

[Nathan Froyd [:froydnj]]

Comment on attachment 9005649 [details] [diff] [review]
Make building geckodriver mandatory.

Review of attachment 9005649 [details] [diff] [review]:
-----------------------------------------------------------------

Canceling review, but not because of anything drastically wrong.

::: toolkit/moz.configure
@@ +914,4 @@
>  
> +option('--enable-geckodriver', default=geckodriver_default,
> +       when='--enable-compile-environment',
> +       help='Override default and attempt to build geckodriver')

I think this should just be "Build geckodriver for test purposes."  Also, the default should probably check --disable-tests, because we wouldn't be using geckodriver in that configuration?

Comment 22

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

Comment on attachment 9005650 [details] [diff] [review]
Update geckodriver build instructions.

Review of attachment 9005650 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/geckodriver/CONTRIBUTING.md
@@ +112,5 @@
>  [WebDriver protocol].  geckodriver translates WebDriver [commands],
>  [responses], and [errors] to the [Marionette protocol], and acts
>  as a proxy between WebDriver clients and [Marionette].
>  
> +To build geckodriver from [mozilla-central]:

You reference `mozilla-unified` here, which means that you have to add at least a `hg up central` line before building geckodriver. Otherwise you end-up with whatever is on tip which can be anything of central, beta, release, or an esr branch, right?

@@ +131,5 @@
> +the binary will be put in the `$(topsrcdir)/target` directory.
> +
> +You can run your freshly built geckodriver this way:
> +
> +	% ./mach geckodriver -- --other --flags

This would even work with artifact builds? That would be great. Also is there a way to specify debug vs. release builds? I assume not, and the appropriate geckodriver build it picked which is similar to Firefox (opt vs. debug).

Also can you please refer to Building.md here to avoid all these duplication of content?

@@ +146,5 @@
>  
>  
>  Running the tests
>  -----------------
>  

Please reference `Testing.md` here so that we can avoid all the code duplication.

::: testing/geckodriver/doc/Testing.md
@@ +18,5 @@
> +
> +	% ./mach test testing/geckodriver
> +
> +The webdriver crate tests are unfortunately not yet runnable through mach.
> +Work to make this possible is tracked in [[https://bugzil.la/1424369]].

This is marked as WFM, and I also fixed bug 1473341 which should include all the tests, even for mozbase rust. So this can be removed.

@@ +46,5 @@
> +As you get in to development of geckodriver and Marionette you will
> +increasingly grow to understand our love for [trace-level logs].
> +They provide us with the input—the HTTP requests—from the client
> +(in WPT’s case from the tests’ use of a custom WebDriver client),
> +the translation geckodriver makes to the [Marionette protocol],

`[Marionette protocol]` is not defined.

@@ +50,5 @@
> +the translation geckodriver makes to the [Marionette protocol],
> +the log output from Marionette, its responses back to geckodriver,
> +and finally the output—or the HTTP response—back to the client.
> +
> +The [trace-level logs] can be surfaced by passing on the `-vv`

`[trace-level logs]` is not defined.

Comment 23

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

Comment on attachment 9005649 [details] [diff] [review]
Make building geckodriver mandatory.

Review of attachment 9005649 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not a build nor toolkit peer to be able to review that.

Comment 24

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #23)

> I'm not a build nor toolkit peer to be able to review that.

But I still need sign-off from a Marionette peer, since this is
very much impacting Marionette.  How do I get that?

Comment 25

[Andreas Tolfsen ❲:ato❳]

(In reply to Nathan Froyd [:froydnj] from comment #21)

> ::: toolkit/moz.configure
> @@ +914,4 @@
> >  
> > +option('--enable-geckodriver', default=geckodriver_default,
> > +       when='--enable-compile-environment',
> > +       help='Override default and attempt to build geckodriver')
> 
> I think this should just be "Build geckodriver for test purposes."  Also,
> the default should probably check --disable-tests, because we wouldn't be
> using geckodriver in that configuration?

We eventually want to release geckodriver from Taskcluster.
Specifically, the same build that is being used for Firefox Nightly
because this has the signing infrastructure available we will also
need for the geckodriver binaries.

My worry was that making it dependent on --disable-tests might
accidentally cause it not to be available in these builds.  But I
checked around about this, and I believe Nightly builds are now
made off the normal mozilla-central configuration, so this should
be a safe change to make.

Comment 26

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #22)

> ::: testing/geckodriver/CONTRIBUTING.md
> @@ +112,5 @@
> >  [WebDriver protocol].  geckodriver translates WebDriver [commands],
> >  [responses], and [errors] to the [Marionette protocol], and acts
> >  as a proxy between WebDriver clients and [Marionette].
> >  
> > +To build geckodriver from [mozilla-central]:
> 
> You reference `mozilla-unified` here, which means that you have to add at
> least a `hg up central` line before building geckodriver. Otherwise you
> end-up with whatever is on tip which can be anything of central, beta,
> release, or an esr branch, right?

I don’t know.  I understand the canonical repository for Firefox
is https://hg.mozilla.org/mozilla-unified/.

Do we want to have VCS specific instructions here?  AIUI the
documentation for that is
https://mozilla-version-control-tools.readthedocs.io/en/latest/.

> @@ +131,5 @@
> > +the binary will be put in the `$(topsrcdir)/target` directory.
> > +
> > +You can run your freshly built geckodriver this way:
> > +
> > +	% ./mach geckodriver -- --other --flags
> 
> This would even work with artifact builds? That would be great. Also is
> there a way to specify debug vs. release builds? I assume not, and the
> appropriate geckodriver build it picked which is similar to Firefox (opt vs.
> debug).

I don’t use artifact builds so I don’t know if they are opt or
debug.  I assume it isn’t possible to pick between the two, but
chmanchester would know better than me.

The line itself should work with artifact builds for as long as
they fetch the build artifact, since the only assumption made in
https://searchfox.org/mozilla-central/source/testing/geckodriver/mach_commands.py
is that the binary exists.

> Also can you please refer to Building.md here to avoid all these duplication
> of content?

CONTRIBUTING.md is valuable for as long as the project is exported
to GitHub, because GitHub picks up this file name convention in
various interesting places.  Building.md is not exported is only
available on central.

Do you mean we should remove the content from CONTRIBUTING.md and
link to firefox-source-docs.m.o instead?

Comment 27

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory.

Enable building of geckodriver by default where we have a compile
environment available.  This makes --enable-geckodriver unavailable
to artifact builds.

Following this change:

  * --enable-geckodriver is implied in supported build configurations,
    but may be used in unsupported build configurations (Android,
    cross compiled, and hazard builds) to force geckodriver to be built.

  * --disable-geckodriver causes geckodriver not to be built.

  * In artifact build mode, a geckodriver binary artifact will
    continue to be downloaded, but it will not be possible to
    specify --enable-geckodriver without a compile environment.

geckodriver remains disabled by default on cross compile builds
and hazard builds, pointing out Android specifically (although it
is cross compiled).

Comment 28

[Andreas Tolfsen ❲:ato❳]

Attachment: Make building geckodriver mandatory.

Updated the commit message slightly to also include:

>   * --disable-tests will imply not building geckodriver, but can
>     be overridden using --enable-geckodriver as indicated above.

Comment 29

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #24)
> But I still need sign-off from a Marionette peer, since this is
> very much impacting Marionette.  How do I get that?

I could just have a look at it once bug 1417646 has been fixed.

(In reply to Andreas Tolfsen ❲:ato❳ from comment #26)
> > > +To build geckodriver from [mozilla-central]:
> > 
> > You reference `mozilla-unified` here, which means that you have to add at
> > least a `hg up central` line before building geckodriver. Otherwise you
> > end-up with whatever is on tip which can be anything of central, beta,
> > release, or an esr branch, right?
> 
> I don’t know.  I understand the canonical repository for Firefox
> is https://hg.mozilla.org/mozilla-unified/.

No, mozilla-unified is a collection from all available repositories. The canonical repository patches should be based on is still mozilla-central, and that's where landed commits are getting merged to from mozilla-inbound and autoland.

> Do we want to have VCS specific instructions here?  AIUI the
> documentation for that is
> https://mozilla-version-control-tools.readthedocs.io/en/latest/.

In that case it's always better to refer to external documentation. So lets better do that, yes.

> > Also can you please refer to Building.md here to avoid all these duplication
> > of content?
> 
> CONTRIBUTING.md is valuable for as long as the project is exported
> to GitHub, because GitHub picks up this file name convention in
> various interesting places.  Building.md is not exported is only
> available on central.
> 
> Do you mean we should remove the content from CONTRIBUTING.md and
> link to firefox-source-docs.m.o instead?

For maintenance it would indeed be better. I wouldn't expect to have to update contributing.md because of changes in Building.md or Testing.md. Also note that we do not regularly update those files, and if it causes extra churn. Given that the canonical repository for geckodriver is mozill-central now, a link to firefox-source-docs.m.o should be perfect.

Comment 30

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #29)

> (In reply to Andreas Tolfsen ❲:ato❳ from comment #24)
> 
> > But I still need sign-off from a Marionette peer, since this is
> > very much impacting Marionette.  How do I get that?
> 
> I could just have a look at it once bug 1417646 has been fixed.

Alright, I’ll re-request r? once that lands and this has been
rebased.  Thanks.

> (In reply to Andreas Tolfsen ❲:ato❳ from comment #26)
> 
> > > > +To build geckodriver from [mozilla-central]:
> > > 
> > > You reference `mozilla-unified` here, which means that you
> > > have to add at least a `hg up central` line before building
> > > geckodriver. Otherwise you end-up with whatever is on tip
> > > which can be anything of central, beta, release, or an esr
> > > branch, right?
> > 
> > I don’t know.  I understand the canonical repository for Firefox
> > is https://hg.mozilla.org/mozilla-unified/.
> 
> No, mozilla-unified is a collection from all available
> repositories. The canonical repository patches should be based
> on is still mozilla-central, and that's where landed commits are
> getting merged to from mozilla-inbound and autoland.

It was my impression that the VCS team wanted developers to use
unified for precisely the reason that you don’t need two or three
different repositories checked out, largely duplicating eachother’s
commits.  Isn’t this worth recommending to new developers?

Since it seems to be contentious to make this documentation change,
I will revert it for the time being to get this patch accepted.

> > Do you mean we should remove the content from CONTRIBUTING.md
> > and link to firefox-source-docs.m.o instead?
>
> For maintenance it would indeed be better. I wouldn't expect to
> have to update contributing.md because of changes in Building.md
> or Testing.md. Also note that we do not regularly update those
> files, and if it causes extra churn. Given that the canonical
> repository for geckodriver is mozill-central now, a link to
> firefox-source-docs.m.o should be perfect.

We can make that change.

I feel that it is a bit out of context to significantly change
CONTRIBUTING.md as part of a changeset to build geckodriver in all
build configs.  I would prefer if the updates to this file were
minimal, reflecting only what needs to change in order to land that
change, and then follow up on de-duplicating content later.

Comment 31

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #30)
> > > I don’t know.  I understand the canonical repository for Firefox
> > > is https://hg.mozilla.org/mozilla-unified/.
> > 
> > No, mozilla-unified is a collection from all available
> > repositories. The canonical repository patches should be based
> > on is still mozilla-central, and that's where landed commits are
> > getting merged to from mozilla-inbound and autoland.
> 
> It was my impression that the VCS team wanted developers to use
> unified for precisely the reason that you don’t need two or three
> different repositories checked out, largely duplicating eachother’s
> commits.  Isn’t this worth recommending to new developers?

Yes, we can or even better should refer to mozilla-unified, but after the checkout you have to make sure to update the the central branch. That's all.

> I feel that it is a bit out of context to significantly change
> CONTRIBUTING.md as part of a changeset to build geckodriver in all
> build configs.  I would prefer if the updates to this file were
> minimal, reflecting only what needs to change in order to land that
> change, and then follow up on de-duplicating content later.

Do whatever is better.

Comment 32

[Nathan Froyd [:froydnj]]

Comment on attachment 9005984 [details] [diff] [review]
Make building geckodriver mandatory.

Review of attachment 9005984 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: toolkit/moz.configure
@@ +917,4 @@
>  
> +option('--enable-geckodriver', default=geckodriver_default,
> +       when='--enable-compile-environment',
> +       help='Override default and attempt to build geckodriver')

I still think this should just be "Build geckodriver", without mentioning anything about overriding defaults; it seems self-explanatory to me that the option overrides "defaults".

Comment 33

[Andreas Tolfsen ❲:ato❳]

Attachment: Update geckodriver build instructions.

Comment 34

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #31)

> (In reply to Andreas Tolfsen ❲:ato❳ from comment #30)
> 
> > I feel that it is a bit out of context to significantly change
> > CONTRIBUTING.md as part of a changeset to build geckodriver in
> > all build configs.  I would prefer if the updates to this file
> > were minimal, reflecting only what needs to change in order to
> > land that change, and then follow up on de-duplicating content
> > later.
> 
> Do whatever is better.

Yes, so what I will do is the reverse: I’ve filed
https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 to redirect
people’s attention to firefox-source-docs.m.o and will fix that
_before_ this lands, since this changeset is anyway waiting on

Comment 35

[Andreas Tolfsen ❲:ato❳]

(In reply to Nathan Froyd [:froydnj] from comment #32)

> ::: toolkit/moz.configure
> @@ +917,4 @@
> >  
> > +option('--enable-geckodriver', default=geckodriver_default,
> > +       when='--enable-compile-environment',
> > +       help='Override default and attempt to build geckodriver')
> 
> I still think this should just be "Build geckodriver", without mentioning
> anything about overriding defaults; it seems self-explanatory to me that the
> option overrides "defaults".

Oh I see what you mean.  That makes sense.  I will fix this up
before pushing.  This changeset needs rebasing on top of
https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 regardless.

Comment 36

[Andreas Tolfsen ❲:ato❳]

(In reply to Andreas Tolfsen ❲:ato❳ from comment #34)

> […] since this changeset is anyway waiting on

https://bugzilla.mozilla.org/show_bug.cgi?id=1417646

(In reply to Andreas Tolfsen ❲:ato❳ from comment #35)

> This changeset needs rebasing on top of
> https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 regardless.

I really meant https://bugzilla.mozilla.org/show_bug.cgi?id=1417646
here as well.

Comment 37

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

Comment on attachment 9006273 [details] [diff] [review]
Update geckodriver build instructions.

Review of attachment 9006273 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/geckodriver/doc/Building.md
@@ +12,2 @@
>  
> +	% ./mach build testing/geckodriver

You should still add `hg up central` before. Otherwise it will be built from whatever branch is on tip.

@@ +43,2 @@
>  [mozconfig]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options
>  [Firefox CI]: https://treeherder.mozilla.org/

I don't see that this is needed anymore. Maybe others too?

::: testing/geckodriver/doc/Testing.md
@@ +49,5 @@
> +
> +	% ./mach wpt --webdriver-arg=-vv testing/web-platform/tests/webdriver
> +
> +[Web Platform Tests]: http://web-platform-tests.org/
> +[cargo]: http://doc.crates.io/guide.html

I don't see it being used in this file.

Comment 38

[Andreas Tolfsen ❲:ato❳]

(In reply to Henrik Skupin (:whimboo) from comment #37)

> ::: testing/geckodriver/doc/Building.md
> @@ +12,2 @@
> >  
> > +	% ./mach build testing/geckodriver
> 
> You should still add `hg up central` before. Otherwise it will be
> built from whatever branch is on tip.

I thought we agreed not to have VCS specific information in this
file?  This documentation is kind of assuming that people know how
to build the right thing; it is not meant for teaching people how
to check out the right branches.

Would it be better if I removed the reference to mozilla-central
above?  That way the docs would not make any assumption about from
where you’re building.

Comment 39

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 9005651 [details] [diff] [review]
Remove --enable-geckodriver override for linux32.

Rendered obsolete by https://hg.mozilla.org/mozilla-central/rev/90096b1bb22a.

Comment 40

[Andreas Tolfsen ❲:ato❳]

Attachment: Update geckodriver build instructions.

Comment 41

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99c19ab72a9236be16a5bdbed3f3f90bd59fb0b7

Comment 42

[Andreas Tolfsen ❲:ato❳]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4fc3a8d2c9b61d905610f2918415ee935f1f2c

Comment 43

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

(In reply to Andreas Tolfsen ❲:ato❳ from comment #38)
> I thought we agreed not to have VCS specific information in this
> file?  This documentation is kind of assuming that people know how
> to build the right thing; it is not meant for teaching people how
> to check out the right branches.

Sorry, looks like that I forgot it.

> Would it be better if I removed the reference to mozilla-central
> above?  That way the docs would not make any assumption about from
> where you’re building.

Maybe do that and just reference the documentation which is about cloning the repository.

Comment 44

[Andreas Tolfsen ❲:ato❳]

Third time lucky try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=091fc13c9938781934aa1032a96e2595ae0dcf2c

Comment 45

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ee11194f69
build: make asan depend on --help; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3a859a6f0720
toolkit: make building geckodriver mandatory; r=froydnj
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6c97dfb586
geckodriver: update geckodriver build instructions; r=whimboo

Comment 46

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/c8ee11194f69
https://hg.mozilla.org/mozilla-central/rev/3a859a6f0720
https://hg.mozilla.org/mozilla-central/rev/3b6c97dfb586