Combine build configurations for Marionette and Remote Protocol into WebDriver
Categories
(Remote Protocol :: Agent, task, P2)
Tracking
(firefox90 fixed)
Tracking | Status | |
---|---|---|
firefox90 | --- | fixed |
People
(Reporter: whimboo, Assigned: whimboo)
References
(Blocks 1 open bug)
Details
(Whiteboard: [bidi-m1-mvp])
Attachments
(2 files)
With the goal to have not only have our partly CDP implementation but also WebDriver BiDi and Marionette under /remote
, we should update the build config:
- Our upcoming implementation for BiDi has to work for all Gecko based applications similar to what Marionette supports. As such restricting the Remote Agent to Firefox only will not work:
-
We should check if we can move parts of
/remote/moz.build
that is only related to CDP to/remote/cdp/moz.build
and make that instead dependent on--enable-cdp
. This would also allow us to move Marionette code to/remote/marionette
and build it independently from the Remote Agent. -
Maybe we could combine both
--enable-cdp
and--enable-marionette
to a single build config like--enable-webdriver
, or keep--enable-cdp
and let it handle the inclusion of CDP only.
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Comment 1•3 years ago
|
||
(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)
- Maybe we could combine both
--enable-cdp
and--enable-marionette
to a single build config like--enable-webdriver
, or keep--enable-cdp
and let it handle the inclusion of CDP only.
Actually meanwhile I'm in favor of this option. With --enable-webdriver
we can get both components means Marionette and Remote Agent built, and with --enable-cdp
(we may want to rename it to --disable-cdp
) our partial CDP support could be easily turned on/off. This can help products that don't want to have CDP support included, but are interested in WebDriver BiDi.
With such a build configuration in place specific CDP related code can then be excluded via if conditions if not wanted.
Maybe we should do this change at the same time with bug 1693993 so that we don't have to care about the /testing/marionette/
folder anymore for the server related code.
James and Julian, do you have any kind of feedback, or does that sound fine with you?
Assignee | ||
Updated•3 years ago
|
Assignee | ||
Updated•3 years ago
|
Comment 3•3 years ago
|
||
I don't have a strong opinion here; I'd be happy with a single flag for everything or a single flag for WebDriver and one for CDP. In practice I think we're unlikely to be in a state where we are not building CDP for Firefox, but the code is still in m-c so if it turns out to be significantly easier to just have a single flag we should do that. Otherwise having two sounds fine.
Comment 4•3 years ago
|
||
I don't have a strong opinion either, but I have a few questions :)
Considering that marionette is used by many test harness for Firefox, will we have to pass --enable-webdriver
anywhere we want to run tests? If yes, I wonder if it's actually worth exposing, people might wonder why they have to enable webdriver just for running regular test suites.
Maybe it would to have a few examples of how --enable-webdriver and --enable-cdp would be used (eg for Firefox on various channels, for Fenix, for Thunderbird,...)?
Assignee | ||
Comment 5•3 years ago
•
|
||
(In reply to Julian Descottes [:jdescottes] from comment #4)
Considering that marionette is used by many test harness for Firefox, will we have to pass
--enable-webdriver
anywhere we want to run tests? If yes, I wonder if it's actually worth exposing, people might wonder why they have to enable webdriver just for running regular test suites.
Oh, no. Sorry when I was unclear. These are actually build config flags that you set via ac_add_options
. But I was also wrong with the names. So it should be --disable-webdriver
and --disable-cdp
, with both enabled by default. If specified in a mozconfig
the appropriate files won't be added to the build. Hereby CDP would require WebDriver to be enabled.
Here an example for e.g Thunderbird:
ac_add_options --enable-application=comm/mail
ac_add_options --enable-debug
ac_add_options --disable-cdp
Starting Firefox once build is done as usual. Only with the --remote-debugging-port
command line argument the Remote Agent component will be startup. Geckodriver will do that transparently.
I hope that clarifies the confusion that you had.
Comment 6•3 years ago
|
||
Thanks for the added info, having --disable-webdriver/cdp as opt-in build flags sounds good to me then!
Assignee | ||
Comment 7•3 years ago
|
||
Greg, mind telling me what the usage of the following code is? Can we just remove the remote agent constant, or would the webdriver one be needed? I don't see ENABLE_MARIONETTE
that's why I'm asking.
Assignee | ||
Comment 8•3 years ago
|
||
The changes as covered by this bug are actually required to have a common code base for Marionette and Remote Agent. As such it has to be a P2.
Comment 9•3 years ago
|
||
Yup, you can remove it without any issues. That data is for testing our metrics processing in mozperftest.
Assignee | ||
Comment 10•3 years ago
|
||
I pushed a complete patch to try:
https://treeherder.mozilla.org/jobs?repo=try&revision=31293da5722ae492611b3e6de3e2943796f66ac3
For the reviews I will split it apart into several revisions given that some pre-work needs to be done across different components.
Assignee | ||
Comment 11•3 years ago
|
||
Assignee | ||
Comment 12•3 years ago
|
||
Depends on D115583
Comment 13•3 years ago
|
||
Pushed by hskupin@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/21b0c552bf0c Combine build flags --disable-marionette and --enable-cdp as --disable-webdriver. r=firefox-build-system-reviewers,Gijs,smaug,keeler,jdescottes,glandium https://hg.mozilla.org/integration/autoland/rev/9226fc524286 Update Marionette and Remote Agent docs for --disable-webdriver build flag. r=remote-protocol-reviewers,marionette-reviewers,jdescottes
Comment 14•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/21b0c552bf0c
https://hg.mozilla.org/mozilla-central/rev/9226fc524286
Assignee | ||
Updated•3 years ago
|
Description
•