Closed Bug 1340637 Opened 3 years ago Closed 3 years ago
Import and build geckodriver in central
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
59 bytes, text/x-review-board-request
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.
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.
Assignee: nobody → ato
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.
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
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.
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?
This is probably bug 1329737.
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.)
Attachment #8842560 - Flags: review?(sphink) → 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)?
Attachment #8838684 - Flags: review?(james) → review+
(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?
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.
Attachment #8838684 - Flags: review?(ted) → 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.
Attachment #8838685 - Flags: review?(ted) → 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'.
Attachment #8842560 - Flags: review?(ted)
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?
Attachment #8842561 - Flags: review?(ted) → review+
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.
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.
See bug 1314071 for licensing concerns.
(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.
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.
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.
(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.
(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.
(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)
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.
Attachment #8842561 - Attachment is obsolete: true
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)
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
Attachment #8843016 - Flags: review?(ato) → 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
Attachment #8842560 - Flags: review?(ted) → review+
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).
Comment on attachment 8838686 [details] Bug 1340637 - Vendor geckodriver dependencies; https://reviewboard.mozilla.org/r/113510/#review132050
Attachment #8838686 - Flags: review?(ted) → review+
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.
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?
Priority: -- → P1
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.
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.
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?
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.
Flags: needinfo?(gps) → needinfo?(ato)
(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.
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.
Attachment #8843016 - Attachment is obsolete: true
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/
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
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.
Comment on attachment 8870961 [details] Bug 1340637 - Disable geckodriver compilation on Linux i686; https://reviewboard.mozilla.org/r/142524/#review146112
Attachment #8870961 - Flags: review?(ato) → review+
Pushed by email@example.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
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.
(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.
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).
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.
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?
This broke local windows builds (see bug 1367995) - it needs to eb backed out immediately.
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.
You need to log in before you can comment on or make changes to this bug.