Andreas Tolfsen ❲:ato❳ Assignee
	Description • 7 years ago

Through recent efforts, it is now possible to build Rust programs in mozilla-central.  We should take advantage of this by importing geckodriver so that we do not have to upload binaries to tooltool in order to run the WPT WebDriver (Wd) tests.

This would have many advantages, chiefly that it would be possible to make changes to geckodriver at the same time as making changes to the Marionette server.  Building geckodriver on the build slaves would also mean access to more platforms and ability to run the Wd tests on other platforms than Linux x86_64.

It will also mean we have to jump through some hoops to synchronise work happening in the GitHub repository (https://github.com/mozilla/geckodriver) and in central (I would suggest testing/geckodriver), but I feel the benefits of assembling the whole stack when building Firefox outweighs the downsides.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 7 years ago

Due to https://bugzilla.mozilla.org/show_bug.cgi?id=1329700, the build system in mozilla-central is unable to deduce what a crate's binary is.  We can work around that by defining a [[bin]] section
in geckodriver's Cargo.toml.

Due to https://bugzilla.mozilla.org/show_bug.cgi?id=1302704, crate dependencies are calculated individually in mozilla-central.  Having a single workspace spanning all of m-c, cargo would generate a single Cargo.lock so the versions would be matched.

Until this is in place, a stop gap solution is to modify geckodriver's Cargo.lock file manually when importing it to m-c.  This effort is made slightly easier by making it depend specifically on clap 2.20.3, since this is already vendored in third_party/rust.

There are more issues, such as that webdriver depends on backtrace-sys, which depends on a version of the gcc crate that isn't available.  We could make webdriver depend on a very specific version, but it is easier to work around this issue when doing an import of geckodriver.

I have submitted https://github.com/mozilla/geckodriver/pull/478 to make future imports of geckodriver easier.  I have also submitted https://reviewboard.mozilla.org/r/113504/ to try to see if it works in practice on the build slaves.  With the combination of the changes described, I’m able to successfully build geckodriver on my local Linux machine as part of the `./mach build` process.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 8 • 7 years ago

With the botched geckodriver Cargo.lock file, the try results look promising: https://treeherder.mozilla.org/#/jobs?repo=try&revision=491436812186

Much to my astonishment, every platform apart from tier-2 AB (Artifact Build) and macOS appears to compile geckodriver successfully.  Doing most builds on native platforms, I suspect spared us some of the unpleasantness involved in getting it to compile on Travis using cross compilation and other special tricks.

The AB failure looks trivial to fix, but I should file a bug blocking https://bugzil.la/1293253.  I have not yet looked at the macOS build failure in detail, but I suspect we need to crank up the verbosity in cargo to get meaningful results.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 9 • 7 years ago

Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1341041 about the build failing on tier-2 artifact builds on try.  Seems there is an invalid key lookup from a dict in mozbuild somewhere, and fortunately easy to reproduce.

As can be told from the try run (https://treeherder.mozilla.org/#/jobs?repo=try&revision=491436812186&selectedJob=78438890), the build is also failing on macOS, although with a more cryptic error message:

[task 2017-02-17T20:40:52.908295Z] 20:40:52     INFO -    = note: "cc" "-m64" "-L" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/deps/geckodriver-7a279504e4466bda.0.o" "-o" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/deps/geckodriver-7a279504e4466bda" "-Wl,-dead_strip" "-nodefaultlibs" "-L" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/deps" "-L" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./release/deps" "-L" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/build/bzip2-sys-621c8de5b1862fa4/out" "-L" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/build/miniz-sys-d7cfa77cf66f89a9/out" "-L" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmozrunner-2d6aa295fba67d07.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog_atomic-dbfbef9fb2cd55d6.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libwebdriver-9a56d9445ca415e2.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libbacktrace-20512a89ba0bf6bb.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libwinapi-059ab74629fc5e13.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libkernel32-4b518f498c1ea5ac.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/librustc_demangle-d8604ff36b59028d.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libhyper-b7d1c5d1d849c727.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libsolicit-d8e6830b07855294.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libhpack-e4ba59af8835f1f6.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libhttparse-39ddd14ae503a388.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunicase-8ae54d175124a903.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libcookie-c6b4f5c08d557750.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liburl-513654a7ee4afefb.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/librustc_serialize-f4966612c71e48c8.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libtypeable-f0ed0017e3fcca89.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmozprofile-55a0a280914bde16.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libtempdir-c0e9b0e3be8f80a1.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/librand-f3e5211f56db4055.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libzip-1f1f055551e13c38.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libflate2-945d2eb78968f97a.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libminiz_sys-9e53897609697b20.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libbzip2-0722adb5ca379393.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libbzip2_sys-93df5a7851b5a420.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libpodio-d505c961496ab04b.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libcfg_if-9e274177fca62f87.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liblazy_static-8dc5aa0427cd4b28.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libtraitobject-a1d2a583fb41ef5d.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmime-fd4caa738959e74b.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmsdos_time-f61afab8dae57e56.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libdbghelp-52ed133691232428.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libclap-fe06492f4782071c.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libterm_size-eeb1ee5b5e1eb249.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libvec_map-30b783050663ec47.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libbitflags-e290cd5819b8b8f3.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunicode_width-532714aec7829268.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libstrsim-138c1b6b5d3ec75f.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunicode_segmentation-077e3519e0fde862.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libidna-02b65ca4ad3c3601.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunicode_normalization-4594db9563c011b7.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunicode_bidi-c03050ac0e7e15d5.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmatches-48eeeed12bed3e42.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libnum_cpus-b634209bff9c50b3.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liblanguage_tags-fbead53520c99a9f.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libregex-0795f72bd1c3b21e.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libutf8_ranges-a1f36e4cffdcb196.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libregex_syntax-8904624ad4d7408d.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libaho_corasick-7c599634a3a57dea.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libmemchr-262dcb3de71bc418.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog_stdlog-d8c72a8ded419bc5.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libcrossbeam-e9253f66a0e4bdee.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liblazy_static-71d2397280697ce5.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liblog-aafafcb4d6981270.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog_term-19db1680c1b96c93.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libthread_local-20995b81eb9d47ef.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libunreachable-02bbc7e02974463c.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libvoid-3313c1ccd1889d8e.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libthread_id-ba6c739df6ce69d8.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libchrono-fa97c5438f4f5d73.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libnum-5196260a17d9d54c.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libnum_iter-2a175c94c0e0f047.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libnum_integer-4431efb114b81eb7.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libnum_traits-d95403325b93dccb.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libtime-21206bae916c8e9f.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libisatty-45a3f8021cdc7371.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog_stream-a5150426f7b7a6a3.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog_extra-09dad9beba43aa07.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libthread_local-6c1c3f7b9e578c1b.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libthread_id-daa3e2ddc51f8d14.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/liblibc-7cef59ab52023ebd.rlib" "/home/worker/workspace/build/src/obj-firefox/testing/geckodriver/x86_64-apple-darwin/release/deps/libslog-2c7f4a732e2d7c49.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libstd-f1544d51c14ee547.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libpanic_unwind-0973ad751bdffbae.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libunwind-30637a1739b412eb.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/librand-6ce8560490ee791c.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libcollections-77c40ab2fac1172e.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/liballoc-40208fb59386bff5.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/liballoc_jemalloc-4b56f5c0b7251555.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/liblibc-cba64299ce12485f.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libstd_unicode-a98ebaa82aaee358.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libcore-cfc94a4f91ad8df0.rlib" "/home/worker/workspace/build/src/rustc/lib/rustlib/x86_64-apple-darwin/lib/libcompiler_builtins-51cf9867f46a760f.rlib" "-l" "System" "-l" "pthread" "-l" "c" "-l" "m"
[task 2017-02-17T20:40:52.908462Z] 20:40:52     INFO -    = note: /home/worker/workspace/build/src/obj-firefox/testing/geckodriver/./x86_64-apple-darwin/release/deps/geckodriver-7a279504e4466bda.0.o: file not recognized: File format not recognized

	(not currently active) Ted Mielczarek
	Comment 10 • 7 years ago

This is the Rust link failing because it's using the wrong linker. We need to pass the right linker info to cargo to make this work.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 11 • 7 years ago

That makes sense.  I think you can do this with `cargo build -- -L LINKER` or by modifying .cargo/config to point to it, but I assume this is something we need to do at a mozbuild level?  Is there a bug for this, or should I file one?

	Nathan Froyd [:froydnj]
	Comment 12 • 7 years ago

This is probably bug 1329737.

	Steve Fink [:sfink] [:s:]
	Comment 31 • 7 years ago
mozreview-review

Comment on attachment 8842560 [details]
Bug 1340637 - Update geckodriver cargo lockfile for vendored crates;

https://reviewboard.mozilla.org/r/116340/#review117970

This is the old version, with the full-on configure option. I think you need to upload the one that grabs it from an environment variable, if you've gotten it working now.

(Also, I'm fine with whatever mechanism ted is ok with, so you can just request review from him.)

	James Graham [:jgraham]
	Comment 32 • 7 years ago
mozreview-review

Comment on attachment 8838684 [details]
Bug 1340637 - Provide build rules for geckodriver;

https://reviewboard.mozilla.org/r/113506/#review118252

I assume this is unchanged from upstream.

I'm in-general supportitive of putting this in-tree, but there are a few outstanding questions:

* How do we expect the development to proceed in the future?
* Does this end up with the same platform coverage?
* Do we expect the released binaries to come from in-tree builds or travis builds?
* Are we correctly packaging the binary somewhere (e.g. in tests.common.zip)?

	(not currently active) Ted Mielczarek
	Comment 33 • 7 years ago

(In reply to James Graham [:jgraham] from comment #32)
> * How do we expect the development to proceed in the future?

This is a pretty important question. Are you intending on doing primary development on GitHub and just periodically vendoring a copy into m-c?

	(not currently active) Ted Mielczarek
	Comment 34 • 7 years ago
mozreview-review

Comment on attachment 8838684 [details]
Bug 1340637 - Provide build rules for geckodriver;

https://reviewboard.mozilla.org/r/113506/#review118254

I'm fine with putting this code here, but I have a couple of questions:
* Are you OK with not having the full git history in m-c, and instead just having a snapshot for every vendored update?
* What process did you use to produce the code in `testing/geckodriver`, and should you include a script here to reproduce the process for future updates?
* What's the process for making changes to the geckodriver code in m-c? PRs against the GitHub repository and then updating the snapshot? I see there's a `Contributing.md` file, perhaps adding a section on how to make changes to the copy used in Firefox builds would be useful.

	(not currently active) Ted Mielczarek
	Comment 35 • 7 years ago
mozreview-review

Comment on attachment 8838685 [details]
Bug 1340637 - Skip geckodriver in hazard builds;

https://reviewboard.mozilla.org/r/113508/#review118256

::: toolkit/toolkit.mozbuild:168
(Diff revision 6)
>          '/testing/firefox-ui',
>          '/testing/marionette',
>      ]
>  
> +    # Disable building of geckodriver for artifact build, macOS,
> +    # and Android.

When the cross-compile bug gets sorted out we'll want to re-enable it for OS X. As we discussed on IRC, for Android we might want to build a host binary instead, which I don't think we currently have support for in the build system. I think maybe you should make this test `if CONFIG['COMPILE_ENVIRONMENT'] and not CONFIG['CROSS_COMPILE']:` since the only reason OS X and Android builds are broken is because they're both cross-compiles.

	(not currently active) Ted Mielczarek
	Comment 36 • 7 years ago
mozreview-review

Comment on attachment 8842560 [details]
Bug 1340637 - Update geckodriver cargo lockfile for vendored crates;

https://reviewboard.mozilla.org/r/116340/#review118258

::: build/moz.configure/toolchain.configure:143
(Diff revision 1)
>  def sccache_verbose_stats(using_sccache, verbose_stats):
>      return using_sccache and bool(verbose_stats)
>  
>  set_config('SCCACHE_VERBOSE_STATS', sccache_verbose_stats)
>  
> +js_option('--enable-hazard-build', env='MOZ_HAZARD',

What sfink said. I think all you need to do is remove the '--enable-hazard-build' here, and replace the one in the `depends_if` below with 'MOZ_HAZARD'.

	(not currently active) Ted Mielczarek
	Comment 37 • 7 years ago
mozreview-review

Comment on attachment 8842561 [details]
Bug 1340637 - Skip geckodriver in hazard builds;

https://reviewboard.mozilla.org/r/116342/#review118260

Can you file a bug on the compiler wrapper + build script issue and put the bug number in the comment?

	(not currently active) Ted Mielczarek
	Comment 38 • 7 years ago

So I only just now saw in the commit message:
"It is the intention that the testing/geckodriver directory
will be a two-way sync between the geckodriver project on GitHub
(https://github.com/mozilla/geckodriver) and mozilla-central."

Have you run this by anyone that's dealing with the Servo syncing situation? That is proving to be a lot of work and I'm not sure we want to commit to doing that for any other projects currently.

	(not currently active) Ted Mielczarek
	Comment 39 • 7 years ago

For the vendored deps patch: could you do a once-over and check the licensing of any new dependencies? Additionally, if you could list the authors of any new crates that would make it easier for me to rubber-stamp the change.

	(not currently active) Ted Mielczarek
	Comment 40 • 7 years ago

See bug 1314071 for licensing concerns.

	James Graham [:jgraham]
	Comment 41 • 7 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #38)
> So I only just now saw in the commit message:
> "It is the intention that the testing/geckodriver directory
> will be a two-way sync between the geckodriver project on GitHub
> (https://github.com/mozilla/geckodriver) and mozilla-central."


FWIW I don't think this is worth it. So far, excluding a license file addition, we have a single digit number of changed lines from contributors who are not otherwise contributing to gecko. I think we should just keep geckodriver in the Mozilla tree and use GitHub just for hosting releases and issue tracking. This will require a little infrastructure to replace the travis configs, but it seems achievable.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 42 • 7 years ago
mozreview-review-reply

Comment on attachment 8838684 [details]
Bug 1340637 - Provide build rules for geckodriver;

https://reviewboard.mozilla.org/r/113506/#review118254

> What process did you use to produce the code in testing/geckodriver,
> and should you include a script here to reproduce the process for
> future updates?

I recursively copied the geckodriver git repository in to
testing/geckodriver, then deleted the .git and target folders.  This
would be equivalent to using git-archive(1):

	% rm -r $GECKO/testing/geckodriver
	% git -C $GECKODRIVER archive master | tar -x -C $GECKO/testing/geckodriver

I could make a script for this, but I hestitate to write a script that
has destructive behaviour and which will delete files.  Since the steps
to dump the geckodriver git repository to testing/geckodriver in central
are so straight forward, it would be sufficient to document them in the
geckodriver README?

> What's the process for making changes to the geckodriver code
> in m-c? PRs against the GitHub repository and then updating the
> snapshot? I see there's a Contributing.md file, perhaps adding a
> section on how to make changes to the copy used in Firefox builds
> would be useful.

I would like it to be possible to edit geckodriver in m-c too, which
would require a sort of two way sync.  See my above reply to jgraham.

We would definitely have to expand the CONTRIBUTING.md file or README.md
files in geckodriver once this lands to elaborate on the development
process.  Thanks for reminding me.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 43 • 7 years ago
mozreview-review-reply

Comment on attachment 8838684 [details]
Bug 1340637 - Provide build rules for geckodriver;

https://reviewboard.mozilla.org/r/113506/#review118252

> I assume this is unchanged from upstream.

It is an export of https://github.com/mozilla/geckodriver/pull/478, so
that needs to land before we import this to central.  Can you have a
look at the preparation changes I made over there?

> How do we expect the development to proceed in the future?

I talk a little bit about this in the commit message:

> It is the intention that the testing/geckodriver directory will
> be a two-way sync between the geckodriver project on GitHub
> (https://github.com/mozilla/geckodriver) and mozilla-central.  This
> means it is perfectly acceptable to land changes to geckodriver in m-c
> without worrying about upstreaming the changes.

I imagine we could accept changes both in-tree and on GitHub, and do a
semi-regular synchronisation between the two by dumping an export from
the git repo in central and applying the hg patches from central to git
(we can use moz-git-tool’s hg-patch-to-git-patch for this).

But ted raises an important point: Are we OK with not having the full
git history in m-c?

My take on this is that the ideal process here would be to have an
automated synchronisation tool that would apply individual commits from
GitHub to central and vice versa.  I hear some rumours have happened in
this area with regards to Servo, so it might not be too hard to stand up
a similar process for testing/geckodriver.

But I wanted to do a one-off dump into testing/geckodriver as a starting
point and a proof of concept.  If you think it is worth investigating
this already, we can do this, but it means I will likely have to redo
the version pinning when third_party/rust dependencies change.  That
is currently not a trivial job, so until `./mach vendor rust` can
transitively vendor dependencies’ dependencies correctly, I would
suggest we either take this dump as it is—or hold off importing
geckodriver to central until mach/cargo has been fixed.

ted: do you know if there is a bug on the dependency vendoring issue
already?  You mentioned you thought it could be to do with cargo
workspaces, but from what I can tell the workspace bug has been resolved
fixed.

> Does this end up with the same platform coverage?

No.  The binaries from the build slaves will, on Linux, be built without
musl, and we won’t have Linux ARM support or macOS support (yet).

> Do we expect the released binaries to come from in-tree builds or
> travis builds?

They can only come from Travis builds until we resolve the macOS compile
environment issue, stand up a TC task to cross compile from Linux
x86_64 to ARMv7 HF, and make the musl toolchain available in the Linux
environments.

All of this is of course do-able, but since Travis (sort of) works for
the time being, I suggest we continue doing releases from there.  I
don’t consider deprecating Travis a priority.

> Are we correctly packaging the binary somewhere (e.g. in
> tests.common.zip)?

This still needs to be done.  If you agree, I want to do this as a
follow-up change.  We also need to make mach/mozharness/wptrunner pick
up the binary from the test zip.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 44 • 7 years ago

(In reply to James Graham [:jgraham] from comment #41)

> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #38)
>
> > So I only just now saw in the commit message:  "It is the
> > intention that the testing/geckodriver directory will be
> > a two-way sync between the geckodriver project on GitHub
> > (https://github.com/mozilla/geckodriver) and mozilla-central."
>
> FWIW I don't think this is worth it. So far, excluding a license
> file addition, we have a single digit number of changed lines from
> contributors who are not otherwise contributing to gecko. I think we
> should just keep geckodriver in the Mozilla tree and use GitHub just
> for hosting releases and issue tracking. This will require a little
> infrastructure to replace the travis configs, but it seems achievable.

OK, that is a fair point.  It is perhaps hard to justify the extra work
that a two-way sync would require.

What do we do with PRs on GitHub in this case?  If they are interesting,
take the commits and apply them ourselves to m-c?  Ask the contributors
to resubmit them through Bugzilla?  I ask about this because I don’t
know if it is possible to _disable PRs_ on GitHub.

Given that we won’t accept PRs on GitHub and only use the GitHub repo
for releases through Travis and issue tracking, this reverts ted’s
earlier question about revision control history: Do we want the git
repository to be an accurate representation of the changes made in
central?  Or are we fine with the occasional dump into git?

FWIW I don’t think it would be too hard to stand up a one-way
conversion of individual hg patches or to do the dumping.  Perhaps we
could start with one-off semi-regular dumps from central.

	James Graham [:jgraham]
	Comment 45 • 7 years ago

(In reply to Andreas Tolfsen ‹:ato› from comment #44)
> (In reply to James Graham [:jgraham] from comment #41)

> Given that we won’t accept PRs on GitHub and only use the GitHub repo
> for releases through Travis and issue tracking, this reverts ted’s
> earlier question about revision control history: Do we want the git
> repository to be an accurate representation of the changes made in
> central?  Or are we fine with the occasional dump into git?

We could replace the master branch on GitHub with a README saying "this code has moved to <location>, please still file issues here". People are unlikely to make a PR against that, which is probably fine given the relative lack of contributions.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 46 • 7 years ago

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #39)
> For the vendored deps patch: could you do a once-over and check the
> licensing of any new dependencies? Additionally, if you could list the
> authors of any new crates that would make it easier for me to rubber-stamp
> the change.

Sure!  This is a complete list of the new dependencies introduced by this changeset, their licenses, and authors:

advapi32-sys (MIT) by Peter Atashian (@retep998)
backtrace-sys (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
backtrace (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
bzip2-sys (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
bzip2 (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
chrono (MIT/Apache-2.0) by Kan Seonghoon (@lifthrasiir)
cookie (MIT/Apache-2.0) by Sergio Benitez (@SergioBenitez) and Alex Crichton (@alexcrichton)
crossbeam (MIT/Apache-2.0) by Aaron Turon (@aturon) and Alex Crichton (@alexcrichton)
dbghelp-sys (MIT) by Peter Atashian (@retep998)
flate2 (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
httparse (MIT/Apache-2.0) by Sean McArthur (@seanmonstar)
hyper (MIT) by Sean McArthur (@seanmonstar)
isatty (MIT/Apache-2.0) by David Tolnay (@dtolnay)
kernel32-sys (MIT) by Peter Atashian (@retep998)
ktmw32-sys (MIT) by Peter Atashian (@retep998)
language-tags (MIT) by @pyfisch
lazy_static (MIT) by Marvin Löbel (@Kimundi)
mime (MIT) by Sean McArthur (@seanmonstar)
miniz-sys (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
mozprofile (MPL-2.0) by James Graham (@jgraham)
mozrunner (MPL-2.0) by James Graham (@jgraham)
msdos_time (MIT/Apache-2.0) by Mathijs van de Nes (@mvdnes)
num-bigint (MIT/Apache-2.0) by Josh Stone (@cuviper) and Łukasz Jan Niemier (@hauleth)
num-complex (MIT/Apache-2.0) by Josh Stone (@cuviper) and Łukasz Jan Niemier (@hauleth)
num-iter (MIT/Apache-2.0) by Josh Stone (@cuviper) and Łukasz Jan Niemier (@hauleth)
num-rational (MIT/Apache-2.0) by Josh Stone (@cuviper) and Łukasz Jan Niemier (@hauleth)
num (MIT/Apache-2.0) by Alex Crichton (@alexcrichton), Josh Stone (@cuviper), and Łukasz Jan Niemier (@hauleth)
podio (MIT/Apache-2.0) by Mathijs van de Nes (@mvdnes)
rustc-demangle (MIT/Apache-2.0) by Alex Crichton (@alexcrichton)
rustc_version (MIT/Apache-2.0) by Marvin Löbel (@Kimundi)
semver (MIT/Apache-2.0) by Alex Crichton (@alexcrichton) and Steve Klabnik (@steveklabnik)
slog-atomic (MPL-2.0) by Dawid Ciężarkiewicz (@dpc)
slog-extra (MPL-2.0) by Dawid Ciężarkiewicz (@dpc)
slog-stream (MPL-2.0) by Dawid Ciężarkiewicz (@dpc)
slog-term (MPL-2.0) by Dawid Ciężarkiewicz (@dpc)
slog (MPL-2.0) by Dawid Ciężarkiewicz (@dpc)
tempdir (MIT/Apache-2.0) by Alex Crichton (@alexcrichton) and Huon Wilson (@huonw)
thread-id (Apache-2.0) by Ruud van Asseldonk (@ruuda)
thread_local (MIT/Apache-2.0) by Amanieu d'Antras (@Amanieu)
traitobject (MIT/Apache-2.0) by Jonathan Reem (@reem)
typeable (MIT) by Jonathan Reem (@reem)
unicase (MIT) by Sean McArthur (@seanmonstar)
webdriver (MPL-2.0) by James Graham (@jgraham) and Andreas Tolfsen (@andreastt)
winreg (MIT) by @gentoo90
zip (MIT) by Mathijs van de Nes (@mvdnes)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 47 • 7 years ago
mozreview-review-reply

Comment on attachment 8838685 [details]
Bug 1340637 - Skip geckodriver in hazard builds;

https://reviewboard.mozilla.org/r/113508/#review118256

> When the cross-compile bug gets sorted out we'll want to re-enable it for OS X. As we discussed on IRC, for Android we might want to build a host binary instead, which I don't think we currently have support for in the build system. I think maybe you should make this test `if CONFIG['COMPILE_ENVIRONMENT'] and not CONFIG['CROSS_COMPILE']:` since the only reason OS X and Android builds are broken is because they're both cross-compiles.

Excellent, thanks for illuminating my knowledge of CONFIG variables.  This now reads:

>     # Disable building of geckodriver for artifcat builds and in
>     # environments where cross compiling occurs (macOS and Android),
>     # and on hazard builds.
>     #
>     # https://bugzilla.mozilla.org/show_bug.cgi?id=1341041
>     # https://bugzilla.mozilla.org/show_bug.cgi?id=1329737
>     if CONFIG['COMPILE_ENVIRONMENT'] and \
>             not CONFIG['CROSS_COMPILE'] and \
>             not CONFIG['MOZ_HAZARD']:
>         DIRS += ['/testing/geckodriver']

I could be more specific in the comment about _why_ building geckodriver in cross-compile builds don’t currently work.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 52 • 7 years ago
mozreview-review-reply

Comment on attachment 8842560 [details]
Bug 1340637 - Update geckodriver cargo lockfile for vendored crates;

https://reviewboard.mozilla.org/r/116340/#review118258

> What sfink said. I think all you need to do is remove the '--enable-hazard-build' here, and replace the one in the `depends_if` below with 'MOZ_HAZARD'.

Thanks for the example, ted!  I changed this to use option and that
works fine:

> option(env='MOZ_HAZARD', help='Build for the GC rooting hazard analysis')

You told me on IRC I should probably leave this as a js_option,
considering it is an option that should be available to standalone
builds.

However, if I s/option/js_option/ I get the following error message:


	 0:05.38 configure: warning: MOZ_HAZARD=: invalid host typeNone
	 0:05.42 loading cache ./config.cacheNone
	 0:05.42 configure: error: can only configure for one host and one target at a timeNone

The change is:

> diff --git a/build/moz.configure/toolchain.configure b/build/moz.configure/toolchain.configure
> index 298a480..bf9da81 100644
> --- a/build/moz.configure/toolchain.configure
> +++ b/build/moz.configure/toolchain.configure
> @@ -140,7 +140,7 @@ def sccache_verbose_stats(using_sccache, verbose_stats):
>  
> set_config('SCCACHE_VERBOSE_STATS', sccache_verbose_stats)
>  
> -option(env='MOZ_HAZARD', help='Build for the GC rooting hazard analysis')
> +js_option(env='MOZ_HAZARD', help='Build for the GC rooting hazard analysis')
>  set_config('MOZ_HAZARD', depends_if('MOZ_HAZARD')(lambda _: True))
>  
>  @depends('--with-compiler-wrapper', ccache)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 54 • 7 years ago
mozreview-review

Comment on attachment 8843016 [details]
Bug 1340637 - Don't send the eForceQuit flag when shutting down the session;

https://reviewboard.mozilla.org/r/116760/#review118410

	(not currently active) Ted Mielczarek
	Comment 55 • 7 years ago
mozreview-review

Comment on attachment 8842560 [details]
Bug 1340637 - Update geckodriver cargo lockfile for vendored crates;

https://reviewboard.mozilla.org/r/116340/#review118696

::: toolkit/toolkit.mozbuild:167
(Diff revision 2)
>      DIRS += [
>          '/testing/firefox-ui',
>          '/testing/marionette',
>      ]
>  
>      # Disable building of geckodriver for artifcat builds and in

typo: artifact (seems like it was in an earlier commit

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 56 • 7 years ago

Over the past two weeks I’ve worked on clearing out the geckodriver PR queue on GitHub in preparation of moving development to mozilla-central.

I’ve rewritten the geckodriver git history so that files appear to have been created in testing/geckodriver, purged all the merge commits, and picked the remaining commits into my mozilla-central git checkout.  The rewritten history can be examined at https://git.sny.no/gecko/log/?h=geckodriver-import.

gps: I want to seek your feedback on the prospect of landing these 287 commits on mozilla-central.  Unlike Servo, we’re not seeking a two-way synchronisation between these repos: https://github.com/mozilla/geckodriver will be decommissioned and all future development will happen in m-c.

In the short- to medium term, we plan to continue doing releases of geckodriver from GitHub through Travis.  This is due to a couple of different factors (release package hosting, TaskCluster to GitHub Releases, and challenges to do with cross-compilation to Android), but the long-term plan is to also enable releases from m-c through TC.

I sent an intent-to-implement outlining the plan in more detail to tools@ a few weeks ago: https://groups.google.com/forum/#!topic/mozilla.tools/9NGyu-A0Cf0

It would be great if you could give me some feedback how to proceed with this.  If you are satisfied with the commit history rewrite, it would be superfluous to upload this to mozreview since the commits have already been reviewed by Mozillians out-of-tree (jgraham/AutomatedTester/maja_zf/ato).

	(not currently active) Ted Mielczarek
	Comment 57 • 7 years ago
mozreview-review

Comment on attachment 8838686 [details]
Bug 1340637 - Vendor geckodriver dependencies;

https://reviewboard.mozilla.org/r/113510/#review132050

	Gregory Szorc [:gps]
	Comment 58 • 7 years ago

The volume and size of the added commits isn't really a problem. We only appear to be talking about a few hundred KB and handful of files.

There are some choices as to how to integrate those into the Firefox repo.

I'm unable to clone your Git repo, so I can't inspect things the way I'd like to. But I will say that the code we used for initially importing and continuing to import Servo's code into mozilla-central is generic and can be used on any Git[Hub] project, including geckodriver. This code is robust and can normalize things in a way that makes the converted commits useful. e.g. it preserves a link to the original Git commit which is rendered on hg.mozilla.org.

If you clone version-control-tools and run `NO_DOCKER=1 ./create-test-environment`, the venv/bin/linearize-git-to-hg script can be used to linearize the history of a git repo and convert it to an hg repo. There's a lot of features it can do, including grabbing PR titles from the GitHub API and rewriting the commit message summary line. Try something like:

  $ linearize-git-to-hg --summary-prefix 'geckodriver:' --source-repo-key Source-Repository --source-revision-key Source-Revision --use-p2-author --copy-similarity 75 --find-copies-harder https://github.com/mozilla/geckodriver master ~/tmp/geckodriver-git ~/tmp/geckodriver-hg

Once you have a Mercurial conversion, the venv/bin/overlay-hg-repos script can be used to replay/overlay the changesets from a source repository into the subdirectory of a destination repository. e.g. you can take your converted geckodriver hg repo and replay the file changes into the geckodriver/ directory of mozilla-central. This will produce linear history in mozilla-central.

Alternatively, you could use linearize-git-to-hg to move files into a subdirectory. Then you could `hg pull -f` that repo into mozilla-central, adding a new root changeset. You would then `hg merge` that into the mozilla-central mainline. This is what we did for Servo. A benefit of that approach is the history of that subdirectory is isolated from the rest of mozilla-central until the merge point. If we go with this approach, we need to make a config change on the server to allow it to be pushed. This is trivial to do though.

Since you aren't looking for an as-robust-as-servo approach, I don't have a strong opinion on what to do here. But I would prefer you use linearize-git-to-hg at least with the sample options I used to ensure an optimal conversion.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 59 • 7 years ago

Thanks for your detailed feedback, gps!

With help from glob, I followed the first approach you outlined.  This worked to my satisfaction and you can see the full hg log in https://gist.github.com/andreastt/10772828bc8f1649354390ab344a6b7b.  This does not, if I interpret the dotted lines added by `hg log -G` correctly, create a new root.

You mention that a benefit of the alternative approach is that the history of testing/geckodriver would be isolated until the merge point, but I wonder how strongly you feel about this?

	Gregory Szorc [:gps]
	Comment 60 • 7 years ago

I have no strong feelings about adding a new root versus an overlay of everything onto mozilla-central.

I will say a benefit of the root approach is it doesn't interfere with bisection as much and it can cut down on e.g. `hg log <directory>` times. These are both "tools driving workflow decisions" which is arguably a tool deficiency. Anyway, given servo's commit count, that bisection issue was a big problem, so we went with roots. It's less of a concern here.

	Gregory Szorc [:gps]
	Comment 61 • 7 years ago

For posterity, I started a brain dump of what factors go in to vendoring projects: https://mozilla-version-control-tools.readthedocs.io/en/latest/vcssync/vendoring.html

It is missing the "and this is how you do it." But I suppose something is better than random comments scattered across random bugs.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 62 • 7 years ago

I have uploaded the linearised patches in https://hg.mozilla.org/users/atolfsen_mozilla.com/gecko/.  Because hg.mozilla.org rejects me from pushing these, could you extract and land them on inbound, gps?

	Gregory Szorc [:gps]
	Comment 63 • 7 years ago

We keep missing each other on IRC.

I discovered that when you run `hg rebase` it drops some of the hidden metadata that's used to track the last overlayed changeset. If we will be incrementally converting from git to hg, this could be problematic. But I /think/ the plan is to make mozilla-central the new canonical home, so loss of this metadata can be tolerated. I just wanted to confirm before landing this just in case.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 64 • 7 years ago

(In reply to Gregory Szorc [:gps] from comment #63)
> I discovered that when you run `hg rebase` it drops some of the hidden
> metadata that's used to track the last overlayed changeset. If we will be
> incrementally converting from git to hg, this could be problematic. But I
> /think/ the plan is to make mozilla-central the new canonical home, so loss
> of this metadata can be tolerated. I just wanted to confirm before landing
> this just in case.

I don’t know what you’re talking about when referring to “hidden metadata”, but it is indeed the intention that this will be a one-time migration from git to mozilla-central, and m-c will be its new canonical home.

I intend to ‘tombstone’ the GitHub project (replace the master branch with a README) once this migration is complete.

	Gregory Szorc [:gps]
	Comment 65 • 7 years ago

https://hg.mozilla.org/integration/autoland/pushloghtml?startID=43042&endID=43043

Sorry it took all week.

`hg overlay` sticks some hidden metadata in the "extra" field of changesets. You can see that if you run `hg --debug log`. Since you aren't doing ongoing conversions, this extra metdata isn't vital. So it didn't make it to the Firefox repo.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 70 • 7 years ago

geckodriver has now landed in m-c in testing/geckodriver, and through some trickery with .cargo/config.in I was able to generate a testing/geckodriver/Cargo.lock that pins the exact versions available under third_party/rust for geckodriver.

I will file a separate bug about making `./mach vendor rust` pick up .cargo/config.in on a second pass in the srcdir later.

pushlog to try: https://hg.mozilla.org/try/pushloghtml?changeset=d1eca3a9544474d2bce4d40f0c1d7edc2227d203
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d1eca3a9544474d2bce4d40f0c1d7edc2227d203
mozreview: https://reviewboard.mozilla.org/r/113504/

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 73 • 7 years ago

I forgot to include some dot-prefixed directories when I added the new dependencies, which caused the integrity check of a crate to fail.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f5176d46f99d

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 74 • 7 years ago

Looks like the failing builds are on Linux x86.  The Linux x86 try builders run on the same x86_64 VMs as the Linux 64 builds, which means we cross-compile geckodriver (and Firefox) to i686.  I’m submitting an additional patch with r=me that disables geckodriver building on Linux i686 for the time being.  I’ve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1367519 to follow up on this later.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 80 • 7 years ago
mozreview-review

Comment on attachment 8870961 [details]
Bug 1340637 - Disable geckodriver compilation on Linux i686;

https://reviewboard.mozilla.org/r/142524/#review146112

	Pulsebot
	Comment 101 • 7 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4d42ddbe3318
Provide build rules for geckodriver; r=jgraham,ted
https://hg.mozilla.org/integration/autoland/rev/26c5ecc78764
Skip geckodriver in hazard builds; r=ted
https://hg.mozilla.org/integration/autoland/rev/f9e93f02596f
Vendor geckodriver dependencies; r=ted
https://hg.mozilla.org/integration/autoland/rev/ffcd5303a4ff
Update geckodriver cargo lockfile for vendored crates; r=ted
https://hg.mozilla.org/integration/autoland/rev/071bd9eec146
Disable geckodriver compilation on Linux i686; r=ato

	Ryan VanderMeulen [:RyanVM]
	Comment 102 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4d42ddbe3318
https://hg.mozilla.org/mozilla-central/rev/26c5ecc78764
https://hg.mozilla.org/mozilla-central/rev/f9e93f02596f
https://hg.mozilla.org/mozilla-central/rev/ffcd5303a4ff
https://hg.mozilla.org/mozilla-central/rev/071bd9eec146

	Gregory Szorc [:gps]
	Comment 103 • 7 years ago

How important is it to build geckodriver by default? It adds significant overhead to the build for a feature that (correct me if I'm wrong) won't be used by the average developer. I question whether developers' machines should spend the CPU required to build it.

	David Burns :automatedtester
	Comment 104 • 7 years ago

(In reply to Gregory Szorc [:gps] from comment #103)
> How important is it to build geckodriver by default? It adds significant
> overhead to the build for a feature that (correct me if I'm wrong) won't be
> used by the average developer. I question whether developers' machines
> should spend the CPU required to build it.

This is going to be used by anyone who is working on web exposed features in tree. It is going to be used by anyone who needs to run web platform tests. If we can set this to build only when we people try run WPT. So thats on by default in automation and if there was a way for mach to know it needed to build it when ./mach web-platform-tests is run then we can be more selective.

	James Graham [:jgraham]
	Comment 105 • 7 years ago

So at this point it's only needed by wdspec tests, so I think it would be acceptable to require a specific --enable-geckodriver in the config if the Rust complilation story is still too much sadness. That would have to be on in CI ofc. In the future this *may* be needed for more wpt tests, so in that case it would ertainly want to be controlled by --disable-test, which I think is the current situation (or at least is what we discussed).

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 106 • 7 years ago

It’s a shame we can’t enable it by default because Rust compilation is too slow.  At the current time, having an --enable-geckodriver build config flag is reasonable, but as jgraham points out, we need to compile geckodriver in CI to run the wdspec tests.  I have filed follow-up bug https://bugzilla.mozilla.org/show_bug.cgi?id=1368035.

Hopefully by the time we need to enable geckodriver by default, the Rust compile story will have improved.  There is ongoing work in Rust master to improve incremental compilation, and I believe ted said he had a patch in his queue to enable sccache’d Rust compilation.

	Ralph Giles (:rillian)
	Comment 107 • 7 years ago

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?

	Milan Sreckovic [:milan] (needinfo for best results)
	Comment 108 • 7 years ago

This broke local windows builds (see bug 1367995) - it needs to eb backed out immediately.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 109 • 7 years ago

We temporarily disabled geckodriver compilation in https://bugzil.la/1368084 whilst we are working on fixing https://bugzilla.mozilla.org/show_bug.cgi?id=1368035 to make it an opt-in.