Closed Bug 1776264 Opened 2 years ago Closed 2 years ago

Release webdriver 0.46.0 crate for updated (warp, hyper, tokio) depencencies

Categories

(Testing :: geckodriver, task, P1)

Default
task
Points:
1

Tracking

(firefox104 fixed)

RESOLVED FIXED
104 Branch
Tracking Status
firefox104 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

References

Details

(Whiteboard: [webdriver:m4][webdriver-external])

Attachments

(3 files)

The webdriver crate got a couple of dependency updates recently. All of these can be found on bug 1710421. Given that major updates are included we might want to consider releasing an out-of-band release of just the webdriver crate.

Beside the updates we also got additions to the action input types via bug 1773265.

Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Release webdriver crate for updated depencencies → Release webdriver 0.46.0 crate for updated (warp, hyper, tokio) depencencies

After discussing this topic with the team I'm going to add this bug to the M4 milestone because of its importance for external users of the crate.

Points: --- → 1
Priority: -- → P1
Whiteboard: [webdriver:m4][webdriver-external]
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b5a663fb2ffd
[webdriver] Release version 0.46.0. r=webdriver-reviewers,jgraham

Backed out for causing lint failure. CLOSED TREE

Backout link
Push with failures
Link to failure log

So the backout here seems to be related to the changes in bug 1776096 which require the local crate version to match the crates.io version number.

The process we typically use for publishing geckodriver and related crates is to update the version numbers in-tree but delay publishing to crates.io until the relevant commit actually reaches central, in case something is backed out or requires additional changes. Does this new auditing mean that we have to publish to crates.io before pushing to autoland, or is there some other approach we should be using?

Flags: needinfo?(bholley)

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

So the backout here seems to be related to the changes in bug 1776096 which require the local crate version to match the crates.io version number.

I would characterize the requirement as: "dependencies must be audited, unless they have a legacy exemption". webdriver 0.45.0 has such an exemption, but 0.46.0 does not, hence the error.

It's worth calling out that something like webdriver is a bit of a special case. It's really first-party code, but I've marked it as audit-as-crates-io in order to nudge us into certifying it in our audit set alongside the crates.io release (which allows others in the ecosystem to mechanically verify that it carries mozilla's stamp of approval).

The process we typically use for publishing geckodriver and related crates is to update the version numbers in-tree but delay publishing to crates.io until the relevant commit actually reaches central, in case something is backed out or requires additional changes. Does this new auditing mean that we have to publish to crates.io before pushing to autoland, or is there some other approach we should be using?

It seems prudent to make sure the code itself sticks before publishing the crates.io release, but the patch in comment 3 just bumps the version, which I have trouble imagining bouncing for a reason that would invalidate the published copy. Can you think of one? If not, my tentative recommendation would be to (1) fully land all of the code for the new version in m-c and make sure it sticks, (2) write a patch to bump the version, (3) cargo publish, (4) cargo vet certify CRATE VERSION, (5) push to phab.

(note that (4) has to come after (3), because cargo-vet should prevent you from certifying a package that doesn't exist).

Flags: needinfo?(bholley)

The reason we've tried to publish after the code has reached m-c before is that offers stronger guarantees that the code you published is the same as the code corresponding to the commit where the version number was updated. In particular there's not really a mechanism to ensure that no code lands in the crate between running cargo publish and the subsequent phab patch reaching autoland. That could, for example, be code that was reviewed as part of another changeset, that wasn't required for the release, and just happened to be pushed at the wrong time by someone who had no idea there was a release in progress.

So I think the process you suggest will normally work (and the detail of the steps required are helpful, thanks), but it does seem to open up race conditions that previously weren't possible.

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

The reason we've tried to publish after the code has reached m-c before is that offers stronger guarantees that the code you published is the same as the code corresponding to the commit where the version number was updated.

Is this an important property to preserve?

It's what we've previously used to identify all the commits since the previous release, so that we can make the semver-correct update to the version number and identify any changes that require release notes.

Note that for the geckodriver release notes we would use the changeset from the mozilla-central repository which actually builds and signs the geckodriver binaries. And since the last release we also push geckodriver itself as crate to crates.io. So the recent changes could indeed cause problems with race conditions as James previously mentioned.

And I forgot to also hand out the steps that we use to create a geckodriver release:
https://firefox-source-docs.mozilla.org/testing/geckodriver/Releasing.html

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

It's what we've previously used to identify all the commits since the previous release, so that we can make the semver-correct update to the version number and identify any changes that require release notes.

Seems like we could store that information somewhere else.

In any case though, the current setup is just intended as a nudge to publish and not an important security invariant. If it's disruptive to your workflow we could alternatives just mark these crates as audit-as-crates-io=false and just have the release process include a step to certify the release in the m-c audit store. Would that be preferable?

OK, I think Henrik's point about geckodriver is actually the strongest argument here; for geckodriver the fact that we upload binaries that are built from mozilla-central, and want the release version on crates.io to be the same code as the binary from central, means that we need to do both releases simultaneously.

So I'll make a patch to update supply-chain/config.toml and update our release process to ensure that the cargo vet certify step is documented.

Depends on: 1776649
Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/8f2c396c0d10
[webdriver] Release version 0.46.0. r=webdriver-reviewers,jgraham
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → 104 Branch

With landing the changes the release hasn't been done yet. As such I pushed the new version of the crate to crates.io:

Uploading webdriver v0.46.0 (/Users/henrik/code/gecko/testing/webdriver)

Sadly I'm not able to audit the new version (cargo vet 0.46.0 didn't even work at all):

$ mach cargo vet certify webdriver 0.46.0
  Γ— 'webdriver' isn't one of your foreign packages

James, can you help here?

Flags: needinfo?(james)

Not really. AFAICT the cargo vet code will only allow auditing things are as "third party", which seems to amount to either having the audit-as-crates-io flag set, or being sourced from crates.io. Since we had to unset the audit-as-crates-io flag in order to allow our build-before-publish workflow, it seems like we're unable to publish audits for the package. I'm not sure if this is just a scenario that hasn't been considered for cargo vet or if this is by-design.

Flags: needinfo?(james) → needinfo?(bholley)

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

Not really. AFAICT the cargo vet code will only allow auditing things are as "third party", which seems to amount to either having the audit-as-crates-io flag set, or being sourced from crates.io. Since we had to unset the audit-as-crates-io flag in order to allow our build-before-publish workflow, it seems like we're unable to publish audits for the package. I'm not sure if this is just a scenario that hasn't been considered for cargo vet or if this is by-design.

That's an oversight. The certify subcommand tries to put things in context of the project's actual third-party dependencies, but that doesn't make sense here. We'll add a --force option today and then pull that in in bug 1777586.

Flags: needinfo?(bholley)

Thanks Bobby! So in case of the webdriver crate which exact command would we have to run after publishing our crate to crates.io? Or won't we have to do anything and can remove the cargo vet step in our releasing documentation for geckodriver?

Flags: needinfo?(bholley)

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

Thanks Bobby! So in case of the webdriver crate which exact command would we have to run after publishing our crate to crates.io? Or won't we have to do anything and can remove the cargo vet step in our releasing documentation for geckodriver?

The exact command is the one you tried in comment 16 (./mach cargo vet webdriver VERSION), which should work now.

Flags: needinfo?(bholley)

Thanks Bobby! Reopening the bug for the remaining patches to land.

Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Depends on D151323

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d04958de1178
[webdriver] Cargo vet 0.46.0 release. r=supply-chain-reviewers,bholley
https://hg.mozilla.org/integration/autoland/rev/69864a2b6a8d
[geckodriver] Update releasing documentation for auditing step. r=webdriver-reviewers,jgraham
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: