Closed
Bug 1471281
Opened 6 years ago
Closed 6 years ago
Include geckodriver in default build
Categories
(Testing :: geckodriver, enhancement, P1)
Testing
geckodriver
Tracking
(firefox64 fixed)
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: ato, Assigned: ato)
References
Details
Attachments
(2 files, 10 obsolete files)
3.56 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
8.63 KB,
patch
|
whimboo
:
review+
|
Details | Diff | Splinter Review |
When we migrated geckodriver from GitHub into central it was originally enabled by default in all local builds. Since this was one of the very first Rust components to land in central, support for Rust was a little bit experimental and we discovered a couple of different problems: (1) Not all developers had Rust installed, and support for Rust in the tree was experimental. This caused some compile errors in particular on Windows, and at the time developers were not happy with requiring Rust for building Firefox. (2) The additional build time induced by including a fairly large Rust component in the default build was considered too high. At the time of writing this, Rust support in central has improved vastly. We also require Rust for regular Firefox builds, which addresses part of issue 1. My working theory is that we now build so much Rust code in central that many of the dependencies geckodriver relies on and that takes up a lot of time to build will have been built as dependencies for other Rust components in the tree, effectively making the additional time it takes to build geckodriver less prominent than it was when we originally tried including it in the default build. This bug will track the work towards including geckodriver in the default build when you do "./mach build" locally. We currently have ac_add_options --enable-geckodriver set for all CI/try builds, which means that we—at least in theory—should not have any additional problems enabling it in local builds as well.
Comment 1•6 years ago
|
||
(In reply to Andreas Tolfsen 「:ato」 from comment #0) > My working theory is that we now build so much Rust code in central > that many of the dependencies geckodriver relies on and that takes > up a lot of time to build will have been built as dependencies for > other Rust components in the tree, effectively making the additional > time it takes to build geckodriver less prominent than it was when > we originally tried including it in the default build. You could have a look at some build logs to prove that.
Comment 2•6 years ago
|
||
We have "plain" build variations in CI that measure how long it takes to build without caches and using default settings. This is essentially a proxy for local developer builds. So you can validate your claim that geckodriver won't add much overhead by doing a Try push with the following tasks: build-linux64-plain/opt build-linux64-plain/debug build-win64-plain/opt build-win64-plain/debug And then compare the Perfherder build metrics against the base revision. e.g. what https://treeherder.mozilla.org/perf.html#/graphs?series=autoland,1682289,1,2&series=autoland,1682265,1,2&series=autoland,1691945,1,2&series=autoland,1682223,1,2&series=autoland,1682281,1,2&series=autoland,1697287,1,2 is rendering.
Assignee | ||
Updated•6 years ago
|
Priority: -- → P3
Assignee | ||
Comment 3•6 years ago
|
||
Some local testing I did showed that enabling geckodriver did not have any noteworthy impact on build time: Without geckodriver: 15:37.97 (938r, 399.50s) With geckodriver: 15:44.74 (945r, 397.83s) I will verify against the plain builds gps mentioned too.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee | ||
Comment 4•6 years ago
|
||
(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #3) > I will verify against the plain builds gps mentioned too. Of course we can’t do that, since geckodriver is built with MOZ_AUTOMATION (on try) but just not locally.
Assignee | ||
Comment 5•6 years ago
|
||
Assignee | ||
Comment 6•6 years ago
|
||
Comment on attachment 9003421 [details] [diff] [review] Make building geckodriver mandatory I understand artifact builds now fetch a geckodriver binary from Treeherder, so if I change the default to be to build geckodriver, how can I make this not fail for those who have --enable-artifact-builds set in their mozconfig?
Attachment #9003421 -
Flags: feedback?(cmanchester)
Comment 7•6 years ago
|
||
(In reply to Andreas Tolfsen ﹝:ato﹞ from comment #6) > Comment on attachment 9003421 [details] [diff] [review] > Make building geckodriver mandatory > > I understand artifact builds now fetch a geckodriver binary from > Treeherder, so if I change the default to be to build geckodriver, > how can I make this not fail for those who have --enable-artifact-builds > set in their mozconfig? This shouldn't be an issue, we don't build any rust code locally in an artifact build anyway.
Updated•6 years ago
|
Attachment #9003421 -
Flags: feedback?(cmanchester)
Assignee | ||
Comment 8•6 years ago
|
||
Let me clarify. If I set ac_add_options --enable-artifact-builds and apply the attached patched to central, I get the following configure error: > 0:03.62 Reticulating splines... > 0:04.34 0:00.74 File already read. Skipping: /home/ato/src/gecko/gfx/angle/targets/angle_common/moz.build > 0:07.68 Traceback (most recent call last): > 0:07.68 File "/home/ato/src/gecko/configure.py", line 123, in <module> > 0:07.68 sys.exit(main(sys.argv)) > 0:07.68 File "/home/ato/src/gecko/configure.py", line 34, in main > 0:07.68 return config_status(config) > 0:07.68 File "/home/ato/src/gecko/configure.py", line 118, in config_status > 0:07.68 return config_status(args=[], **encode(sanitized_config, encoding)) > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/config_status.py", line 146, in config_status > 0:07.68 the_backend.consume(definitions) > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/backend/base.py", line 126, in consume > 0:07.68 for obj in objs: > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 185, in emit > 0:07.68 objs = list(emitfn(out)) > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 1184, in emit_from_context > 0:07.68 for obj in self._handle_linkables(context, passthru, generated_files): > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/emitter.py", line 620, in _handle_linkables > 0:07.68 self._binaries[program] = cls(context, program, cargo_file) > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/data.py", line 582, in __init__ > 0:07.68 cargo_dir = cargo_output_directory(context, self.TARGET_SUBST_VAR) > 0:07.68 File "/home/ato/src/gecko/python/mozbuild/mozbuild/frontend/data.py", line 563, in cargo_output_directory > 0:07.68 return mozpath.join(context.config.substs[target_var], rust_build_kind) > 0:07.68 KeyError: u'RUST_TARGET' This is because I’ve changed the default of the "geckodriver" rule to build it by default, except for some unsupported build configurations. You added a compile_env check in https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/toolkit/moz.configure#924-926, which as far as I can see is the only compile_env check in this configure script. Do we still need this?
Flags: needinfo?(cmanchester)
Comment 9•6 years ago
|
||
Right, by removing that check your patch gets us back to the error that motivated it in bug 1370535. We can either hide the option when the compile environment isn't available, as we do with several others (I believe "--disable-gtest-in-build" is an example), or we can dodge the access to that configure variable in the manner we did in https://hg.mozilla.org/mozilla-central/rev/d0fccf681c20 .
Flags: needinfo?(cmanchester)
Assignee | ||
Comment 10•6 years ago
|
||
Thanks, your comment was pretty useful. I found this in the same
toolkit/moz.configure file:
> option('--disable-gtest-in-build',
> help='Force disable building the gtest libxul during the build.',
> when='--enable-compile-environment')
So the option function takes an optional keyword argument ‘when’
one can use to guard, or “hide”, an option with.
Assignee | ||
Comment 11•6 years ago
|
||
Enable building of geckodriver by default where we have a compile environment available. This makes --enable-geckodriver unavailable to artifact builds. geckodriver remains disabled by default on cross compile builds and hazard builds, with Android pointed out specifically (although it is cross compiled).
Attachment #9005202 -
Flags: feedback?(cmanchester)
Assignee | ||
Comment 12•6 years ago
|
||
Attachment #9005203 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9003421 -
Attachment is obsolete: true
Assignee | ||
Comment 13•6 years ago
|
||
Enable building of geckodriver by default where we have a compile environment available. This makes --enable-geckodriver unavailable to artifact builds. Following this change: * --enable-geckodriver is implied in supported build configurations, but may be used in unsupported build configurations (Android, cross compiled, and hazard builds) to force geckodriver to be built. * --disable-geckodriver causes geckodriver not to be built. * In artifact build mode, a geckodriver binary artifact will continue to be downloaded, but it will not be possible to specify --enable-geckodriver without a compile environment. geckodriver remains disabled by default on cross compile builds and hazard builds, pointing out Android specifically (although it is cross compiled).
Attachment #9005213 -
Flags: feedback?(cmanchester)
Assignee | ||
Comment 14•6 years ago
|
||
Attachment #9005214 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9005202 -
Attachment is obsolete: true
Attachment #9005202 -
Flags: feedback?(cmanchester)
Assignee | ||
Updated•6 years ago
|
Attachment #9005203 -
Attachment is obsolete: true
Attachment #9005203 -
Flags: review?(hskupin)
Assignee | ||
Comment 15•6 years ago
|
||
Depending on the length of the review process, I’m likely going to hold off landing any of the patches until hiro-san’s work on https://bugzilla.mozilla.org/show_bug.cgi?id=1417646 is completed.
Assignee | ||
Updated•6 years ago
|
Attachment #9005214 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9005213 -
Flags: feedback?(cmanchester)
Assignee | ||
Comment 16•6 years ago
|
||
I confirmed through two try runs that the attached patches work as intended: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd38f8cf1fb070591ccb4d063c36e3d3fa3654aa https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e1b997a6d3e807875da27123ae571d5e59689ac But for them to land on central we first need to land hiro-san’s patches from https://bugzilla.mozilla.org/show_bug.cgi?id=1417646, and an extra patch to remove the explicit --enable-geckodriver on linux32 that glandium added in https://bugzilla.mozilla.org/show_bug.cgi?id=1486652 along with the changeset in this bug. In total this will make geckodriver work in cross-compile environments (except Android) and enable geckodriver by default, which will render the --enable-geckodriver flag unnecessary in build/unix/mozconfig.linux32, and this in turn will make artifact builds happy since the flag isn’t available to them.
Assignee | ||
Updated•6 years ago
|
Attachment #9005214 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9005213 -
Flags: review?(hskupin)
Attachment #9005213 -
Flags: review?(cmanchester)
Assignee | ||
Comment 17•6 years ago
|
||
Enable building of geckodriver by default where we have a compile environment available. This makes --enable-geckodriver unavailable to artifact builds. Following this change: * --enable-geckodriver is implied in supported build configurations, but may be used in unsupported build configurations (Android, cross compiled, and hazard builds) to force geckodriver to be built. * --disable-geckodriver causes geckodriver not to be built. * In artifact build mode, a geckodriver binary artifact will continue to be downloaded, but it will not be possible to specify --enable-geckodriver without a compile environment. geckodriver remains disabled by default on cross compile builds and hazard builds, pointing out Android specifically (although it is cross compiled).
Attachment #9005213 -
Attachment is obsolete: true
Attachment #9005649 -
Flags: review?(hskupin)
Attachment #9005649 -
Flags: review?(cmanchester)
Attachment #9005213 -
Flags: review?(hskupin)
Attachment #9005213 -
Flags: review?(cmanchester)
Assignee | ||
Comment 18•6 years ago
|
||
Attachment #9005650 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9005214 -
Attachment is obsolete: true
Attachment #9005214 -
Flags: review?(hskupin)
Assignee | ||
Comment 19•6 years ago
|
||
Now that geckodriver is enabled by default in all builds and works in cross-compile environments, we can drop the explicit --enable-geckodriver flag from the linux32 build config since it is no longer inhibited geckodriver not working in cross-compile environments.
Attachment #9005651 -
Flags: review?(cmanchester)
Comment 20•6 years ago
|
||
Comment on attachment 9005649 [details] [diff] [review] Make building geckodriver mandatory. I don't think I'm going to be able to look at this before PTO, forwarding to the shared queue.
Attachment #9005649 -
Flags: review?(cmanchester) → review?(core-build-config-reviews)
Updated•6 years ago
|
Attachment #9005651 -
Flags: review?(cmanchester) → review?(core-build-config-reviews)
Comment 21•6 years ago
|
||
Comment on attachment 9005649 [details] [diff] [review] Make building geckodriver mandatory. Review of attachment 9005649 [details] [diff] [review]: ----------------------------------------------------------------- Canceling review, but not because of anything drastically wrong. ::: toolkit/moz.configure @@ +914,4 @@ > > +option('--enable-geckodriver', default=geckodriver_default, > + when='--enable-compile-environment', > + help='Override default and attempt to build geckodriver') I think this should just be "Build geckodriver for test purposes." Also, the default should probably check --disable-tests, because we wouldn't be using geckodriver in that configuration?
Attachment #9005649 -
Flags: review?(core-build-config-reviews)
Updated•6 years ago
|
Attachment #9005651 -
Flags: review?(core-build-config-reviews) → review+
Comment 22•6 years ago
|
||
Comment on attachment 9005650 [details] [diff] [review] Update geckodriver build instructions. Review of attachment 9005650 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/geckodriver/CONTRIBUTING.md @@ +112,5 @@ > [WebDriver protocol]. geckodriver translates WebDriver [commands], > [responses], and [errors] to the [Marionette protocol], and acts > as a proxy between WebDriver clients and [Marionette]. > > +To build geckodriver from [mozilla-central]: You reference `mozilla-unified` here, which means that you have to add at least a `hg up central` line before building geckodriver. Otherwise you end-up with whatever is on tip which can be anything of central, beta, release, or an esr branch, right? @@ +131,5 @@ > +the binary will be put in the `$(topsrcdir)/target` directory. > + > +You can run your freshly built geckodriver this way: > + > + % ./mach geckodriver -- --other --flags This would even work with artifact builds? That would be great. Also is there a way to specify debug vs. release builds? I assume not, and the appropriate geckodriver build it picked which is similar to Firefox (opt vs. debug). Also can you please refer to Building.md here to avoid all these duplication of content? @@ +146,5 @@ > > > Running the tests > ----------------- > Please reference `Testing.md` here so that we can avoid all the code duplication. ::: testing/geckodriver/doc/Testing.md @@ +18,5 @@ > + > + % ./mach test testing/geckodriver > + > +The webdriver crate tests are unfortunately not yet runnable through mach. > +Work to make this possible is tracked in [[https://bugzil.la/1424369]]. This is marked as WFM, and I also fixed bug 1473341 which should include all the tests, even for mozbase rust. So this can be removed. @@ +46,5 @@ > +As you get in to development of geckodriver and Marionette you will > +increasingly grow to understand our love for [trace-level logs]. > +They provide us with the input—the HTTP requests—from the client > +(in WPT’s case from the tests’ use of a custom WebDriver client), > +the translation geckodriver makes to the [Marionette protocol], `[Marionette protocol]` is not defined. @@ +50,5 @@ > +the translation geckodriver makes to the [Marionette protocol], > +the log output from Marionette, its responses back to geckodriver, > +and finally the output—or the HTTP response—back to the client. > + > +The [trace-level logs] can be surfaced by passing on the `-vv` `[trace-level logs]` is not defined.
Attachment #9005650 -
Flags: review?(hskupin) → review-
Comment 23•6 years ago
|
||
Comment on attachment 9005649 [details] [diff] [review] Make building geckodriver mandatory. Review of attachment 9005649 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a build nor toolkit peer to be able to review that.
Attachment #9005649 -
Flags: review?(hskupin)
Assignee | ||
Comment 24•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #23) > I'm not a build nor toolkit peer to be able to review that. But I still need sign-off from a Marionette peer, since this is very much impacting Marionette. How do I get that?
Flags: needinfo?(hskupin)
Assignee | ||
Comment 25•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #21) > ::: toolkit/moz.configure > @@ +914,4 @@ > > > > +option('--enable-geckodriver', default=geckodriver_default, > > + when='--enable-compile-environment', > > + help='Override default and attempt to build geckodriver') > > I think this should just be "Build geckodriver for test purposes." Also, > the default should probably check --disable-tests, because we wouldn't be > using geckodriver in that configuration? We eventually want to release geckodriver from Taskcluster. Specifically, the same build that is being used for Firefox Nightly because this has the signing infrastructure available we will also need for the geckodriver binaries. My worry was that making it dependent on --disable-tests might accidentally cause it not to be available in these builds. But I checked around about this, and I believe Nightly builds are now made off the normal mozilla-central configuration, so this should be a safe change to make.
Assignee | ||
Comment 26•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #22) > ::: testing/geckodriver/CONTRIBUTING.md > @@ +112,5 @@ > > [WebDriver protocol]. geckodriver translates WebDriver [commands], > > [responses], and [errors] to the [Marionette protocol], and acts > > as a proxy between WebDriver clients and [Marionette]. > > > > +To build geckodriver from [mozilla-central]: > > You reference `mozilla-unified` here, which means that you have to add at > least a `hg up central` line before building geckodriver. Otherwise you > end-up with whatever is on tip which can be anything of central, beta, > release, or an esr branch, right? I don’t know. I understand the canonical repository for Firefox is https://hg.mozilla.org/mozilla-unified/. Do we want to have VCS specific instructions here? AIUI the documentation for that is https://mozilla-version-control-tools.readthedocs.io/en/latest/. > @@ +131,5 @@ > > +the binary will be put in the `$(topsrcdir)/target` directory. > > + > > +You can run your freshly built geckodriver this way: > > + > > + % ./mach geckodriver -- --other --flags > > This would even work with artifact builds? That would be great. Also is > there a way to specify debug vs. release builds? I assume not, and the > appropriate geckodriver build it picked which is similar to Firefox (opt vs. > debug). I don’t use artifact builds so I don’t know if they are opt or debug. I assume it isn’t possible to pick between the two, but chmanchester would know better than me. The line itself should work with artifact builds for as long as they fetch the build artifact, since the only assumption made in https://searchfox.org/mozilla-central/source/testing/geckodriver/mach_commands.py is that the binary exists. > Also can you please refer to Building.md here to avoid all these duplication > of content? CONTRIBUTING.md is valuable for as long as the project is exported to GitHub, because GitHub picks up this file name convention in various interesting places. Building.md is not exported is only available on central. Do you mean we should remove the content from CONTRIBUTING.md and link to firefox-source-docs.m.o instead?
Assignee | ||
Comment 27•6 years ago
|
||
Enable building of geckodriver by default where we have a compile environment available. This makes --enable-geckodriver unavailable to artifact builds. Following this change: * --enable-geckodriver is implied in supported build configurations, but may be used in unsupported build configurations (Android, cross compiled, and hazard builds) to force geckodriver to be built. * --disable-geckodriver causes geckodriver not to be built. * In artifact build mode, a geckodriver binary artifact will continue to be downloaded, but it will not be possible to specify --enable-geckodriver without a compile environment. geckodriver remains disabled by default on cross compile builds and hazard builds, pointing out Android specifically (although it is cross compiled).
Attachment #9005649 -
Attachment is obsolete: true
Attachment #9005983 -
Flags: review?(nfroyd)
Assignee | ||
Comment 28•6 years ago
|
||
Updated the commit message slightly to also include:
> * --disable-tests will imply not building geckodriver, but can
> be overridden using --enable-geckodriver as indicated above.
Assignee | ||
Updated•6 years ago
|
Attachment #9005983 -
Attachment is obsolete: true
Attachment #9005983 -
Flags: review?(nfroyd)
Assignee | ||
Updated•6 years ago
|
Attachment #9005984 -
Flags: review?(nfroyd)
Comment 29•6 years ago
|
||
(In reply to Andreas Tolfsen ❲:ato❳ from comment #24) > But I still need sign-off from a Marionette peer, since this is > very much impacting Marionette. How do I get that? I could just have a look at it once bug 1417646 has been fixed. (In reply to Andreas Tolfsen ❲:ato❳ from comment #26) > > > +To build geckodriver from [mozilla-central]: > > > > You reference `mozilla-unified` here, which means that you have to add at > > least a `hg up central` line before building geckodriver. Otherwise you > > end-up with whatever is on tip which can be anything of central, beta, > > release, or an esr branch, right? > > I don’t know. I understand the canonical repository for Firefox > is https://hg.mozilla.org/mozilla-unified/. No, mozilla-unified is a collection from all available repositories. The canonical repository patches should be based on is still mozilla-central, and that's where landed commits are getting merged to from mozilla-inbound and autoland. > Do we want to have VCS specific instructions here? AIUI the > documentation for that is > https://mozilla-version-control-tools.readthedocs.io/en/latest/. In that case it's always better to refer to external documentation. So lets better do that, yes. > > Also can you please refer to Building.md here to avoid all these duplication > > of content? > > CONTRIBUTING.md is valuable for as long as the project is exported > to GitHub, because GitHub picks up this file name convention in > various interesting places. Building.md is not exported is only > available on central. > > Do you mean we should remove the content from CONTRIBUTING.md and > link to firefox-source-docs.m.o instead? For maintenance it would indeed be better. I wouldn't expect to have to update contributing.md because of changes in Building.md or Testing.md. Also note that we do not regularly update those files, and if it causes extra churn. Given that the canonical repository for geckodriver is mozill-central now, a link to firefox-source-docs.m.o should be perfect.
Flags: needinfo?(hskupin)
Assignee | ||
Comment 30•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #29) > (In reply to Andreas Tolfsen ❲:ato❳ from comment #24) > > > But I still need sign-off from a Marionette peer, since this is > > very much impacting Marionette. How do I get that? > > I could just have a look at it once bug 1417646 has been fixed. Alright, I’ll re-request r? once that lands and this has been rebased. Thanks. > (In reply to Andreas Tolfsen ❲:ato❳ from comment #26) > > > > > +To build geckodriver from [mozilla-central]: > > > > > > You reference `mozilla-unified` here, which means that you > > > have to add at least a `hg up central` line before building > > > geckodriver. Otherwise you end-up with whatever is on tip > > > which can be anything of central, beta, release, or an esr > > > branch, right? > > > > I don’t know. I understand the canonical repository for Firefox > > is https://hg.mozilla.org/mozilla-unified/. > > No, mozilla-unified is a collection from all available > repositories. The canonical repository patches should be based > on is still mozilla-central, and that's where landed commits are > getting merged to from mozilla-inbound and autoland. It was my impression that the VCS team wanted developers to use unified for precisely the reason that you don’t need two or three different repositories checked out, largely duplicating eachother’s commits. Isn’t this worth recommending to new developers? Since it seems to be contentious to make this documentation change, I will revert it for the time being to get this patch accepted. > > Do you mean we should remove the content from CONTRIBUTING.md > > and link to firefox-source-docs.m.o instead? > > For maintenance it would indeed be better. I wouldn't expect to > have to update contributing.md because of changes in Building.md > or Testing.md. Also note that we do not regularly update those > files, and if it causes extra churn. Given that the canonical > repository for geckodriver is mozill-central now, a link to > firefox-source-docs.m.o should be perfect. We can make that change. I feel that it is a bit out of context to significantly change CONTRIBUTING.md as part of a changeset to build geckodriver in all build configs. I would prefer if the updates to this file were minimal, reflecting only what needs to change in order to land that change, and then follow up on de-duplicating content later.
Comment 31•6 years ago
|
||
(In reply to Andreas Tolfsen ❲:ato❳ from comment #30) > > > I don’t know. I understand the canonical repository for Firefox > > > is https://hg.mozilla.org/mozilla-unified/. > > > > No, mozilla-unified is a collection from all available > > repositories. The canonical repository patches should be based > > on is still mozilla-central, and that's where landed commits are > > getting merged to from mozilla-inbound and autoland. > > It was my impression that the VCS team wanted developers to use > unified for precisely the reason that you don’t need two or three > different repositories checked out, largely duplicating eachother’s > commits. Isn’t this worth recommending to new developers? Yes, we can or even better should refer to mozilla-unified, but after the checkout you have to make sure to update the the central branch. That's all. > I feel that it is a bit out of context to significantly change > CONTRIBUTING.md as part of a changeset to build geckodriver in all > build configs. I would prefer if the updates to this file were > minimal, reflecting only what needs to change in order to land that > change, and then follow up on de-duplicating content later. Do whatever is better.
Comment 32•6 years ago
|
||
Comment on attachment 9005984 [details] [diff] [review] Make building geckodriver mandatory. Review of attachment 9005984 [details] [diff] [review]: ----------------------------------------------------------------- Thanks! ::: toolkit/moz.configure @@ +917,4 @@ > > +option('--enable-geckodriver', default=geckodriver_default, > + when='--enable-compile-environment', > + help='Override default and attempt to build geckodriver') I still think this should just be "Build geckodriver", without mentioning anything about overriding defaults; it seems self-explanatory to me that the option overrides "defaults".
Attachment #9005984 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 33•6 years ago
|
||
Attachment #9005650 -
Attachment is obsolete: true
Attachment #9006273 -
Flags: review?(hskupin)
Assignee | ||
Comment 34•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #31) > (In reply to Andreas Tolfsen ❲:ato❳ from comment #30) > > > I feel that it is a bit out of context to significantly change > > CONTRIBUTING.md as part of a changeset to build geckodriver in > > all build configs. I would prefer if the updates to this file > > were minimal, reflecting only what needs to change in order to > > land that change, and then follow up on de-duplicating content > > later. > > Do whatever is better. Yes, so what I will do is the reverse: I’ve filed https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 to redirect people’s attention to firefox-source-docs.m.o and will fix that _before_ this lands, since this changeset is anyway waiting on
Assignee | ||
Comment 35•6 years ago
|
||
(In reply to Nathan Froyd [:froydnj] from comment #32) > ::: toolkit/moz.configure > @@ +917,4 @@ > > > > +option('--enable-geckodriver', default=geckodriver_default, > > + when='--enable-compile-environment', > > + help='Override default and attempt to build geckodriver') > > I still think this should just be "Build geckodriver", without mentioning > anything about overriding defaults; it seems self-explanatory to me that the > option overrides "defaults". Oh I see what you mean. That makes sense. I will fix this up before pushing. This changeset needs rebasing on top of https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 regardless.
Assignee | ||
Comment 36•6 years ago
|
||
(In reply to Andreas Tolfsen ❲:ato❳ from comment #34) > […] since this changeset is anyway waiting on https://bugzilla.mozilla.org/show_bug.cgi?id=1417646 (In reply to Andreas Tolfsen ❲:ato❳ from comment #35) > This changeset needs rebasing on top of > https://bugzilla.mozilla.org/show_bug.cgi?id=1488443 regardless. I really meant https://bugzilla.mozilla.org/show_bug.cgi?id=1417646 here as well.
Comment 37•6 years ago
|
||
Comment on attachment 9006273 [details] [diff] [review] Update geckodriver build instructions. Review of attachment 9006273 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/geckodriver/doc/Building.md @@ +12,2 @@ > > + % ./mach build testing/geckodriver You should still add `hg up central` before. Otherwise it will be built from whatever branch is on tip. @@ +43,2 @@ > [mozconfig]: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Configuring_Build_Options > [Firefox CI]: https://treeherder.mozilla.org/ I don't see that this is needed anymore. Maybe others too? ::: testing/geckodriver/doc/Testing.md @@ +49,5 @@ > + > + % ./mach wpt --webdriver-arg=-vv testing/web-platform/tests/webdriver > + > +[Web Platform Tests]: http://web-platform-tests.org/ > +[cargo]: http://doc.crates.io/guide.html I don't see it being used in this file.
Attachment #9006273 -
Flags: review?(hskupin) → review+
Assignee | ||
Comment 38•6 years ago
|
||
(In reply to Henrik Skupin (:whimboo) from comment #37) > ::: testing/geckodriver/doc/Building.md > @@ +12,2 @@ > > > > + % ./mach build testing/geckodriver > > You should still add `hg up central` before. Otherwise it will be > built from whatever branch is on tip. I thought we agreed not to have VCS specific information in this file? This documentation is kind of assuming that people know how to build the right thing; it is not meant for teaching people how to check out the right branches. Would it be better if I removed the reference to mozilla-central above? That way the docs would not make any assumption about from where you’re building.
Assignee | ||
Comment 39•6 years ago
|
||
Comment on attachment 9005651 [details] [diff] [review] Remove --enable-geckodriver override for linux32. Rendered obsolete by https://hg.mozilla.org/mozilla-central/rev/90096b1bb22a.
Attachment #9005651 -
Attachment is obsolete: true
Assignee | ||
Comment 40•6 years ago
|
||
Attachment #9006888 -
Flags: review?(hskupin)
Assignee | ||
Updated•6 years ago
|
Attachment #9006273 -
Attachment is obsolete: true
Assignee | ||
Comment 41•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=99c19ab72a9236be16a5bdbed3f3f90bd59fb0b7
Assignee | ||
Comment 42•6 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1a4fc3a8d2c9b61d905610f2918415ee935f1f2c
Comment 43•6 years ago
|
||
(In reply to Andreas Tolfsen ❲:ato❳ from comment #38) > I thought we agreed not to have VCS specific information in this > file? This documentation is kind of assuming that people know how > to build the right thing; it is not meant for teaching people how > to check out the right branches. Sorry, looks like that I forgot it. > Would it be better if I removed the reference to mozilla-central > above? That way the docs would not make any assumption about from > where you’re building. Maybe do that and just reference the documentation which is about cloning the repository.
Assignee | ||
Comment 44•6 years ago
|
||
Third time lucky try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=091fc13c9938781934aa1032a96e2595ae0dcf2c
Updated•6 years ago
|
Attachment #9006888 -
Flags: review?(hskupin) → review+
Comment 45•6 years ago
|
||
Pushed by ato@sny.no: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8ee11194f69 build: make asan depend on --help; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/3a859a6f0720 toolkit: make building geckodriver mandatory; r=froydnj https://hg.mozilla.org/integration/mozilla-inbound/rev/3b6c97dfb586 geckodriver: update geckodriver build instructions; r=whimboo
Comment 46•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c8ee11194f69 https://hg.mozilla.org/mozilla-central/rev/3a859a6f0720 https://hg.mozilla.org/mozilla-central/rev/3b6c97dfb586
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in
before you can comment on or make changes to this bug.
Description
•