Closed Bug 1695031 Opened 3 years ago Closed 3 years ago

Combine build configurations for Marionette and Remote Protocol into WebDriver

Categories

(Remote Protocol :: Agent, task, P2)

task
Points:
8

Tracking

(firefox90 fixed)

RESOLVED FIXED
90 Branch
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:

  1. 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:

https://searchfox.org/mozilla-central/rev/6f8f3d0e9022b6f0405da26ec940a89455416202/toolkit/moz.configure#1057-1058

  1. 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.

  2. 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.

Blocks: 1690252
Points: --- → 13
Priority: -- → P3

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

  1. 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?

Flags: needinfo?(jdescottes)
Flags: needinfo?(james)
Depends on: 1706216, 1676803
Summary: Update build configuration for Remote Protocol → Update build configuration for Remote Protocol for WebDriver BiDi (Remote Agent, Marionette)

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.

Flags: needinfo?(james)

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,...)?

Flags: needinfo?(jdescottes)

(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.

Thanks for the added info, having --disable-webdriver/cdp as opt-in build flags sounds good to me then!

Blocks: 1693803
Depends on: 1710839

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.

https://searchfox.org/mozilla-central/rev/443f87caa5fadba920b0382e12874693d6c6133a/python/mozperftest/mozperftest/tests/data/browsertime-results-video/browsertime.json#46

Flags: needinfo?(gmierz2)

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.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Priority: P3 → P2
Summary: Update build configuration for Remote Protocol for WebDriver BiDi (Remote Agent, Marionette) → Combine build configurations for Marionette and Remote Protocol into WebDriver

Yup, you can remove it without any issues. That data is for testing our metrics processing in mozperftest.

Flags: needinfo?(gmierz2)

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.

Blocks: 1711931
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → 90 Branch
See Also: → 1712801
Points: 13 → 8
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: