1413195 - Running `mach vendor rust` produces a testing/webdriver/Cargo.lock file

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

At some point in the last few days, running `mach vendor rust` has started to produce a testing/webdriver/Cargo.lock file. I don't know if this is expected or what but it wasn't happening before. Presumably whoever caused this to happen should either check it in, or make sure it doesn't happen, or add it to the .hgignore/.gitignore lists, or something else.

I'll narrow down the range in which this started happening.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Happened somewhere in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a89e5587c7a7&tochange=083a9c84fbd0, almost certainly bug 1412037.

Comment 2

[(not currently active) Ted Mielczarek]

Yeah:
https://hg.mozilla.org/mozilla-central/rev/540273e5cf22#l1.12

Comment 3

[Andreas Tolfsen ❲:ato❳]

We need to include testing/webdriver in
python/mozbuild/mozbuild/vendor_rust.py [1] so that "./mach vendor
rust" picks it up when updating Rust dependencies.  The webdriver
crate doesn’t have Cargo.lock checked in because it is a library
and not a standalone executable.  I don’t know what we should do
about the generated Cargo.lock file as a result of this.

[1] https://searchfox.org/mozilla-central/source/python/mozbuild/mozbuild/vendor_rust.py

Comment 4

[Kartikaya Gupta (email:[e-mail])]

I think in this case it makes sense to check it in. AIUI the normal rationale for not committing Cargo.lock files for libraries is so that downstream users don't get conflicts with particular versions but in this case since we're vendoring the dependencies into m-c we should in fact lock them to the versions that we have in-tree. The build peers should probably make the call though.

Comment 5

[(not currently active) Ted Mielczarek]

The geckodriver crate, which does build a binary, references webdriver as a path dependency:
https://dxr.mozilla.org/mozilla-central/rev/ee21e5f7f1c1726e0ed2697eb45df54cdceedd36/testing/geckodriver/Cargo.toml#28

and geckodriver is already listed in vendor_rust and has a Cargo.lock file checked in. Why do we separately need to vendor the dependencies of webdriver? Surely they should all be accounted for via its use in geckodriver?

Comment 6

[Kartikaya Gupta (email:[e-mail])]

Good question!

Comment 7

[Kartikaya Gupta (email:[e-mail])]

After a bit of experimentation, I think ted is right, we don't need the webdriver crate listed explicitly. AIUI your goal was to update some of the vendored dependencies but the correct way to do that is to run `cargo update` in testing/geckodriver before running `mach vendor rust`. If you want to only update some dependencies you can run `cargo update -p crate1 -p crate2 ...` instead.

Comment 8

[Andreas Tolfsen ❲:ato❳]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #5)

> Why do we separately need to vendor the dependencies of
> webdriver? Surely they should all be accounted for via its use in
> geckodriver?

I thought so too initially, but I must have done something wrong
when I released a new version of webdriver because "./mach vendor
rust" barfed and I mistakenly took this for a lacking entry in
python/mozbuild/mozbuild/vendor_rust.py.

I followed up on kats excellent advice and removed
"testing/webdriver" from vendor_rust.py, ran "./mach vendor rust"
again, and it appears to build just fine.

I’m also sorry about my inaccurate comment earlier.  I will submit
a patch that rectifies this.

Comment 9

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1413195 - Vendor webdriver's dependencies through geckodriver

The webdriver crate's dependencies are accounted for through geckodriver's
Cargo.lock file, because the webdriver dependency is given by path.

This commit reverts 540273e5cf22 made in bug 1412037.

Review commit: https://reviewboard.mozilla.org/r/196252/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196252/

Comment 10

[Andreas Tolfsen ❲:ato❳]

Attachment: Bug 1413195 - Un-vendor separate webdriver dependencies

Review commit: https://reviewboard.mozilla.org/r/196254/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/196254/

Comment 11

[James Graham [:jgraham]]

Comment on attachment 8925005 [details]
Bug 1413195 - Vendor webdriver's dependencies through geckodriver

https://reviewboard.mozilla.org/r/196252/#review201884

Comment 12

[James Graham [:jgraham]]

Comment on attachment 8925006 [details]
Bug 1413195 - Un-vendor separate webdriver dependencies

https://reviewboard.mozilla.org/r/196254/#review201886

Will this come back if we cargo upgrade geckodriver?

Comment 13

[Kartikaya Gupta (email:[e-mail])]

(In reply to James Graham [:jgraham] from comment #12)
> Will this come back if we cargo upgrade geckodriver?

If you `cargo update` geckodriver and then re-run `mach vendor rust`, you'll get updated dependencies. You won't get webdriver's Cargo.lock file. It's not clear what "this" in your question refers to.

Comment 14

[James Graham [:jgraham]]

It refers to the general set of changes in that review. Without the context of mozreview the comment probably  makes less sense :) I guess the underlying question for ato is whether it would be simpler just to run cargo update for geckodriver rather than removing the deps now and readding them at some later point.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

It looks like the geckodriver update already happened, in bug 1414254. So really this second patch is obsolete and in need of rebasing anyway, and your question is moot.

But assuming that hadn't happened, I would have argued that "removing the deps" is still valuable because most of the deps that are being removed in this second patch are duplicated packages anyway. For example, this patch is removing hyper-0.10.10, which you can tell is a duplicated package because it has the version number in the folder name. There is another copy (a newer version, 0.10.13) of hyper at third_party/rust/hyper/. So if this second patch landed, and then we did a cargo update for geckodriver, the geckodriver dependency tree would update to use 0.10.13, and then it wouldn't re-add the second copy of hyper, but instead use the one that's already in tree. That is exactly what happened in bug 1414254. To put it another way, the deps getting removed here (mostly) wouldn't be readded back at some later point because the things getting removed are just older versions of packages for which we already have a newer version in the tree. Sort of confusing, but I hope that makes sense.

Comment 16

[Andreas Tolfsen ❲:ato❳]

(In reply to James Graham [:jgraham] from comment #14)

> It refers to the general set of changes in that review. Without
> the context of mozreview the comment probably makes less sense
> :) I guess the underlying question for ato is whether it would
> be simpler just to run cargo update for geckodriver rather than
> removing the deps now and readding them at some later point.

As kats explained, we _are_ running "cargo update" for geckodriver.
I think in retrospect my problem was that after I bumped the
webdriver crate version, I failed to "cargo update" geckodriver’s
Cargo.lock file, thus causing a build error because its lockfile was
out of date with the path dependency version.

Comment 17

[James Graham [:jgraham]]

I see. Thanks for the explanation. I agree that removing old versions of things is good; I just wanted to be sure we weren't going to do some silly remove and readd dance.

Comment 18

[Andreas Tolfsen ❲:ato❳]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment
#15)

> It looks like the geckodriver update already happened, in bug
> 1414254. So really this second patch is obsolete and in need of
> rebasing anyway, and your question is moot.

Yes, thanks for pointing this out.  It is gone following a rebase.

> But assuming that hadn't happened, I would have argued
> that "removing the deps" is still valuable because most of
> the deps that are being removed in this second patch are
> duplicated packages anyway. For example, this patch is removing
> hyper-0.10.10, which you can tell is a duplicated package because
> it has the version number in the folder name.

Well you’re right, but it’s not always the case that a
duplicated crate can be removed if the crates depending on it are
too specific with their versioning.  I don’t know OTOH but I think
there are some cases of that.

> Sort of confusing, but I hope that makes sense.

No I think I got it all, but let me know if I didn’t.  Thanks for
explaining!

Comment 19

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8925005 [details]
Bug 1413195 - Vendor webdriver's dependencies through geckodriver

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/196252/diff/1-2/

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/783faae49328
Vendor webdriver's dependencies through geckodriver r=jgraham

Comment 21

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/783faae49328