1399679 - Switch build environment from CentOS to Debian

Status: RESOLVED FIXED

(Firefox Build System :: General, enhancement)

Description

[Gregory Szorc [:gps]]

We currently build Firefox on Linux using CentOS 6 as the base operating system.

Because Debian is investing a lot in reproducible builds and because I feel that most Linux developers are on Debian-based distros (like Ubuntu), I think it makes sense for us to transition the official build environment from CentOS to Debian. Plus, CentOS 6 is a bit long in the tooth and it is consuming a non-trivial amount of engineering effort to continue to work around its age. If we switch to a modern distro, we get a lot of nice things out-of-the-box and don't have to spend as much effort massaging the environment.

Since the build environment is controlled via Docker images whose population is defined in-tree, we can make this change and it will "ride the trains" just like everything else.

When we change the build environment, we may also change run-time dependencies. For example, newer distros have a newer glibc and this prevents binaries compiled on new distros from running on older ones. CentOS 6's libraries are sufficiently old and run in a lot of places.

glandium: could you please brain dump and/or set up bug dependencies to track what needs to be configured on Debian (presumably Stretch) to build Firefox so that produced binaries have acceptable binary compatibility with various Linux distributions?

Comment 1

[Mike Hommey [:glandium]]

I was actually considering working on this next quarter.

> (presumably Stretch)

that's way too recent. The best we can do at the moment is wheezy. And that will allow us to drop the gtk3 tooltool package.

Comment 2

[Gregory Szorc [:gps]]

What are the exact reasons we can't use stretch (or newer)? I assume it has something to do with library compatibility, including glibc. Could we compile appropriate versions of the run-time dependencies from source so "modern" dependencies don't leak into shipped builds? I'm thinking of something like having a shadow /usr in /mozilla or something. As long as configure is picking up the proper dependencies, we should be good, right?

I *really* don't want the general execution environment beholden to compatibility with the oldest distro we need run-time support for. i.e. I'd love if we could standardize on stretch (or later) so there is greater coherency across images and we don't have to manage configuring so many one-offs.

Comment 3

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] (away until ~October 23) from comment #2)
> What are the exact reasons we can't use stretch (or newer)? I assume it has
> something to do with library compatibility, including glibc. Could we
> compile appropriate versions of the run-time dependencies from source so
> "modern" dependencies don't leak into shipped builds? I'm thinking of
> something like having a shadow /usr in /mozilla or something. As long as
> configure is picking up the proper dependencies, we should be good, right?

Maintaining that and ensuring it doesn't screw things up randomly in the future is much more work than maintaining a base image of an old debian, even if the latter means we need to backport a few packages (at least python, thanks to https://bz.mercurial-scm.org/show_bug.cgi?id=5710 ; as for mercurial, we already don't install it from packages).

A lower maintenance solution would be to use chroots of an older debian in a stretch image, but then you end up having to switch between running things on the host and running others in the chroot. Remember mock? Not really appealing.

FWIW, I built a wheezy based image by barely changing the dockerfile from the stretch-based android-build image.

Comment 4

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Check differences between builds on CentOS and Debian.

We enable stripping the binaries and ensure STRIP_FLAGS doesn't reduce
the amount of stripping that happens on Linux (where the default of
--strip-debug doesn't strip the symbol stable), so that the comparison
doesn't show differences we don't care about.

We also reduce the noise by excluding differences in .gnu_debuglink
sections, as well as .dynstr. The former is because they are a side
effect of suble changes in debug info (such as line numbers in system
headers) changing the signature used for the debug link. The latter
because in some libraries, there are differences in those sections, but
they are not accompanied with actual changes in the list of symbols,
meaning the only change is in the order in which the strings for the
symbols are laid out, not the symbol lists themselves (which would be
indicated by differences in readelf --section output).

We explicitly don't skip the notes sections differences, despite them
being affected by the same problem as debug link for the build id they
contain, but they also contain other information.

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

Comment 5

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Avoid differences in pm_linux.cpp

struct perf_event_attr is larger(!) in the older(!) kernel headers
(2.6.32) on CentOS 6, because they backported features from kernels
(3.7 and 3.19) newer than the one in Debian wheezy (3.2) or
wheezy-backports (3.14).

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

Comment 6

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Add a no-op cast to cairo-ft-font.c

In the freetype version on CentOS, many FT_* constants are defined as
plain integer. One of them is FT_LOAD_VERTICAL_LAYOUT, defined as 0x10.
On the version on Debian wheezy, it's defined as (1L << 4), which is the
same value, but explicitly defined as a long.

For some reason, there's one place in cairo-ft-font.c where that
difference in constant definitions makes a difference in codegen (and
not other places).

So we add an explicit cast to long for that value in that one place.
The variable tested against is an int, so there's no inherent problem
with the code either way.

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

Comment 7

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Expand the NextRequest macro from Xlib.h in nsX11ErrorHandler.cpp

On CentOS 6, the macro expands to:
  (((_XPrivDisplay)(dpy))->request + 1)

On Debian wheezy, it expands to:
  (((_XPrivDisplay)dpy)->request + 1)

where dpy is the argument it's given in both cases.

This could make a difference if dpy was a non trivial expression, but in
nsX11ErrorHandler.cpp, it's simply "display", which is a `Display*`.

So both expansions have the same semantics, but for some reason, in
nsX11ErrorHandler.cpp, using one or the other causes the compiler to
reorder some of the instructions it emits.

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

Comment 8

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Arrange some includes such that linux/posix_types.h is included before sys/select.h

The FD_SET/FD_CLR/FD_ZERO/FD_ISSET macros, when you look all the way
down, are defined depending on a _FDMASK or __FDMASK and a _NFDBITS or
__NFDBITS macro.

As described in sys/select.h, "Some versions of <linux/posix_types.h>
define these macros", and that's the case on CentOS 6. That's however
not the case on Debian wheezy.

sys/select.h has, following that comment, some #undef's to remove those
definitions, but when linux/posix_types.h is included afterwards, its
definitions take precedence.

Interestingly, while their effect is the same, the definitions are not
exactly the same between sys/select.h and linux/posix_types.h on CentOS
6, and depending on which is included first, codegen can be slightly
different.

So we arrange for the definitions in sys/select.h to be picked.

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

Comment 9

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Disable math inlines on Linux x86

On CentOS 6, the glibc headers only have math.h inlines for the signbit
function. On Debian wheezy, they have much more, including ceil and
floor, which changes the codegen for every place ceil or floor are used,
since call to libm functions are replaced with inline code.

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

Comment 10

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Revert everything.

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

Comment 11

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Use debian7-*-build instead of desktop-build.

Switch almost all builds currently using the desktop-build image to use
the right debian7-*-build image instead. The only exception is the rust
bindgen spidermonkey builds, that require cmake being installed, but I
don't want to add it to the base images because that involves risking a
cmake dependency unwantedly slipping in Firefox (rust-bindgen ironically
requires cmake to build a single C++ file...)

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

Comment 12

[Mike Hommey [:glandium]]

Attachment: Bug 1399679 - Build all tasks that depended on the desktop-build image

try_task_config.json generated with:

curl -sL https://queue.taskcluster.net/v1/task/PufIaq6fTMaxyh3U9ggBjg/runs/0/artifacts/public/full-task-graph.json | jq '{"tasks": [to_entries[] | select(try .value.dependencies[.value.task.payload.image.taskId["task-reference"] | ltrimstr("<") | rtrimstr(">")] == "build-docker-image-desktop-build") | .key]}' > try_task_config.json

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

Comment 13

[Mike Hommey [:glandium]]

- Here's a push with the first patch, which does a raw comparison between builds on CentOS and builds on Debian:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9dff3d5f42beb1080642e481eb33c7ac6bdbc5bb

Those have large linux diffs (follow the diff.html links in the various "diff opt" jobs), and minimal macosx diffs, that only show a difference in buildconfig.html, showing that the path to GNU make changed, and that's expected. See the commit message of that patch for some of the tweaks that were done to reduce the raw diff size (e.g. stripping binaries).

- Here's a push with all the patches up to "Disable math inlines on Linux x86":
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14ca230ca6105e67833fb2ff84aaa6640cf84bc5
Each of those commits explains what is being changed, and how they affect code. They are not meant to land, they are only there as a documentation of what differences switching the builds to Debian inevitably will do, allowing an assessment of the risk.

There again, follow the diff.html links in the various "diff opt" jobs. The macosx diff is the same, unsurprisingly, and the linux* diffs have the same buildconfig.html difference in the GNU make path, and build-id differences in all binaries.

- Here's a push with all the patches applied:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f616ed03ee0fea7a9441955c0c3a120a198034a1

It enabled all the jobs that are currently built with the desktop-build docker image. Considering the "Revert everything" patch, it's equivalent to only the last two patches being applied, the last one being the try selection. How the try selection was done is documented in the corresponding commit message (taking the full-task-graph.json from the mozilla-central changeset this is all based on).

As of writing there is only one job failed: osx searchfox idx, but AFAICT, it's been busted for a while. I expect everything else to go green (as you can expect, this is not the first time I pushed a full blown test, and it's been all green (except that searchfox job) multiple times already).

As noted in bug 1430506, I don't want to land this before central goes to 60.

Comment 14

[Gregory Szorc [:gps]]

Comment on attachment 8943536 [details]
Bug 1399679 - Use debian7-*-build instead of desktop-build.

https://reviewboard.mozilla.org/r/213886/#review220404

glandium: the work you've done to get us to this point is amazing. The Try pushes showing binary diffs and you showing the series of patches required to minimize those diffs is excellent, thorough engineering. Switching the official build environment from CentOS to Debian is not something that should be undertaken lightly. But your work demonstrates that very little actually changes as a result of that switch. Of course, you put in so much effort to reduce those differences to get us to this point. You deserve commendation for your work on this project.

This change is a massive step forward in making Firefox builds deterministic and reproducible. That's not only a marketable product feature that can improve the security and trust of Firefox, but it is also a property that will foster more effective build caching and make it easier for others to build Firefox like our automation does (by making the toolchains we use for Firefox builds readily reusable).

FWIW, one of the lesser-noticed improvements of this change is that the zstd compressed Docker image for linux64 is now ~177 MB instead of ~650 MB. That means faster task starts when the worker doesn't have the image cached.

mozilla-central is Firefox 60 as of a few hours ago. So you are clear to land this.

Review granted with delight. Congratulations on a project very well done.

::: taskcluster/ci/spidermonkey/linux.yml:44
(Diff revision 1)
>      treeherder:
>          symbol: SM-tc(rust)
>          tier: 2
>          platform: linux64/debug
> +    worker:
> +        docker-image: {in-tree: desktop-build}

It would be *really* nice to nuke the `desktop-build` image from the source repository. Would you mind filing a bug so we can track this?

Comment 15

[Mike Hommey [:glandium]]

(In reply to Gregory Szorc [:gps] from comment #14)
> ::: taskcluster/ci/spidermonkey/linux.yml:44
> (Diff revision 1)
> >      treeherder:
> >          symbol: SM-tc(rust)
> >          tier: 2
> >          platform: linux64/debug
> > +    worker:
> > +        docker-image: {in-tree: desktop-build}
> 
> It would be *really* nice to nuke the `desktop-build` image from the source
> repository. Would you mind filing a bug so we can track this?

We unfortunately can't just yet. I have to file a series of bug for the steps required to do so, which is essentially move valgrind and mingw builds off of it (the valgrind builds don't use the desktop-build image, but use an image that is almost exactly it, except for a few details). There are a few things I want to do before to make that transition easier though, one of which is allow actually building derivative docker images without relying on the hub.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8664edd863ff
Use debian7-*-build instead of desktop-build. r=gps

Comment 17

[Mike Hommey [:glandium]]

Oh, and the mozjs rust bindings, too because it uses cmake.

Comment 18

[Gregory Szorc [:gps]]

Attachment: Bug 1399679 - Add a version string to cache names;

So we can change a string in this file to force the use of new caches,
thus ensuring a clean break from one changeset to the next.

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

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8944647 [details]
Bug 1399679 - Add a version string to cache names;

https://reviewboard.mozilla.org/r/214804/#review220424

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e331a3b9fae2
Add a version string to cache names; r=glandium

Comment 21

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/8664edd863ff
https://hg.mozilla.org/mozilla-central/rev/e331a3b9fae2

Comment 22

[Nick Alexander :nalexander [he/him]]

(In reply to Gregory Szorc [:gps] from comment #14)
> Comment on attachment 8943536 [details]
> Bug 1399679 - Use debian7-*-build instead of desktop-build.
> 
> https://reviewboard.mozilla.org/r/213886/#review220404
> 
> glandium: the work you've done to get us to this point is amazing. The Try
> pushes showing binary diffs and you showing the series of patches required
> to minimize those diffs is excellent, thorough engineering. Switching the
> official build environment from CentOS to Debian is not something that
> should be undertaken lightly. But your work demonstrates that very little
> actually changes as a result of that switch. Of course, you put in so much
> effort to reduce those differences to get us to this point. You deserve
> commendation for your work on this project.
> 
> This change is a massive step forward in making Firefox builds deterministic
> and reproducible. That's not only a marketable product feature that can
> improve the security and trust of Firefox, but it is also a property that
> will foster more effective build caching and make it easier for others to
> build Firefox like our automation does (by making the toolchains we use for
> Firefox builds readily reusable).
> 
> FWIW, one of the lesser-noticed improvements of this change is that the zstd
> compressed Docker image for linux64 is now ~177 MB instead of ~650 MB. That
> means faster task starts when the worker doesn't have the image cached.
> 
> mozilla-central is Firefox 60 as of a few hours ago. So you are clear to
> land this.
> 
> Review granted with delight. Congratulations on a project very well done.

I just want to pile on here -- this has been a strong effort that I've watched with pleasure and learned from.  Great work!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5fad234e2e84
Use posix shell in comm-confvars.sh; r=me
https://hg.mozilla.org/comm-central/rev/32a25748090a
Port Bug 1399679: Use debian7-*-build instead of desktop-build; r=me