2035541 - Update ANGLE's vendoring process to make use of gn_processor.py to generate build scripts

Status: RESOLVED FIXED

(Core :: Graphics, task)

Description

[Jamie Nicol [:jnicol]]

Currently angle is vendored into tree using the update-angle.py script. This must be run on Windows and from an angle checkout. Additionally you must have already run gclient sync to fetch all of angle's dependencies.

The script generates a GN config for the build, queries a list of all the required files and copies them to gfx/angle/checkout. It also generates the moz.build files, and we plan to replace that part of it in bug 2035532. Perhaps we should also replace the vendoring part of the script with a more more vanilla "mach vendor" process.

There are some unresolved questions to figure out:

• Do we continue vendoring from our fork (https://github.com/mozilla/angle), or instead vendor directly from the upstream repository and use the moz.yaml's patches field to apply our patches?
  • The fork perhaps makes manual rebases somewhat easier
  • But on the other hand it might be easier/quicker to land security fixes without having to first push to a public repo
• How do we fetch ANGLE's dependencies?
  • The current system relies on you running gclient sync in the angle checkout prior to running the update script.
  • From my investigations so far there are a few dependencies required to build the current setup, and there will be a few more if we want to build the Vulkan backend on Android too (bug 1984486).
  • So that's roughly 6 dependencies in total. That's enough that maintaining a separate moz.yaml and vendoring each manually, ensuring their versions remain in sync would be a pain
  • The ANGLE repository has its dependencies as git submodules. Codex suggested we using a custom vendor host, and override upstream_snapshot() to fetch the required submodules and add them to the archive.

Comment 1

[Jamie Nicol [:jnicol]]

Tom, do you have any thoughts on how a more standard vendoring process for angle would look? Specifically interested in answers to the questions outlined above, but also any other thoughts/guidance you may have.

Comment 2

[Tom Ritter [:tjr]]

My typical answer to these questions is that Updatebot exists to make your processes easier, so you should figure out what you want your ergonomics to look like and then adapt Updatebot to do that.

Working off a fork in some way was something that just came up with Cairo (Bug 2034315) so it might be worth talking to Jimb to see if your thoughts on ergonomics align with his.

I could imagine a workflow where Updatebot watches the official upstream workflow, but instead of you enumerating patches in moz.yaml, you just say "get patches from this branch in this other repo."

Quite some time ago we set up a Windows Updatebot job specifically for Angle but it was never used. That whole thing is bitrotten but it would serve as a guidepost.

The hardest part, I think, is that we don't want to be open to supply chain attacks, so we had a design where we would generate the depot_tools binaries necessary, then download them from the job and check them in via tooltool.

Comment 3

[Jamie Nicol [:jnicol]]

Thanks Tom, that's helpful!

Jimb and I have chatted a bit about this. We seemed to agree the main benefit of vendoring from a fork is that having our "patch stack" be an actual repo encourages good practices such as descriptive commit messages, and makes rebasing when there's a conflict slightly easier. The main benefit of an in tree patch stack is that it avoids having to push to a separate repo prior to vendoring, which is especially helpful for sec bugs.

Both options are better than the current situation where we vendor from a local checkout of a fork, which makes it all to easy to vendor changes into firefox and forget to publish to the fork, meaning they might get clobbered by the next update, or at least cause annoyance to the person who has to clean that up.

I think I lean more towards the patch stack being kept in tree and applying patches on top of the upstream repo.

I did however try out a very vanilla mach vendor setup, and there were a couple points which make it undesirable IMO:

• keep, include, exclude are too fiddly for a complex library like ANGLE. Either we use coarse approximations and accept vendoring a lot more files than strictly necessary, or these must be very long and difficult to maintain lists even to get close to pruning the tree to its current state. I think we instead want to continue programmatically determining which files are used.
• patches are applied after the above keep, include, and exclude steps. Our use case will often involve cherry picking security fixes from upstream. If a patch touches a file that's been excluded, the vendoring will fail. It's fiddly to have to trim these patches.

So what I'm leaning towards is a workflow which is still invoked by running mach vendor. But this will skip quite a few if the steps (keep, include, exclude, move-contents, update-mozbuild). It'll then have a run-command action which calls our script.

The script can fetch a few required dependencies itself, without the need to use gclient or have depot_tools available. Then apply our patch stack. Then generate the moz.builds, determine which files are actually used, and move them into place.

For fetching the dependencies, either we can fetch the angle repo itself using git, then initialise and fetch the required git submodules. Or we can fetch angle as a googlesource archive, manually parse the DEPS file, then fetch the dependencies as googlesource archives. The latter will be a bit more code but is so much faster.

Comment 4

[Tom Ritter [:tjr]]

I did however try out a very vanilla mach vendor setup, and there were a couple points which make it undesirable IMO:

• keep, include, exclude are too fiddly for a complex library like ANGLE. Either we use coarse approximations and accept vendoring a lot more files than strictly necessary, or these must be very long and difficult to maintain lists even to get close to pruning the tree to its current state. I think we instead want to continue programmatically determining which files are used.

Makes sense. opus and irregexp do something like that.

• patches are applied after the above keep, include, and exclude steps. Our use case will often involve cherry picking security fixes from upstream. If a patch touches a file that's been excluded, the vendoring will fail. It's fiddly to have to trim these patches.

So what I'm leaning towards is a workflow which is still invoked by running mach vendor. But this will skip quite a few if the steps (keep, include, exclude, move-contents, update-mozbuild). It'll then have a run-command action which calls our script.

The script can fetch a few required dependencies itself, without the need to use gclient or have depot_tools available. Then apply our patch stack. Then generate the moz.builds, determine which files are actually used, and move them into place.

Sounds good!

For fetching the dependencies, either we can fetch the angle repo itself using git, then initialise and fetch the required git submodules. Or we can fetch angle as a googlesource archive, manually parse the DEPS file, then fetch the dependencies as googlesource archives. The latter will be a bit more code but is so much faster.

So there's two flows that are implicated by this. The first is the actual vendoring, the second is -check-for-update. check-for-update basically asks the upstream repo as efficiently as possible is there any new version, and if so returns its tag/hash. We try hard to avoid cloning or downloading anything to answer that question if we can avoid it, although we do need to do so for tags. We have a GoogleSourceHost defined already that seems like it will do what you want except the recursive "parse DEPS, download more things", but I think you could subclass GoogleSourceHost into AngleSourceHost and add that into upstream_snapshot.

Comment 5

[Jim Blandy :jimb]

For Cairo we settled on having a patch stack (gfx/cairo/patches) that is applied by UpdateBot (gfx/cairo/moz.yaml).

The deciding factors were:

• We want to be able to land security patches without publishing the problem in a GitHub repo.

• Drawing from an external repo raises questions about whether review occurs in the external repo or in Mozilla Central. Confusion about review is not great. When the patch stack lives in Mozilla Central alongside the sources, there's no ambiguity here. You end up reviewing patches that contain patches ("what is the second derivative of... your source code??"), but I get weirder things than that free in my breakfast cereal.

• UpdateBot's polling for upstream changes works most readily when you can point it at the true upstream repo.

In the end, we settled on having gfx/cairo/moz.yaml point at the true upstream Cairo repo, and contain a list of 33(!!) patches to apply.

Comment 6

[Jim Blandy :jimb]

It does make me unhappy that our local changes to Cairo are not represented as commits in a version control system, as all changes to source code should be.

Comment 7

[Tom Ritter [:tjr]]

(In reply to Jim Blandy :jimb from comment #6)

It does make me unhappy that our local changes to Cairo are not represented as commits in a version control system, as all changes to source code should be.

When you vendor through mach vendor there should be a second separate commit that basically squashes all the local patches into one big one, right?

Updatebot could be changed to submit them as separate commits - but then you'd need to go through and r+ 34 revisions instead of two...

Comment 8

[Jim Blandy :jimb]

(In reply to Tom Ritter [:tjr] from comment #7)

When you vendor through mach vendor there should be a second separate commit that basically squashes all the local patches into one big one, right?

Updatebot could be changed to submit them as separate commits - but then you'd need to go through and r+ 34 revisions instead of two...

This isn't the issue. Storing changes as a series of patch files is what I don't like. I think it's justified in the case of Cairo, for the reasons given in comment 5.

Comment 9

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Implement upstream_snapshot() for AngleHost. r?#gfx-reviewers

This will be used by the following patches, where mach vendor's
"fetch" step will be used to fetch ANGLE.

AngleHost now derives from GoogleSourceHost, whose implementation is
used to fetch the ANGLE tarball. We additionally need to fetch a
number of ANGLE's dependencies. To do so we first extract the tarball,
then parse the DEPS file in the ANGLE repo root to obtain the URL and
revision of the required dependencies. Each dependency is then fetched
and extracted into the correct location, then the entire checkout is
tarballed up.

vendor_manifest's safe_extract_tar() helper function is moved to its
top-level so that it can be imported and reused here.

Comment 10

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Switch ANGLE to commit tracking, fixing the Chromium Beta channel query. r?#gfx-reviewers

Previously ANGLE was tracking tags, with a note explaining that we're
really tracking branches but tags were a close enough approximation.
AngleHost.upstream_tag() would return the ANGLE branch name currently
used by Chromium's Beta channel.

The problem with this is that branches are mutable. New commits appear
in branches, and we need to detect this even if the Chromium beta
channel remains on the same ANGLE branch. This patch therefore
switches ANGLE to commit tracking. Similar to before,
update_commit() with a revision of "HEAD" will return the ANGLE
revision currently used by Chromium's Beta channel, but importantly we
return the sha rather than the branch name.

This will be important when the updated ANGLE vendor process will be
invoked via mach vendor, which will be responsible for both checking
whether there are any updates and updating the revision in the yaml.

An upside of this, is that mach vendor can now be invoked with
either a branch name or precise revision for the --revision
argument, while retaining the previous behaviour of matching Chromium
Beta if omitted.

Note that the service previously used to query the Chromium Beta
channel is now defunct, so this patch switches to using an
alternative.

Comment 11

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Update ANGLE vendoring process. r?#gfx-reviewers

This replaces the existing update-angle.py script.

The new vendoring process is invoked via mach vendor. This fetches
an ANGLE checkout and required dependencies, then calls the new
update-angle.py script.

The new script:

• Applies a set of local patches to the checkout.
• Generates moz.build files using gn_processor.py
• Vendors the required files into tree.

Like with the previous script, we programatically determine which
files are required to be vendored into tree. This is preferred to
relying on mach vendor's "include" and "exclude" rules, which are
too coarse for a project as complex as ANGLE.

Patches are also applied via the script rather than mach vendor
directly. This is because we must apply patches before determining
which files are required to be vendored, but mach vendor/updatebot
like to commit the update first, and then apply patches in a second
commit.

Our usage of gn_processor.py requires the checkout to be contained
within firefox's root third_party directory. The ANGLE checkout will
therefore now live in third_party/angle rather than
gfx/angle/checkout.

Note that the existing vendor_from_git.py script is retained despite
not being used by the new script, as it is still used for updating
WebGL conformance tests.

To keep things reviewable, we have not yet revendored using this
process. That will occur in the next patch.

Comment 12

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Add files required to generate ANGLE's build scripts with gn_processor. r?#gfx-reviewers

The build_overrides files mostly just ensure GN can find ANGLE and its
dependencies at the correct location within the vendored tree. The
angle_has_build option ensures GN uses (Firefox's vendored fork of)
Chromium's build repo for build configuration.

gn-configs/angle.json contains metadata used by gn_processor. The
interesting pieces are:

• We build the "//:libEGL" target (and all its dependencies, including
  libGLESv2 and the shader translator) on Windows. Everywhere else, we
  only build the shader translator.

• We disable all backends and non-required features by default. The
  translator's GLSL and ESSL backends are enabled everywhere. On windows
  we additionally enable the D3D and HLSL backends.

• ANGLE_ENABLE_KEYEDMUTEX, ANGLE_SKIP_DXGI_1_2_CHECK, and
  ANGLE_PLATFORM_EXPORT are defined here to match the existing behaviour
  in gfx/angle/moz.build.common. The first two enable required features
  and the latter avoids compilation errors.

• Disabling ANGLE_ENABLE_DEBUG_ANNOTATIONS avoids overly verbose logging
  in debug builds.

• The INCLUDE_GECKO_ZLIB_HANDLING mozbuild variable ensures the
  compression_utils_portable library links against Firefox's zlib (after
  the update-angle.py script has stubbed out the Google zlib
  dependency).

Comment 13

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Add ANGLE patches. r?#gfx-reviewers

These patches will be applied in numerical order by the
update-angle.py script when vendoring ANGLE into Firefox.

The patches were taken from the firefox-153 branch of
https://github.com/mozilla/angle, with a few additional patches, the
purposes of which are described in the patches themselves.

See gfx/angle/README.md for instructions regarding rebasing and adding
new patches.

Comment 14

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Update references for ANGLE's new location and target names. r?#gfx-reviewers

ANGLE will now be vendored into tree at third_party/angle rather than
gfx/angle/checkout, and the shader translator static library is now
named "translator_gn".

Update include paths and moz.build references accordingly.

Note that this patch will temporarily break the build, until the
following patch actually re-vendors ANGLE into the new location. It
has been kept separate to make review easier.

Comment 15

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Re-vendor ANGLE using new process. r?#gfx-reviewers

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/99f7892de848
https://hg.mozilla.org/integration/autoland/rev/63a069919db4
Implement upstream_snapshot() for AngleHost. r=gfx-reviewers,ErichDonGubler
https://github.com/mozilla-firefox/firefox/commit/a478e8bb3d00
https://hg.mozilla.org/integration/autoland/rev/637558e13ce3
Switch ANGLE to commit tracking, fixing the Chromium Beta channel query. r=gfx-reviewers,ErichDonGubler
https://github.com/mozilla-firefox/firefox/commit/fc5c583ab310
https://hg.mozilla.org/integration/autoland/rev/3e1b90be8569
Update ANGLE vendoring process. r=gfx-reviewers,ErichDonGubler
https://github.com/mozilla-firefox/firefox/commit/c68d3c951b18
https://hg.mozilla.org/integration/autoland/rev/b84ff2a8c7f7
Add files required to generate ANGLE's build scripts with gn_processor. r=gfx-reviewers,ErichDonGubler
https://github.com/mozilla-firefox/firefox/commit/1500a0f2ae96
https://hg.mozilla.org/integration/autoland/rev/009999f4be79
Add ANGLE patches. r=gfx-reviewers,ErichDonGubler
https://github.com/mozilla-firefox/firefox/commit/0834e7feaccd
https://hg.mozilla.org/integration/autoland/rev/0fa2d59bef4b
Update references for ANGLE's new location and target names. r=gfx-reviewers,webrtc-reviewers,ErichDonGubler,mjf
https://github.com/mozilla-firefox/firefox/commit/7320f912e2a3
https://hg.mozilla.org/integration/autoland/rev/75870a236402
Re-vendor ANGLE using new process. r=gfx-reviewers,nical

Comment 17

[Jamie Nicol [:jnicol]]

Attachment: Bug 2035541 - Set BUG_COMPONENT for third_party/angle.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/f90bbc5ed435
https://hg.mozilla.org/integration/autoland/rev/4d11378e3f07
Set BUG_COMPONENT for third_party/angle.

Comment 19

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/63a069919db4
https://hg.mozilla.org/mozilla-central/rev/637558e13ce3
https://hg.mozilla.org/mozilla-central/rev/3e1b90be8569
https://hg.mozilla.org/mozilla-central/rev/b84ff2a8c7f7
https://hg.mozilla.org/mozilla-central/rev/009999f4be79
https://hg.mozilla.org/mozilla-central/rev/0fa2d59bef4b
https://hg.mozilla.org/mozilla-central/rev/75870a236402
https://hg.mozilla.org/mozilla-central/rev/4d11378e3f07