Closed Bug 1272629 Opened 8 years ago Closed 6 years ago

Use newer binutils on builds using clang from tooltool

Categories

(Firefox Build System :: General, defect)

Product:
Firefox Build System
Component:
General
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED DUPLICATE of bug 1486998

People

(Reporter: glandium, Assigned: egoktas)

References

(Blocks 1 open bug)

Details

Attachments

(5 files, 1 obsolete file)

Bug 1272629 - Add taskcluster task to build binutils package
58 bytes, text/x-review-board-request
glandium
: review+
gps
: review-
Details
Bug 1272629 - Add bison package installation to taskcluster docker setup script
58 bytes, text/x-review-board-request
dustin
: review+
Details
Packages differences between the build that succeeded with 0.1.6 and the build that failed with 0.1.7
28.45 KB, patch
Details | Diff | Splinter Review
Bug 1272629 - Add bison package to desktop-build image;
58 bytes, text/x-review-board-request
glandium
: review+
Details
Bug 1272629 - Add taskcluster task to build binutils package;
58 bytes, text/x-review-board-request
glandium
: review+
Details 
Currently, we are using binutils 2.25.1 when building with GCC from tooltool because the GCC package from tooltool contains binutils.

That's however not the case when building with clang from tooltool because the clang package from tooltool doesn't contain binutils. The result is that we end up using whatever binutils are on the underlying OS, which in most cases is CentOS 6, which comes with ancient binutils. Which are suspected to cause problems in bug 1253299.

So, it would be good to have binutils shipped with clang as well. There are multiple ways we can do this:
- Build binutils when building clang from build-clang.py, like it's built in build-gcc.sh.
- Copy the necessary binutils tools from the GCC archive to the clang archive in build-clang.py (we already copy files like headers and libgcc this way, so it's not entirely crazy)
- Build a separate tooltool package for binutils.

The latter would have the advantage of allowing independent updates of binutils in the future, but would require more work (especially since I'll insist on having the package built on taskcluster the same way the GCC and clang packages are built on taskcluster).
:glandium do you know who might be able to work on this and in what timeframe? Trying to assess this as getting the ASAN builds on TC is a Q2 releng goal
Flags: needinfo?(mh+mozilla)
I'd say anyone (and I can give directions if necessary), which then makes it more of a management question, which I guess would fall on jgriffin or dburns.
Flags: needinfo?(mh+mozilla)
Flags: needinfo?(jgriffin)
Flags: needinfo?(dburns)
Note that to unblock bug 1253299 or at least check that this would, indeed, unblock bug 1253299, it would be enough to pull the gcc tarball from tooltool (i.e. add it to the relevant manifest) and add $topsrcdir/gcc/bin to $PATH so that the `ld` program is pulled from there instead of /usr/bin.
I think gbrown may be able to help with at least comment #3
Flags: needinfo?(jgriffin)
Sure, I'll check on comment 3 right away.
Flags: needinfo?(gbrown)
thanks gbrown!
Flags: needinfo?(dburns)
That seems to work just fine: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4fc6e08234e8

Thanks glandium!
Flags: needinfo?(gbrown)
Thanks gbrown, I really appreciate it, I'll attach some new patches in bug 1272629
Assignee: nobody → egoktas
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/1-2/
Attachment #8771650 - Attachment is obsolete: true
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/2-3/
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/3-4/
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

https://reviewboard.mozilla.org/r/64742/#review62086

::: build/unix/build-binutils/build-binutils.sh:3
(Diff revision 4)
> +#!/bin/bash
> +
> +binutils_version=2.26

Let's start with keeping the same version as the one we're currently using: 2.25.1

::: build/unix/build-binutils/build-binutils.sh:19
(Diff revision 4)
> +wget -c -P $TMPDIR ftp://ftp.gnu.org/gnu/bison/bison-${bison_version}.tar.xz || exit 1
> +tar xf $TMPDIR/bison-${bison_version}.tar.xz
> +
> +# Build Bison
> +mkdir bison-objdir
> +cd bison-objdir
> +
> +../bison-${bison_version}/configure --prefix /tools/bison/ || exit 1
> +make $make_flags || exit 1
> +make install $make_flags DESTDIR=$root_dir || exit 1

It would be better to avoid doing this at all. The simple solution is just to install bison in the taskcluster docker image used.
You can do this by changing testing/docker/centos6-build/system-setup.sh. You'll then probably need to update testing/docker/centos6-build/VERSION, testing/docker/centos6-build-upd/VERSION and testing/docker/centos6-build-upd/Dockerfile (that's what the last change to testing/docker/centos6-build/system-setup.sh required, but you should check with Dustin Mitchell whether that's still necessary, because things may have changed since the last change to that file)

::: taskcluster/ci/legacy/tasks/branches/base_jobs.yml:813
(Diff revision 4)
> +  linux64-binutils:
> +    task: tasks/builds/linux64_binutils.yml
> +    root: true
> +    when:
> +        file_patterns:
> +          - 'build/build-binutils/**'

build/unix/build-binutils/**
Attachment #8771645 - Flags: review?(mh+mozilla)
Update centos6 build version from 0.1.6 to 0.1.7

Review commit: https://reviewboard.mozilla.org/r/65328/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65328/
Attachment #8771645 - Flags: review?(mh+mozilla)
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/4-5/
Comment on attachment 8772560 [details]
Bug 1272629 - Add bison package installation to taskcluster docker setup script

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65328/diff/1-2/
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/5-6/
https://reviewboard.mozilla.org/r/65328/#review62378

You'll want to r? dustin for this patch.

::: testing/docker/centos6-build/system-setup.sh:274
(Diff revision 2)
>  install libuuid-devel
>  install openssl-static
>  install cmake
>  install subversion
> +
> +# required for building binutits

typo on binutils
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

https://reviewboard.mozilla.org/r/64742/#review62380

::: build/unix/build-binutils/build-binutils.sh:4
(Diff revision 6)
> +#!/bin/bash
> +
> +binutils_version=2.25.1
> +#bison_version=3.0.2

Please remove the commented parts building bison entirely.
Attachment #8771645 - Flags: review?(mh+mozilla)
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/6-7/
Attachment #8771645 - Flags: review?(mh+mozilla)
Comment on attachment 8772560 [details]
Bug 1272629 - Add bison package installation to taskcluster docker setup script

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65328/diff/2-3/
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/64742/diff/7-8/
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

https://reviewboard.mozilla.org/r/64742/#review62384
Attachment #8771645 - Flags: review?(mh+mozilla) → review+
Attachment #8772560 - Flags: review?(dustin)
Uploaded the resulting binutils package to tooltool.

"size": 27710776,
"digest": "3ab85f17cbb26a0d91eb55c6724323de32df298caecec40559b1d7cf275344a42b8ba6d6dd9cfdfde4307dcb2af07a82dae88aa0b00590efaf6c09cf457d4a24",

From there, the next steps are:
- Build a new gcc package without binutils
- Add the binutils package to the tooltool manifests for all builds using gcc.tar.xz
- Change the build/unix/mozconfig.* files to use binutils from the binutils package instead of gcc
- Remove the gcc package from tooltool manifests for clang-based builds
Attachment #8771645 - Flags: review-
Comment on attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

https://reviewboard.mozilla.org/r/64742/#review62604

::: build/unix/build-binutils/build-binutils.sh:17
(Diff revision 8)
> +wget -c -P $TMPDIR ftp://ftp.gnu.org/gnu/binutils/binutils-${binutils_version}.tar.bz2 || exit 1
> +tar xjf $TMPDIR/binutils-${binutils_version}.tar.bz2

This is insecure.

Please verify the SHA-256 of the downloaded archive.
(In reply to Gregory Szorc [:gps] from comment #27)
> Comment on attachment 8771645 [details]
> Bug 1272629 - Add taskcluster task to build binutils package
> 
> https://reviewboard.mozilla.org/r/64742/#review62604
> 
> ::: build/unix/build-binutils/build-binutils.sh:17
> (Diff revision 8)
> > +wget -c -P $TMPDIR ftp://ftp.gnu.org/gnu/binutils/binutils-${binutils_version}.tar.bz2 || exit 1
> > +tar xjf $TMPDIR/binutils-${binutils_version}.tar.bz2
> 
> This is insecure.
> 
> Please verify the SHA-256 of the downloaded archive.

This code is already in-tree. This can be done in a followup.
(In reply to Mike Hommey [:glandium] from comment #28)
> This code is already in-tree.

(in build-gcc.sh)
(and we have the same problem in build-gcc and build-clang, so this is something that ought to be addressed all at once, not as a one off for this new script, that is just split off build-gcc)
Note that you have both (gps and egoktas) bumped centos6-build to 0.1.7.  Whoever lands second will need to change to 0.1.8 while rebasing.
Comment on attachment 8772560 [details]
Bug 1272629 - Add bison package installation to taskcluster docker setup script

https://reviewboard.mozilla.org/r/65328/#review62912
Attachment #8772560 - Flags: review?(dustin) → review+
Pushed by mh@glandium.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/66c7af8b2654
Add bison package installation to taskcluster docker setup script. r=dustin
https://hg.mozilla.org/integration/mozilla-inbound/rev/86d07e6bd5b7
Add taskcluster task to build binutils package. r=glandium
Keywords: leave-open
backed out since we have failing valgrind tests like https://treeherder.mozilla.org/logviewer.html#?job_id=32414351&repo=mozilla-inbound after this changes landed
Flags: needinfo?(egoktas)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d14e19de0a1e
Backed out changeset 86d07e6bd5b7 
https://hg.mozilla.org/integration/mozilla-inbound/rev/968c7be2c2a6
Backed out changeset 66c7af8b2654 for failing valgrind tests
The only way I can explain this is if taskcluster image building is busted, and/or the currently used image doesn't actually match the docker files in the tree. Because, really, I see no way adding bison to the docker image would trigger those valgrind errors.
Flags: needinfo?(egoktas) → needinfo?(dustin)
(In reply to Carsten Book [:Tomcat] from comment #34)
> backed out since we have failing valgrind tests like
> https://treeherder.mozilla.org/logviewer.html#?job_id=32414351&repo=mozilla-
> inbound after this changes landed

Yes that is very strange, the only significant change to the os is that the 'bison' package is installed. I will try to see if 'bison' is causing the problem.
(without looking too closely) the other possibility is that something else has been updated in CentOS since the last build.  One way to determine that may be to make a one-click loaner from both a passing and failing build, and compare the package versions.
Flags: needinfo?(dustin)
As can be seen, there's a lot more than bison that has been changed... the joys of docker.
(In reply to Mike Hommey [:glandium] from comment #39)
> Created attachment 8774935 [details] [diff] [review]
> Packages differences between the build that succeeded with 0.1.6 and the
> build that failed with 0.1.7
> 
> As can be seen, there's a lot more than bison that has been changed... the
> joys of docker.

This makes me sad.

The base image builds "FROM centos:6." So when we build that image, it will download from Docker Hub whatever the latest version of the centos:6 image is (and it gets updated every few days). That is not deterministic.

Then when we do a `yum` operation as part of building the image, we sync the latest version of the yum package database from a 3rd party server and install the latest versions of packages.

The reason why so many packages changed and the reason Valgrind broke is because our image building process isn't deterministic over time.

So any time someone changes the base image, we run the risk of pulling in unwanted package version bumps that could break things. It is a ticking time bomb. And it went off in this bug.

You can work around the issue by using the existing centos6-build and centos6-build-upd image tags and add binutils in the desktop-build image. Unfortunately, my work in bug 1289249 to overhaul the desktop-build Docker image won't be so fortunate. Furthermore, my work will make rebuilding the desktop-build Docker image more frequent, which means higher chances of random breakage due to unwanted package updates. For better or worse, I guess I'll need to figure out package pinning. Gah, scope bloat.
There's no practical way to make builds of full linux installs like this deterministic (CentOS is bad, Ubuntu is worse!), because of those 3rd party package databases.  Freezing the repos is, IMHO, impractical (we tried that with PuppetAgain and it's been a nightmare).

We also need to balance the stability of a deterministic build against the need to keep the images up-to-date.  Aside from the (admittedly minor) security issues with running out-of-date images, it can be very difficult to install new packages on a system that is using a years-old base image and repositories.  Again, PuppetAgain has demonstrated this pretty clearly.  For example, this bison install may have required either rebuilding bison against the frozen repository, or importing a lot more packages than just bison into a custom repository; and some of those additional packages may have been the cause of the valgrind issues.  At any rate, with a frozen image there's a rapidly increasing disincentive to touch it as it becomes more and more out-of-date and the likelihood of bustage from an innocent modification grow without bound.

I think the right place to get determinism is in using the same image for multiple builds.

We can get the updates, as well as minimizing the number of extraneous changes pulled in when making changes like this, by rebuilding the docker image periodically (I had suggested weekly), in a distinct cset that can be backed out and investigated if it causes failures.  We're not doing this yet, due to a shortage of round tuits, but I think it is the best way to balance the competing concerns here.
Blocks: 1289812
I filed bug 1289812 to track improving the system package management issue. Let's not rat hole here.

I proposed a workaround in comment 40 for this bug. It is hacky. But it works and is likely far less effort than alternatives. I recommend you do that.
Installing the bison package through yum caused other packages to be updated and this caused some build problems.

Shall we pull and compile bison at the time we are building binutils, instead of yum-installing it?

In this way we can finish this bug I think. I believe that we do not necessarily need to yum-install bison.
Flags: needinfo?(dustin)
I'll take a stab at fixing this.
Flags: needinfo?(dustin)
Blocks: 1286788
Comment on attachment 8791453 [details]
Bug 1272629 - Add bison package to desktop-build image;

https://reviewboard.mozilla.org/r/78860/#review78346
Attachment #8791453 - Flags: review?(mh+mozilla) → review+
Comment on attachment 8791454 [details]
Bug 1272629 - Add taskcluster task to build binutils package;

https://reviewboard.mozilla.org/r/78862/#review78350
Attachment #8791454 - Flags: review?(mh+mozilla) → review+
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9be03f78e363
Add bison package to desktop-build image; r=glandium
https://hg.mozilla.org/integration/autoland/rev/b0d43f0d4e1e
Add taskcluster task to build binutils package; r=glandium
(In reply to Mike Hommey [:glandium] from comment #26)
> Uploaded the resulting binutils package to tooltool.
> 
> "size": 27710776,
> "digest":
> "3ab85f17cbb26a0d91eb55c6724323de32df298caecec40559b1d7cf275344a42b8ba6d6dd9c
> fdfde4307dcb2af07a82dae88aa0b00590efaf6c09cf457d4a24",
> 
> From there, the next steps are:
> - Build a new gcc package without binutils
> - Add the binutils package to the tooltool manifests for all builds using
> gcc.tar.xz
> - Change the build/unix/mozconfig.* files to use binutils from the binutils
> package instead of gcc
> - Remove the gcc package from tooltool manifests for clang-based builds

Hm I think we missed these parts. Shall we file a new bug for these steps?
Flags: needinfo?(mh+mozilla)
Considering the title of this bug, and the fact it's not closed, you can continue here.
Flags: needinfo?(mh+mozilla)
Product: Core → Firefox Build System
The leave-open keyword is there and there is no activity for 6 months.
:kmoir, maybe it's time to close this bug?
Flags: needinfo?(kmoir)
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Flags: needinfo?(kmoir)
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: