904979 - Add build option for rel-eng type builds, and make default builds enable or disable appropriate toolchain features

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

As discussed on irc a few days ago, we're starting to need to differentiate release engineering-type builds from local developer builds.

Comment 1

[Mike Hommey [:glandium]]

Attachment: Add build option for rel-eng type builds

I'd like to avoid non-mozilla releng-type builds not setting this option. I'm wondering what would be appropriate for this. I've been pondering erroring out on make package and make install for browser builds when DEVELOPER_OPTIONS is set, what do you think?

Comment 2

[Mike Hommey [:glandium]]

Attachment: Use -gsplit-dwarf when supported for local builds

Comment 3

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Created attachment 789974 [details] [diff] [review]
> Use -gsplit-dwarf when supported for local builds

FWIW, this made a clobber build on my machine go from 1740s to 1362s, which is 21.7% build time improvement on a clobber. Build time in toolkit/library (which is mostly linker time) went from 85s to 11s. That's with GNU ld.

Comment 4

[Mike Hommey [:glandium]]

Comment on attachment 789974 [details] [diff] [review]
Use -gsplit-dwarf when supported for local builds

No wonder the build was so much faster... clang silently ignores -gsplit-dwarf... and doesn't create debug symbols at all.

Comment 5

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #4)
> Comment on attachment 789974 [details] [diff] [review]
> Use -gsplit-dwarf when supported for local builds
> 
> No wonder the build was so much faster... clang silently ignores
> -gsplit-dwarf... and doesn't create debug symbols at all.

clang 3.2. clang 3.4 does recognize -gsplit-dwarf.

Comment 6

[Mike Hommey [:glandium]]

Attachment: Use -gsplit-dwarf when supported for local builds

This gets me down to 13s of link time with GNU ld, and 1546s for the whole build with clang 3.4, although the 1740s were with clang 3.2, and it's not clear what makes most of the difference. The GNU ld difference is real, though.

Comment 7

[Mike Hommey [:glandium]]

Attachment: Force use gold, if possible, when the default linker is BFD ld, for local builds.

While being more aggressive, let's use gold when it's available. I've been using this trick now and then for my own builds.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Disable ICF and dead code removal on local builds

Together with the other patches, this gets the actual linking time of libxul.so (as run manually by copy/pasting the command make would run) down to 3.5s on my machine, instead of ~1 minute.

Comment 9

[(not currently active) Ted Mielczarek]

(In reply to Mike Hommey [:glandium] from comment #1)
> I'd like to avoid non-mozilla releng-type builds not setting this option.
> I'm wondering what would be appropriate for this. I've been pondering
> erroring out on make package and make install for browser builds when
> DEVELOPER_OPTIONS is set, what do you think?

That seems kind of gross, there are plenty of reasons for people to want to package their local build without caring if it matches other things. We could make builds with MOZILLA_OFFICIAL set but not --enable-release error out (although we could also be leveraging --enable-release to set MOZILLA_OFFICIAL).

Comment 10

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #9)
> That seems kind of gross, there are plenty of reasons for people to want to
> package their local build without caring if it matches other things. We
> could make builds with MOZILLA_OFFICIAL set but not --enable-release error
> out.

I was under the impression that MOZILLA_OFFICIAL was not supposed to be used for redistribution, although in practice, it is. So I guess that's good enough.

> (although we could also be leveraging --enable-release to set
> MOZILLA_OFFICIAL).

That doesn't seem antagonistic.

Comment 11

[Mike Hommey [:glandium]]

Attachment: Add build option for rel-eng type builds

This adds a failure to make package and make install when MOZILLA_OFFICIAL is set and --enable-release wasn't given. This may break c-c if they don't include mozconfig.common.
I'm not adding MOZILLA_OFFICIAL with --enable-release because non-official builds may still want to use --enable-release, and I'm not sure what MOZILLA_OFFICIAL does these days. This can be figured in a followup.

Comment 12

[(not currently active) Ted Mielczarek]

Not a whole lot:
Enabling Breakpad at runtime: http://mxr.mozilla.org/mozilla-central/source/build/application.ini#41
Showing about:rights: http://mxr.mozilla.org/mozilla-central/source/browser/modules/AboutHomeUtils.jsm#55
Telemetry: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/Telemetry.cpp#1848, http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#759

Comment 13

[(not currently active) Ted Mielczarek]

You could just make the build error in configure if you --enable-release without MOZILLA_OFFICIAL.

Comment 14

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #13)
> You could just make the build error in configure if you --enable-release
> without MOZILLA_OFFICIAL.

The point is to avoid MOZ_OFFICIAL builds without --enable-release, not the other way around, but yeah, that could be done in configure.

Comment 15

[Mike Hommey [:glandium]]

Actually, it might be better *not* in configure, because i suspect some developers are using MOZILLA_OFFICIAL to enable some of those in local builds, and that would block them. They are however less likely to do make package. And if they really need to, they can override one of the variables at make package time. It's harder to workaround if it fails in configure.

Comment 16

[(not currently active) Ted Mielczarek]

Comment on attachment 790155 [details] [diff] [review]
Add build option for rel-eng type builds

Review of attachment 790155 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +82,5 @@
>  AC_DEFUN([MOZ_COMPILER_OPTS],
>  [
> +  DEVELOPER_OPTIONS=1
> +  MOZ_ARG_ENABLE_BOOL(release,
> +  [  --enable-release       Build with more conservative, release engineering-oriented options.],

Might want to mention that it is likely to make builds slower.

::: build/mozconfig.common
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Common mozconfig for all release engineering builds

I would probably say "official builds".

Comment 17

[(not currently active) Ted Mielczarek]

Comment on attachment 790037 [details] [diff] [review]
Disable ICF and dead code removal on local builds

Review of attachment 790037 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +237,5 @@
>  
> +if test "$GNU_CC" -a -z "$DEVELOPER_OPTIONS"; then
> +    CFLAGS="$CFLAGS -ffunction-sections -fdata-sections"
> +    CXXFLAGS="$CXXFLAGS -ffunction-sections -fdata-sections"
> +fi

Can you not just stick this block inside the existing GNU_CC block below?

Comment 18

[(not currently active) Ted Mielczarek]

Comment on attachment 790036 [details] [diff] [review]
Force use gold, if possible, when the default linker is BFD ld, for local builds.

Review of attachment 790036 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/autoconf/compiler-opts.m4
@@ +225,5 @@
> +        esac
> +        if test -n "$GOLD"; then
> +            mkdir -p $_objdir/build/unix/gold
> +            ln -s "$GOLD" $_objdir/build/unix/gold/ld
> +            if $CC -B $_objdir/build/unix/gold -Wl,--version 2>&1 | grep -q "GNU gold"; then

This is a little wacky, but I guess there's no better way to do this.

@@ +226,5 @@
> +        if test -n "$GOLD"; then
> +            mkdir -p $_objdir/build/unix/gold
> +            ln -s "$GOLD" $_objdir/build/unix/gold/ld
> +            if $CC -B $_objdir/build/unix/gold -Wl,--version 2>&1 | grep -q "GNU gold"; then
> +                LDFLAGS="$LDFLAGS -B $_objdir/build/unix/gold"

What's the failure mode here? Just somehow this setup doesn't work?

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 790023 [details] [diff] [review]
Use -gsplit-dwarf when supported for local builds

Review of attachment 790023 [details] [diff] [review]:
-----------------------------------------------------------------

Not totally happy with this patch yet.

::: build/autoconf/ccache.m4
@@ +35,5 @@
>  fi
>  
> +dnl -gsplit-dwarf isn't supported by ccache, so it's dangerous to use both
> +if test -n "$MOZ_USING_CCACHE" && echo " $MOZ_DEBUG_FLAGS " | grep -q " -gsplit-dwarf "; then
> +    AC_MSG_ERROR([ccache does not support -gsplit-dwarf. Please choose to either build --without-ccache, or force not to use split-dwarf with --enable-debug-symbols=-g])

Since ccache isn't the default, it's probably more helpful to say "don't use --with-ccache".

This is an unfortunate tradeoff. Is the win from -gsplit-dwarf worth the cost of disabling ccache? Also, I'm a little worried that we're basically making --with-ccache useless.

Comment 20

[Mike Hommey [:glandium]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> > +# Common mozconfig for all release engineering builds
> 
> I would probably say "official builds".

Considering this is something things like icecat, iceweasel or palemoon should be using, "official builds" seems charged. Releng is not really clear but still better imho.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #18)
> > +                LDFLAGS="$LDFLAGS -B $_objdir/build/unix/gold"
> 
> What's the failure mode here? Just somehow this setup doesn't work?

Either using gold that way works and we use the flag, or it doesn't and we use BFD ld.

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #19)
> This is an unfortunate tradeoff. Is the win from -gsplit-dwarf worth the
> cost of disabling ccache? Also, I'm a little worried that we're basically
> making --with-ccache useless.

It depends what the usecase is. For incremental builds involving little changes, split-dwarf is a clear win. That's what cuts most of the linking time, and when you're iterating on small changes and relinking libxul a lot, it matters much more than ccache.

Note that a mozilla contributor has submitted patches to https://bugzilla.samba.org/show_bug.cgi?id=10005 . I don't know how well they work, but we could build and provide binaries, and add a test whether ccache supports the dwo files.

Comment 21

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #20)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #16)
> > > +# Common mozconfig for all release engineering builds
> > 
> > I would probably say "official builds".
> 
> Considering this is something things like icecat, iceweasel or palemoon
> should be using, "official builds" seems charged. Releng is not really clear
> but still better imho.

Ah, I realized this was about the mozconfig comment. Yeah, official builds would work, there.

Comment 22

[Mike Hommey [:glandium]]

I'm going to land the reviewed patches and keep the -gsplit-dwarf work for a followup (already cloned as bug 905646)

Comment 23

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/543772de681e
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f24ebad0ad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5150f990fad

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/543772de681e
https://hg.mozilla.org/mozilla-central/rev/6f24ebad0ad8
https://hg.mozilla.org/mozilla-central/rev/a5150f990fad

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Un-busting comm-central.
https://hg.mozilla.org/comm-central/rev/9de9edeff7d7

Comment 26

[Robert Kaiser]

So, does this mean that building B2G is broken by default now as MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter will not work) but --enable-release is probably not set by default? Sounds messy to me...

Comment 27

[Mike Hommey [:glandium]]

(In reply to Robert Kaiser (:[e-mail]) from comment #26)
> So, does this mean that building B2G is broken by default now as
> MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter
> will not work)

That's not entirely true. In fact, I'd argue MOZILLA_OFFICIAL is the wrong bit for that.

> but --enable-release is probably not set by default? Sounds
> messy to me...

--enable-release is set on all releng builds through mozconfig.

Comment 28

[Mike Hommey [:glandium]]

Note that if something is broken as a result of this landing, followup bugs can be filed.

Comment 29

[Justin Wood (:Callek)]

(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Robert Kaiser (:[e-mail]) from comment #26)
> > but --enable-release is probably not set by default? Sounds
> > messy to me...
> 
> --enable-release is set on all releng builds through mozconfig.

Except builds/compiles that don't use a mozconfig yet, (e.g. l10n dep builds, iirc call configure directly passing in specific arguments, and I have no idea what DXR or spidermonkey do top of my head, nevermind b2g builds)

Comment 30

[Robert Kaiser]

(In reply to Mike Hommey [:glandium] from comment #27)
> (In reply to Robert Kaiser (:[e-mail]) from comment #26)
> > So, does this mean that building B2G is broken by default now as
> > MOZILLA_OFFICIAL is enabled by default there (as without it, crash reporter
> > will not work)
> 
> That's not entirely true. In fact, I'd argue MOZILLA_OFFICIAL is the wrong
> bit for that.

Well, it has been the bit for that for ages (the reason was that back in the days, it was decided that crash reports from unofficial builds would be noise and unwanted).

> > but --enable-release is probably not set by default? Sounds
> > messy to me...
> 
> --enable-release is set on all releng builds through mozconfig.

Well, all official B2G builds are not created by releng but by manufacturers. I guess this patch is why Geeksphone's Nightly builds for today weren't produced.

Comment 31

[Mike Hommey [:glandium]]

(In reply to Robert Kaiser (:[e-mail]) from comment #30)
> Well, it has been the bit for that for ages (the reason was that back in the
> days, it was decided that crash reports from unofficial builds would be
> noise and unwanted).

I meant that MOZILLA_OFFICIAL is the wrong bit for b2g to enable the crash reporter. MOZILLA_OFFICIAL is not the only way to enable the crash reporter.

Comment 32

[Mike Hommey [:glandium]]

That being said, comment 28 still applies.

Comment 33

[Robert Kaiser]

And I said that MOZILLA_OFFICIAL has been required to enable crash reporter for ages. The more I read this and think about it, the more I actually think this bug should be backed out on the basis of that.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

So is there a way to disable -gsplit-dwarf easily?  I depend on being able to rsync dist/bin from a debug build to another machine and then debug the result, but I'd rather not have to track whatever other changes we make to debugging options.

Comment 35

[Mike Hommey [:glandium]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #34)
> So is there a way to disable -gsplit-dwarf easily?  I depend on being able
> to rsync dist/bin from a debug build to another machine and then debug the
> result, but I'd rather not have to track whatever other changes we make to
> debugging options.

-gsplit-dwarf is not there yet, it's moved to bug 905646.