Closed Bug 1413195 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(firefox58 fixed)

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: kats, Assigned: ato)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Happened somewhere in https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a89e5587c7a7&tochange=083a9c84fbd0, almost certainly bug 1412037.
Blocks: 1412037
Flags: needinfo?(james)
Flags: needinfo?(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
Flags: needinfo?(ato)
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.
Flags: needinfo?(james) → needinfo?(ted)
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?
Flags: needinfo?(ted)
Good question!
Flags: needinfo?(ato)
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.
(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.
Assignee: nobody → ato
Status: NEW → ASSIGNED
Flags: needinfo?(ato)
OS: Unspecified → All
Hardware: Unspecified → All
Comment on attachment 8925005 [details]
Bug 1413195 - Vendor webdriver's dependencies through geckodriver

https://reviewboard.mozilla.org/r/196252/#review201884
Attachment #8925005 - Flags: review?(james) → review+
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?
Attachment #8925006 - Flags: review?(james) → review+
(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.
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.
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.
(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.
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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
Attachment #8925006 - Attachment is obsolete: true
Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/783faae49328
Vendor webdriver's dependencies through geckodriver r=jgraham
https://hg.mozilla.org/mozilla-central/rev/783faae49328
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: