Use newer binutils on builds using clang from tooltool

NEW
Assigned to

Status

()

Core
Build Config
2 years ago
9 months ago

People

(Reporter: glandium, Assigned: egoktas)

Tracking

(Blocks: 2 bugs, {leave-open})

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

(Reporter)

Description

2 years ago
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).

Comment 1

2 years ago
: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)
(Reporter)

Comment 2

2 years ago
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)
(Reporter)

Comment 3

2 years ago
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)

Comment 6

2 years ago
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)

Comment 8

2 years ago
Thanks gbrown, I really appreciate it, I'll attach some new patches in bug 1272629
(Assignee)

Updated

a year ago
Assignee: nobody → egoktas
(Assignee)

Comment 9

a year ago
Created attachment 8771645 [details]
Bug 1272629 - Add taskcluster task to build binutils package

Review commit: https://reviewboard.mozilla.org/r/64742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64742/
Attachment #8771645 - Flags: review?(mh+mozilla)
(Assignee)

Comment 10

a year ago
Created attachment 8771650 [details]
Bug 1272629 - Make binutils building scripts executable

Review commit: https://reviewboard.mozilla.org/r/64748/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/64748/
(Assignee)

Comment 11

a year ago
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/
(Assignee)

Updated

a year ago
Attachment #8771650 - Attachment is obsolete: true
(Assignee)

Comment 12

a year ago
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/
(Assignee)

Comment 13

a year ago
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/
(Reporter)

Comment 14

a year ago
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)
(Assignee)

Comment 15

a year ago
Created attachment 8772560 [details]
Bug 1272629 - Add bison package installation to taskcluster docker setup script

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)
(Assignee)

Comment 16

a year ago
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/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce9a4b5d8ed2
(Assignee)

Comment 18

a year ago
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/
(Assignee)

Comment 19

a year ago
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/
(Reporter)

Comment 20

a year ago
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
(Reporter)

Comment 21

a year ago
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)
(Assignee)

Comment 22

a year ago
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)
(Assignee)

Comment 23

a year ago
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/
(Assignee)

Comment 24

a year ago
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/
(Reporter)

Comment 25

a year ago
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+
(Assignee)

Updated

a year ago
Attachment #8772560 - Flags: review?(dustin)
(Reporter)

Comment 26

a year ago
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

Updated

a year ago
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.
(Reporter)

Comment 28

a year ago
(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.
(Reporter)

Comment 29

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

(in build-gcc.sh)
(Reporter)

Comment 30

a year ago
(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+

Comment 33

a year ago
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
(Reporter)

Updated

a year ago
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)

Comment 35

a year ago
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
(Reporter)

Comment 36

a year ago
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)
(Assignee)

Comment 37

a year ago
(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)
(Reporter)

Comment 39

a year ago
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.
(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.

Updated

a year ago
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.
(Assignee)

Comment 43

a year ago
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Blocks: 1286788
(Reporter)

Comment 47

a year ago
mozreview-review
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+
(Reporter)

Comment 48

a year ago
mozreview-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+

Comment 49

a year ago
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

Comment 50

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9be03f78e363
https://hg.mozilla.org/mozilla-central/rev/b0d43f0d4e1e
(Assignee)

Comment 51

a year ago
(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)
(Reporter)

Comment 52

a year ago
Considering the title of this bug, and the fact it's not closed, you can continue here.
Flags: needinfo?(mh+mozilla)
You need to log in before you can comment on or make changes to this bug.