Closed Bug 1368265 Opened 7 years ago Closed 7 years ago

Import and build webdriver crate

Categories

(Testing :: geckodriver, defect, P2)

Version 3
defect

Tracking

(firefox57 fixed)

RESOLVED FIXED
mozilla57
Tracking Status
firefox57 --- fixed

People

(Reporter: ato, Assigned: ato)

References

(Blocks 1 open bug)

Details

Attachments

(6 files, 1 obsolete file)

We want to import the git history and build the webdriver Rust crate in mozilla-central: https://github.com/mozilla/webdriver-rust

We will then tombstone the GitHub project repository and (maybe later?) hook it up as a dependency to testing/geckodriver.
Priority: -- → P2
Assignee: nobody → ato
Status: NEW → ASSIGNED
Hm, what will be the target directory for the source? I feel that we should not cluttering /testing/ that much, given that there are also other rust related repos still in question to be imported too.
(In reply to Henrik Skupin (:whimboo) from comment #1)
> Hm, what will be the target directory for the source? I feel that we should
> not cluttering /testing/ that much, given that there are also other rust
> related repos still in question to be imported too.

I intend to put it under testing/geckodriver and change
testing/geckodriver/Cargo.toml’s webdriver entry to point to ../geckodriver.

I don’t know what you mean about “cluttering” the testing directory.  Can you
expand?  What is a better directory?
With cluttering I meant that we might not want to put every rust repository under /testing. So I assume you meant to have something like testing/geckodriver/webdriver? If that is the case you might want to think about using a workspace in your Cargo.toml. Also because I assume that geckodriver is the only consumer of webdriver-rust?
(In reply to Henrik Skupin (:whimboo) from comment #3)

> With cluttering I meant that we might not want to put every rust
> repository under /testing.

Irregardless what programming language is used, code should live where
it is most appropriate.  If there are good reasons that speak for
putting code in a sub-directory, then we should listen to those.

> So I assume you meant to have something like
> testing/geckodriver/webdriver? If that is the case you might want
> to think about using a workspace in your Cargo.toml. Also because I
> assume that geckodriver is the only consumer of webdriver-rust?

The webdriver crate is a separate library also used in Servo.  Whilst it
is theoretically possible to put it in testing/geckodriver/webdriver,
I don’t think a cargo workspace has any practical benefit except
inflating the number of directory levels you need to traverse to access
the code?
(In reply to Andreas Tolfsen ‹:ato› from comment #4)
> The webdriver crate is a separate library also used in Servo.  Whilst it
> is theoretically possible to put it in testing/geckodriver/webdriver,
> I don’t think a cargo workspace has any practical benefit except
> inflating the number of directory levels you need to traverse to access
> the code?

It might make it easier for our build config so only one project has to be specified, and the other is just build in tandem. Both would also use the exact same dependencies, as specified in the top-level Cargo.lock. Also aren't both closely related?

If it doesn't make sense feel free to drop my comments.
(In reply to Henrik Skupin (:whimboo) from comment #5)
> (In reply to Andreas Tolfsen ‹:ato› from comment #4)
> > The webdriver crate is a separate library also used in Servo.  Whilst it
> > is theoretically possible to put it in testing/geckodriver/webdriver,
> > I don’t think a cargo workspace has any practical benefit except
> > inflating the number of directory levels you need to traverse to access
> > the code?
> 
> It might make it easier for our build config so only one project has to be
> specified, and the other is just build in tandem. Both would also use the
> exact same dependencies, as specified in the top-level Cargo.lock. Also
> aren't both closely related?

As far as I understand, no special changes need to be made to the build config.
We just need to tell geckodriver to pick up the webdriver dependency from
correct relative path.

From the point of the build system, the target is geckodriver, and it’s up to
cargo where it finds the dependencies.  Whether it finds the dependencies in
/third_party/rust as it does now, or from somewhere else, is irrelevant.
(In reply to Andreas Tolfsen ‹:ato› from comment #0)
> it up as a dependency to testing/geckodriver.

I filed bug 1369086 to get this component created.
Component: Marionette → geckodriver
I’ve now linearised the git repository history
from https://github.com/mozilla/webdriver-rust.git
and overlaid it onto mozilla-inbound like we did for
testing/geckodriver, based on gps’ instructions in
https://bugzilla.mozilla.org/show_bug.cgi?id=1340637#c58.

The result of this is now available at
https://hg.mozilla.org/users/atolfsen_mozilla.com/gecko/.  Because
I’m unable to push this to inbound myself (“Rev […] needs "Bug
N" or "No bug" in the commit message” error), I need help to put
these commits into upstream.  Could you do the honours or let me
know how to bypass that restriction?

The graphically annotated log can be viewed
at

	https://gist.githubusercontent.com/andreastt/a08f0829986aed65163ea10476b1207b/raw/66069e8a58a410d745006d71b0bc159f2e783da0/gistfile1.txt
Flags: needinfo?(gps)
For posterity I’ve documented the steps to linearise and overlay a
git repository into mozilla-central:

> % cd /home/ato/src
> % git clone hg::https://hg.mozilla.org/hgcustom/version-control-tools
> % cat <<EOF >>/home/ato/.hgrc
> [extensions]
> overlay = ~/src/version-control-tools/hgext/overlay
> EOF
> % cd version-control-tools
> % ./create-environment vcssync
> % source venv/vcssync/bin/activate
> % cd /tmp
> % linearize-git-to-hg --sumary-prefix 'webdriver:' \
> 	--source-repo-key Source-Repository \
> 	--source-revision-key Source-Revision \
> 	--use-p2-author \
> 	--copy-similarity 75 \
> 	--find-copies-harder \
> 	https://github.com/mozilla/webdriver-rust \
> 	master \
> 	/tmp/webdriver-rust-git \
> 	/tmp/webdriver-rust-hg
> % hg clone https://hg.mozilla.org/integration/mozilla-inbound
> % hg --cwd webdriver-rust-hg serve &
> [1] 28156
> % cd mozilla-inbound
> % hg overlay --into testing/webdriver http://tristan.corp.lon2.mozilla.com:8000
> % hg push -f ssh://hg.mozilla.org/users/atolfsen_mozilla.com/gecko
You can add "IGNORE BAD COMMIT MESSAGES" to the commit that is pushed. Alternatively, we could merge this repo into mozilla-central by introducing a new root commit in the repo. That is what we did with Servo. The latter requires us to add the root commit's SHA-1 to the server config on hg.mo.
Flags: needinfo?(gps)
(In reply to Gregory Szorc [:gps] from comment #10)

> You can add "IGNORE BAD COMMIT MESSAGES" to the commit that
> is pushed.  Alternatively, we could merge this repo into
> mozilla-central by introducing a new root commit in the repo. That
> is what we did with Servo. The latter requires us to add the root
> commit's SHA-1 to the server config on hg.mo.

Thanks for the instructions!

Because whimboo had landed a few commits to the git repository after
I did the conversion process, I re-did the steps above with the
slight alteration s/Source-Repository/Source-Repo/ as source repo
key.

Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=a12424febc5e41c5d52d77fb01f6b86f0150f6cd
Treeherder: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=a12424febc5e41c5d52d77fb01f6b86f0150f6cd
Comment on attachment 8904042 [details]
Bug 1368265 - Use in-tree testing/webdriver.

https://reviewboard.mozilla.org/r/175772/#review180986
Attachment #8904042 - Flags: review?(dburns) → review+
Comment on attachment 8904043 [details]
Bug 1368265 - Remove third_party/rust/webdriver.

https://reviewboard.mozilla.org/r/175774/#review180988
Attachment #8904043 - Flags: review?(dburns) → review+
Comment on attachment 8904044 [details]
Bug 1368265 - Update geckodriver and webdriver Cargo.tomls.

https://reviewboard.mozilla.org/r/175776/#review180990
Attachment #8904044 - Flags: review?(dburns) → review+
Comment on attachment 8904045 [details]
Bug 1368265 - Provide a webdriver README.

https://reviewboard.mozilla.org/r/175778/#review180992

::: testing/webdriver/README.md:83
(Diff revision 1)
> +	        …
> +	    }
> +	}
> +
> +    let extension_routes = vec![
> +        Method::Get, "/session/{sessionId}/moz/hello",

Is there a missing `(` here? The line below has an unbalanced `)`
Attachment #8904045 - Flags: review?(dburns) → review+
Comment on attachment 8904046 [details]
Bug 1368265 - Remove unnecessary license files.

https://reviewboard.mozilla.org/r/175780/#review180994
Attachment #8904046 - Flags: review?(dburns) → review+
Comment on attachment 8904047 [details]
Bug 1368265 - Add testing/webdriver hgignore file.

https://reviewboard.mozilla.org/r/175782/#review180996
Attachment #8904047 - Flags: review?(dburns) → review+
Comment on attachment 8904048 [details]
Bug 1368265 - Remove outdated contribution advice for geckodriver.

https://reviewboard.mozilla.org/r/175784/#review181000

I can't seem to find contribution advice for Geckodriver on Github or MDN only usage advice.
Attachment #8904048 - Flags: review?(dburns) → review-
Comment on attachment 8904045 [details]
Bug 1368265 - Provide a webdriver README.

https://reviewboard.mozilla.org/r/175778/#review180992

> Is there a missing `(` here? The line below has an unbalanced `)`

Well spotted.
Attachment #8904048 - Attachment is obsolete: true
(In reply to David Burns :automatedtester from comment #25)

> Comment on attachment 8904048 [details] Bug 1368265 - Remove outdated
> contribution advice for geckodriver.
> 
> https://reviewboard.mozilla.org/r/175784/#review181000
> 
> I can't seem to find contribution advice for Geckodriver on Github
> or MDN only usage advice.

Most of the information in testing/geckodriver/CONTRIBUTING.md
concerns checking out the repository from GitHub and working with
that.  None of that is relevant now that the project lives in m-c.

The first section about filing good issue reports arguably is,
however.  So in the interests of landing this I have dropped this
commit and will file a follow-up to correct the information in
CONTRIBUTING.md.
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/29486614c67c
Use in-tree testing/webdriver. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/82ee54d395bb
Remove third_party/rust/webdriver. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/ad94237d3b32
Update geckodriver and webdriver Cargo.tomls. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/4d2853618098
Provide a webdriver README. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/04786fb4329f
Remove unnecessary license files. r=automatedtester
https://hg.mozilla.org/integration/autoland/rev/fb3c4b567fe2
Add testing/webdriver hgignore file. r=automatedtester
You need to log in before you can comment on or make changes to this bug.