PSA: Non-unified builds no longer occurring on central/inbound and friends

[Ehsan Akhgari]

From now on, the only supported build mode is unified compilation. I
am planning to follow-up with removing support for the
--disable-unified-compilation configure option altogether in bug 1121000.

This puts an end to the issue of burning non-unified builders when pushing.

[Steve Fink]

On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
> From now on, the only supported build mode is unified compilation. I
> am planning to follow-up with removing support for the
> --disable-unified-compilation configure option altogether in bug 1121000.

I commented in the bug, but I guess this is probably a better forum.

Why is the configure option being removed? I understand always building
unified in automation, but not having a straightforward way at all to
see if your code is buggy seems... suboptimal. If someone wants to go
through occasionally and make our codebase valid C++, how should they do
it after this change?

Perhaps the previous dev-platform thread would provide enlightenment and
I ought to go back and reread it :( , but I do not offhand recall it
deciding to remove the configure option altogether.

[ISHIKAWA,chiaki]

I have an issue here, too.

Debugging using gdb will be very difficult when the unified build
creates a source file on the fly (but it is removed, correct?).

No sane compiler/debugger combination can help me do
the source level debugging if the source code that the compiler compiled
is gone by the time debugger tries to find it...

Maybe I am missing something...

TIA

[Jonathan Kew]

On 15/1/15 15:56, ISHIKAWA,chiaki wrote:

> Debugging using gdb will be very difficult when the unified build
> creates a source file on the fly (but it is removed, correct?).
>
> No sane compiler/debugger combination can help me do
> the source level debugging if the source code that the compiler compiled
> is gone by the time debugger tries to find it...
>
> Maybe I am missing something...

This shouldn't be a problem. The "unified" source file simply #includes
the real sources, and so the debugger will know the paths to those.

JK

[Bobby Holley]

On Wed, Jan 14, 2015 at 5:37 PM, Steve Fink <sf...@mozilla.com> wrote:

> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
> > From now on, the only supported build mode is unified compilation. I
> > am planning to follow-up with removing support for the
> > --disable-unified-compilation configure option altogether in bug 1121000.
>
> I commented in the bug, but I guess this is probably a better forum.
>
> Why is the configure option being removed? I understand always building
> unified in automation, but not having a straightforward way at all to
> see if your code is buggy seems... suboptimal. If someone wants to go
> through occasionally and make our codebase valid C++, how should they do
> it after this change?
>

IIUC, in the absence of somebody committed to doing this on a daily basis,
we will quickly slide towards hundreds of include errors, and the configure
option will quickly become useless.

bholley

[Sylvestre Ledru]

On 15/01/2015 16:56, ISHIKAWA,chiaki wrote:

> On 2015/01/15 10:37, Steve Fink wrote:
>> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>>> From now on, the only supported build mode is unified compilation. I
>>> am planning to follow-up with removing support for the
>>> --disable-unified-compilation configure option altogether in bug
>>> 1121000.
>>
>> I commented in the bug, but I guess this is probably a better forum.
>>
>> Why is the configure option being removed? I understand always building
>> unified in automation, but not having a straightforward way at all to
>> see if your code is buggy seems... suboptimal. If someone wants to go
>> through occasionally and make our codebase valid C++, how should they do
>> it after this change?
>>

>> Perhaps the previous dev-platform thread would provide enlightenment and
>> I ought to go back and reread it :( , but I do not offhand recall it
>> deciding to remove the configure option altogether.
>>
>
> I have an issue here, too.
>

> Debugging using gdb will be very difficult when the unified build
> creates a source file on the fly (but it is removed, correct?).
>
> No sane compiler/debugger combination can help me do
> the source level debugging if the source code that the compiler
> compiled is gone by the time debugger tries to find it...
>
> Maybe I am missing something...

I have a similar question about static analysis tools (clang analyzer,
coverity, etc).

Doing the SA on the whole unified file is going to produce some useless
results if all the files content gets merged.
But maybe I am just missing the point.

Cheers,
Sylvestre

[Chris Peterson]

On 1/14/15 5:37 PM, Steve Fink wrote:
> Why is the configure option being removed? I understand always building
> unified in automation, but not having a straightforward way at all to
> see if your code is buggy seems... suboptimal. If someone wants to go
> through occasionally and make our codebase valid C++, how should they do
> it after this change?

Another small benefit of optional non-unified builds is that clang does
a better job of reporting -Wunused-variable warnings with smaller
translation units. I assume that the number of identifiers in the
unified files exceed some clang analysis limit.


chris

[Steve Fink]

On 01/15/2015 09:31 AM, Bobby Holley wrote:

> On Wed, Jan 14, 2015 at 5:37 PM, Steve Fink <sf...@mozilla.com
> <mailto:sf...@mozilla.com>> wrote:
>
> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
> > From now on, the only supported build mode is unified
> compilation. I
> > am planning to follow-up with removing support for the
> > --disable-unified-compilation configure option altogether in bug
> 1121000.
>
> I commented in the bug, but I guess this is probably a better forum.
>

> Why is the configure option being removed? I understand always
> building
> unified in automation, but not having a straightforward way at all to
> see if your code is buggy seems... suboptimal. If someone wants to go
> through occasionally and make our codebase valid C++, how should
> they do
> it after this change?
>
>

> IIUC, in the absence of somebody committed to doing this on a daily
> basis, we will quickly slide towards hundreds of include errors, and
> the configure option will quickly become useless.

If that is the case, then adding a file has the potential of exposing a
bunch of those include errors, if it perturbs the chunking boundaries.

But we've already had that argument, and decided in favor of always
building unified in automation. I think the question of removing the
configure option is separate. It's ok if we gradually accumulate these
errors. I just don't want to make life unnecessarily hard for the
occasional angelic figure with supernatural papercut fixing abilities
(hi cpeterson!).

And if this is actually useful to a subset of people (eg IDE users),
then perhaps the debt won't build up too much. Personally, I do
occasionally run a build, grab the compile command, change the
*unified*.cpp file to just the file I'm interested in, and rerun. But
that's a very niche use.

[Daniel Holbert]

On 01/15/2015 09:46 AM, Chris Peterson wrote:
> Another small benefit of optional non-unified builds is that clang does
> a better job of reporting -Wunused-variable warnings with smaller
> translation units. I assume that the number of identifiers in the
> unified files exceed some clang analysis limit.

(I don't think it's a number-of-identifiers thing.

In the cases I've seen, e.g. bug 1008083, clang doesn't report some
variables as "unused" in #included files because it assumes the
#included thing is a header, and the variable might be used in some
*other* translation unit that includes this header.)

[Steve Fink]

On 01/15/2015 09:39 AM, Sylvestre Ledru wrote:
> On 15/01/2015 16:56, ISHIKAWA,chiaki wrote:

>> On 2015/01/15 10:37, Steve Fink wrote:
>>> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>>>> From now on, the only supported build mode is unified compilation. I
>>>> am planning to follow-up with removing support for the
>>>> --disable-unified-compilation configure option altogether in bug
>>>> 1121000.
>>> I commented in the bug, but I guess this is probably a better forum.
>>>
>>> Why is the configure option being removed? I understand always building
>>> unified in automation, but not having a straightforward way at all to
>>> see if your code is buggy seems... suboptimal. If someone wants to go
>>> through occasionally and make our codebase valid C++, how should they do
>>> it after this change?
>>>

>>> Perhaps the previous dev-platform thread would provide enlightenment and
>>> I ought to go back and reread it :( , but I do not offhand recall it
>>> deciding to remove the configure option altogether.
>>>
>> I have an issue here, too.
>>
>> Debugging using gdb will be very difficult when the unified build
>> creates a source file on the fly (but it is removed, correct?).
>>
>> No sane compiler/debugger combination can help me do
>> the source level debugging if the source code that the compiler
>> compiled is gone by the time debugger tries to find it...
>>
>> Maybe I am missing something...
> I have a similar question about static analysis tools (clang analyzer,
> coverity, etc).
>
> Doing the SA on the whole unified file is going to produce some useless
> results if all the files content gets merged.
> But maybe I am just missing the point.

So far, that's worked out for the things I've looked at. gdb and the
hazard analysis both end up obeying the #line directives that give the
original source filename. There are occasionally problems using emacs to
jump to error messages, I can't remember why, but it hasn't been too bad.

But I don't know about the other static analyses.

[Ehsan Akhgari]

Yes, clang can handle all of this very well. So can every other
compiler on the planet. :-)

[Benjamin Kelly]

On Wed, Jan 14, 2015 at 8:37 PM, Steve Fink <sf...@mozilla.com> wrote:

> Why is the configure option being removed? I understand always building
> unified in automation, but not having a straightforward way at all to
> see if your code is buggy seems... suboptimal. If someone wants to go
> through occasionally and make our codebase valid C++, how should they do
> it after this change?
>

For what its worth, you can still verify individual directories by changing
moz.build to use SOURCES instead of UNIFIED_SOURCES.

Ben

[Ehsan Akhgari]

On 2015-01-15 1:49 PM, Daniel Holbert wrote:
> On 01/15/2015 09:46 AM, Chris Peterson wrote:

>> Another small benefit of optional non-unified builds is that clang does
>> a better job of reporting -Wunused-variable warnings with smaller
>> translation units. I assume that the number of identifiers in the
>> unified files exceed some clang analysis limit.
>

> (I don't think it's a number-of-identifiers thing.
>
> In the cases I've seen, e.g. bug 1008083, clang doesn't report some
> variables as "unused" in #included files because it assumes the
> #included thing is a header, and the variable might be used in some
> *other* translation unit that includes this header.)

Yeah, that is definitely one of the implications of unified builds.

[Ehsan Akhgari]

On 2015-01-14 8:37 PM, Steve Fink wrote:
> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>> From now on, the only supported build mode is unified compilation. I
>> am planning to follow-up with removing support for the
>> --disable-unified-compilation configure option altogether in bug 1121000.
>
> I commented in the bug, but I guess this is probably a better forum.
>

> Why is the configure option being removed?

Because unsupported build configurations that are *guaranteed* to not be
maintained are not worth keeping around. I am 90% sure that
--disable-unified-compilation would have been broken since yesterday. :-)

> I understand always building
> unified in automation, but not having a straightforward way at all to
> see if your code is buggy seems... suboptimal. If someone wants to go
> through occasionally and make our codebase valid C++, how should they do
> it after this change?

Switch the UNIFIED_SOURCES variable to SOURCES in the moz.build file.
That still works.

[Mike Hommey]

On Thu, Jan 15, 2015 at 02:11:16PM -0500, Benjamin Kelly wrote:
> For what its worth, you can still verify individual directories by changing
> moz.build to use SOURCES instead of UNIFIED_SOURCES.

On Thu, Jan 15, 2015 at 02:21:25PM -0500, Ehsan Akhgari wrote:
> Switch the UNIFIED_SOURCES variable to SOURCES in the moz.build file. That
> still works.

Instead of that, set FILES_PER_UNIFIED_FILE to 1.

Mike

[Seth Fowler]

What’s a bit unfortunate about these approaches is that you can accidentally qref them into your patch. I’ve managed to do so repeatedly.

- Seth

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Ehsan Akhgari]

On 2015-01-15 5:57 PM, Seth Fowler wrote:
> What’s a bit unfortunate about these approaches is that you can accidentally qref them into your patch. I’ve managed to do so repeatedly.

That's true of any local change, right?

[Seth Fowler]

It wouldn’t be true of, say, a mach argument specifying directories that should be built non-unified.

Not that it matters nearly as much now that we’ve made the decision not to support non-unified builds.

[Ehsan Akhgari]

On 2015-01-15 6:46 PM, Seth Fowler wrote:
> It wouldn’t be true of, say, a mach argument specifying directories that should be built non-unified.
>
> Not that it matters nearly as much now that we’ve made the decision not to support non-unified builds.

Yeah that's true, but that mach command would break people's builds
constantly. I don't see any value in supporting that, *unless* we
actually see a flurry of patches coming in to fix non-unified builds.
When and if that happens, I would happily change my mind. :-)

[Steve Fink]

On 01/15/2015 11:21 AM, Ehsan Akhgari wrote:
> On 2015-01-14 8:37 PM, Steve Fink wrote:
>> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>>> From now on, the only supported build mode is unified compilation. I
>>> am planning to follow-up with removing support for the
>>> --disable-unified-compilation configure option altogether in bug
>>> 1121000.
>>
>> I commented in the bug, but I guess this is probably a better forum.
>>
>> Why is the configure option being removed?
>
> Because unsupported build configurations that are *guaranteed* to not
> be maintained are not worth keeping around. I am 90% sure that
> --disable-unified-compilation would have been broken since yesterday.
> :-)

But that's not the point. I totally agree that non-unified builds will
get broken immediately and will stay broken most of the time. That's not
in question. The point is how hard it is for someone who comes along and
wants to fix our codebase to be valid C++, even if that fix is not
permanent.

Specifically, what I imagine happening is this: we start out nowish with
nonunified builds working. Within a day, they're busted. After some
time, somebody who gains from them being correct wants to fix them, and
uses --disable-unified-builds to do so. Rinse, repeat. The amount of
breakage to fix each time is determined by the time between fixes.

If this hypothetical person has to go through every moz.build and change
UNIFIED_SOURCES to SOURCES, or FILES_PER_UNIFIED_FILE to 1, then they're
less likely to do it, and the interval between fixes will grow, and
eventually the code base will be rotten enough that nobody will want to
deal with it. Oh, except that people will occasionally add new files and
uncover breakage that wasn't their fault because they shift around the
boundaries. Which isn't awful, because you'll only have to fix a
localized area, but it is guaranteed to baffle many of the people who
hit it until they've been educated. More overhead to contributing, more
"specialness" in our codebase.

If you were saying that the --disable-unified-builds mechanism itself is
likely to break, then I couldn't really argue with removing it. Is there
a simpler way to accomplish the same thing? A
FORCE_MAX_FILES_PER_UNIFIED_FILE_EVERYWHERE=1 setting in the toplevel
moz.build or something? It doesn't need to be pretty, but it should be
easier than

for f in **/moz.build; do echo "FILES_PER_UNIFIED_FILE=1" >> $f; done

(assuming that even works, and note that you'll need zsh for the '**'
thing).

>
> > I understand always building
>> unified in automation, but not having a straightforward way at all to
>> see if your code is buggy seems... suboptimal. If someone wants to go
>> through occasionally and make our codebase valid C++, how should they do
>> it after this change?
>

[Ehsan Akhgari]

There are many ways to force UNIFIED_SOURCES to be built in non-unified
mode. Reverting my patch is one of them. Applying the following patch
is another:

diff --git a/python/mozbuild/mozbuild/backend/recursivemake.py
b/python/mozbuild/mozbuild/backend/recursivemake.py
index 608f6f5..de754b6 100644
--- a/python/mozbuild/mozbuild/backend/recursivemake.py
+++ b/python/mozbuild/mozbuild/backend/recursivemake.py
@@ -394,7 +394,7 @@ class RecursiveMakeBackend(CommonBackend):
non_unified_var = var[len('UNIFIED_'):]

files_per_unification = obj.files_per_unified_file
- do_unify = files_per_unification > 1
+ do_unify = False
# Sorted so output is consistent and we don't bump mtimes.
source_files = list(sorted(obj.files))


Part of what you're complaining about above is not about removing
support for the configure option, it's about dropping support for
non-unified builds on infrastructure. Yes, there are definitely
downsides to what we decided to do. Our code is no longer "valid C++"
(which honestly is a very arbitrary line -- our code has never been
valid C++ from a puristic standpoint any way because of all of the
compiler specific extensions that we use). And yes, there will be the
potential for breakage when you add or remove unified sources. And that
breakage would not be your fault, and you would need to be educated
about what's going on, unless you have experience with another project
that uses the same idea (which is unlikely.)

But there are upsides too. You won't get backed out because you broke a
build configuration that we do not ship. We don't pay the monetary and
human resource cost of keeping it working. And you will get *fast*
builds. I'd like to remind folks that unified builds improved our build
times by more than 2x.

So it is a trade-off. It's completely fine if you disagree on the call
that we made there, but I'd like to keep that conversation separate with
the one about how to build the tree locally in non-unified mode.

For the latter conversation, there are multiple ways. I don't think we
should accept such patches because 1) we already have too many
configure/etc options that break your build, and we keep problem reports
from people who have options in their mozconfigs that are unsupported
and cause broken builds (Please see the archives of
<https://groups.google.com/forum/#!forum/mozilla.dev.builds>.) and 2)
because I have yet to see any evidence to suggest that there will be
people who will maintain that build configuration to work. However, I
very well realize that both of these points are my opinion. This is
ultimately gps/glandium's call, and if they disagree with me, I won't
fight more on this hill.

However, I very strongly believe that the trade-offs for whether we
should do these builds *on the infrastructure* tip very heavily towards
not doing so. Not sure whose call that will ultimately be, but I would
be extremely sad to impose the cost of backouts because of breaking a
configuration that we don't ship to our developers.

[Martin Thomson]

On Thu, Jan 15, 2015 at 4:30 PM, Steve Fink <sf...@mozilla.com> wrote:

> But that's not the point. I totally agree that non-unified builds will
> get broken immediately and will stay broken most of the time. That's not
> in question. The point is how hard it is for someone who comes along and
> wants to fix our codebase to be valid C++, even if that fix is not
> permanent.

Why would that even be a goal? If the project builds, is that not
sufficient? I don't see any value in some platonic "correctness".

[Steve Fink]

On 01/15/2015 04:52 PM, Ehsan Akhgari wrote:
> On 2015-01-15 7:30 PM, Steve Fink wrote:
>> On 01/15/2015 11:21 AM, Ehsan Akhgari wrote:
>>> On 2015-01-14 8:37 PM, Steve Fink wrote:
>>>> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>>>>> From now on, the only supported build mode is unified
>>>>> compilation. I
>>>>> am planning to follow-up with removing support for the
>>>>> --disable-unified-compilation configure option altogether in bug
>>>>> 1121000.
>>>>
>>>> I commented in the bug, but I guess this is probably a better forum.
>>>>
>>>> Why is the configure option being removed?
>>>
>>> Because unsupported build configurations that are *guaranteed* to not
>>> be maintained are not worth keeping around. I am 90% sure that
>>> --disable-unified-compilation would have been broken since yesterday.
>>> :-)
>>

>> But that's not the point. I totally agree that non-unified builds will
>> get broken immediately and will stay broken most of the time. That's not
>> in question. The point is how hard it is for someone who comes along and
>> wants to fix our codebase to be valid C++, even if that fix is not
>> permanent.
>>

This is a little cryptic, but something similar to this does seem
preferable to keeping all of the code for --disable-unified-compilation.
It just needs to be discoverable by the people who would usefully
discover it.

> Part of what you're complaining about above is not about removing
> support for the configure option, it's about dropping support for
> non-unified builds on infrastructure.

No, because I agree with that decision.

> Yes, there are definitely downsides to what we decided to do. Our
> code is no longer "valid C++" (which honestly is a very arbitrary line
> -- our code has never been valid C++ from a puristic standpoint any
> way because of all of the compiler specific extensions that we use).

I don't think that's really fair. I don't give a rip about sticking to
only c++0x or whatever. Our code conforms to an even stricter standard,
namely the common subset supported by a collection of different compilers.

What I'm concerned about is code that doesn't compile on any compiler if
you do it the "normal" way, one file at a time. Code that uses symbols
that are totally unknown or not in any using'd namespace. Code referring
to mysterious ALL_CAPS_SYMBOLS (that happen to be defined as macros in
headers that are not #included.) Code that depends on template
specializations from a non-included file for correctness. Unified
compilation creates dummy .cpp files that #include <dogs> and <cats> and
force them to live together.

It's bad for IDEs, it's bad for people reading the code ("what is this
Value thing? Oh... some other .cpp file must be using JS::Value..."),
it's a hazard when adding/removing files, and it's a problem when you
want to dig into one source file in particular perhaps by generating a
.i for it.

> And yes, there will be the potential for breakage when you add or
> remove unified sources. And that breakage would not be your fault,
> and you would need to be educated about what's going on, unless you
> have experience with another project that uses the same idea (which is
> unlikely.)
>
> But there are upsides too. You won't get backed out because you broke
> a build configuration that we do not ship. We don't pay the monetary
> and human resource cost of keeping it working. And you will get
> *fast* builds. I'd like to remind folks that unified builds improved
> our build times by more than 2x.

Er, I was trying to keep that separate too, since I happen to agree with
that decision. Well, with some misgivings, but all in all I prefer
turning off nonunified builds in automation. (I'd kinda like to keep
building one across all platforms daily or weekly or something, treating
it as non-tier-1, but I can believe that it might end up looked at by
exactly zero people.)

> So it is a trade-off. It's completely fine if you disagree on the
> call that we made there, but I'd like to keep that conversation
> separate with the one about how to build the tree locally in
> non-unified mode.

As would I.

> For the latter conversation, there are multiple ways. I don't think
> we should accept such patches because 1) we already have too many
> configure/etc options that break your build, and we keep problem
> reports from people who have options in their mozconfigs that are
> unsupported and cause broken builds (Please see the archives of
> <https://groups.google.com/forum/#!forum/mozilla.dev.builds>.) and 2)
> because I have yet to see any evidence to suggest that there will be
> people who will maintain that build configuration to work. However, I
> very well realize that both of these points are my opinion. This is
> ultimately gps/glandium's call, and if they disagree with me, I won't
> fight more on this hill.

I am also uncertain of #2.

> However, I very strongly believe that the trade-offs for whether we
> should do these builds *on the infrastructure* tip very heavily
> towards not doing so. Not sure whose call that will ultimately be,
> but I would be extremely sad to impose the cost of backouts because of
> breaking a configuration that we don't ship to our developers.

Agreed. My main concern is the added difficulty of contributing to the
code base when there are unified bugs lurking. I agree with you, that
cost is less than the daily cost of nonunified bustage backouts.

[Trevor Saunders]

On Thu, Jan 15, 2015 at 06:39:56PM +0100, Sylvestre Ledru wrote:
>
> On 15/01/2015 16:56, ISHIKAWA,chiaki wrote:

> > On 2015/01/15 10:37, Steve Fink wrote:
> >> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
> >>> From now on, the only supported build mode is unified compilation. I
> >>> am planning to follow-up with removing support for the
> >>> --disable-unified-compilation configure option altogether in bug
> >>> 1121000.
> >>
> >> I commented in the bug, but I guess this is probably a better forum.
> >>

> >> Why is the configure option being removed? I understand always building

> >> unified in automation, but not having a straightforward way at all to
> >> see if your code is buggy seems... suboptimal. If someone wants to go
> >> through occasionally and make our codebase valid C++, how should they do
> >> it after this change?
> >>

> >> Perhaps the previous dev-platform thread would provide enlightenment and
> >> I ought to go back and reread it :( , but I do not offhand recall it
> >> deciding to remove the configure option altogether.
> >>
> >
> > I have an issue here, too.
> >
> > Debugging using gdb will be very difficult when the unified build
> > creates a source file on the fly (but it is removed, correct?).
> >
> > No sane compiler/debugger combination can help me do
> > the source level debugging if the source code that the compiler
> > compiled is gone by the time debugger tries to find it...
> >
> > Maybe I am missing something...
> I have a similar question about static analysis tools (clang analyzer,
> coverity, etc).
>
> Doing the SA on the whole unified file is going to produce some useless
> results if all the files content gets merged.

why? In general I would expect it to work better with unified
compilation since it can see more code at once.

Trev

> But maybe I am just missing the point.
>

> Cheers,
> Sylvestre

[Ehsan Akhgari]

On 2015-01-16 2:10 AM, Steve Fink wrote:
> On 01/15/2015 04:52 PM, Ehsan Akhgari wrote:
>> On 2015-01-15 7:30 PM, Steve Fink wrote:
>>> On 01/15/2015 11:21 AM, Ehsan Akhgari wrote:

>>>> On 2015-01-14 8:37 PM, Steve Fink wrote:
>>>>> On 01/14/2015 11:26 AM, Ehsan Akhgari wrote:
>>>>>> From now on, the only supported build mode is unified
>>>>>> compilation. I
>>>>>> am planning to follow-up with removing support for the
>>>>>> --disable-unified-compilation configure option altogether in bug
>>>>>> 1121000.
>>>>>
>>>>> I commented in the bug, but I guess this is probably a better forum.
>>>>>
>>>>> Why is the configure option being removed?
>>>>

Can you think of something better/more obvious?

>> Part of what you're complaining about above is not about removing
>> support for the configure option, it's about dropping support for
>> non-unified builds on infrastructure.
>
> No, because I agree with that decision.

OK, that's good to know. :-)

>> Yes, there are definitely downsides to what we decided to do. Our
>> code is no longer "valid C++" (which honestly is a very arbitrary line
>> -- our code has never been valid C++ from a puristic standpoint any
>> way because of all of the compiler specific extensions that we use).
>
> I don't think that's really fair. I don't give a rip about sticking to
> only c++0x or whatever. Our code conforms to an even stricter standard,
> namely the common subset supported by a collection of different compilers.
>
> What I'm concerned about is code that doesn't compile on any compiler if
> you do it the "normal" way, one file at a time. Code that uses symbols
> that are totally unknown or not in any using'd namespace. Code referring
> to mysterious ALL_CAPS_SYMBOLS (that happen to be defined as macros in
> headers that are not #included.) Code that depends on template
> specializations from a non-included file for correctness. Unified
> compilation creates dummy .cpp files that #include <dogs> and <cats> and
> force them to live together.

Here is the issue that I have with what you're saying. It has never
been the case that you can take one of our cpp files, and just pass it
to the compiler and get an object file out of it. We use disgusting
hacks to mask the path to #included files (I'm looking at
LOCAL_INCLUDES/GENERATED_INCLUDES) instead of doing #include
"path/to/file.h". We require you to pass things such as
-DMOZILLA_INTERNAL_API on the command line for almost all of the code in
our tree (but not all of it!). We require you to specify -include
../../../mozilla-config.h and whatnot.

So, I agree with your assertion that this makes it harder to take a .cpp
file, and compile it on its own. But doing that without the help of the
build system has never been possible. And once you start to beg the
build system for the exact command line, you can benefit from things
such as the above patch. Why is this not acceptable to you?

> It's bad for IDEs,

Ben Turner pointed out to me on IRC yesterday that Visual Studio will
have a hard time because of this, and he's right, but that's because the
Visual Studio project generator is broken. It should add the
UnifiedXXX.cpp files to the project, not FileIncludedInUnified.cpp. So
that's solvable.

> it's bad for people reading the code ("what is this
> Value thing? Oh... some other .cpp file must be using JS::Value..."),
> it's a hazard when adding/removing files, and it's a problem when you
> want to dig into one source file in particular perhaps by generating a
> .i for it.

Yes, these problems do exist.

Yeah, I acknowledge that if a new contributor adds a .cpp file and
suddenly gets build failures in another file, that's crappy and
non-obvious. That makes me sad, but I have not been able to come up
with a solution that doesn't leave a sour taste in my mouth. :/

[Nathan Froyd]

On Fri, Jan 16, 2015 at 3:17 PM, Ehsan Akhgari <ehsan....@gmail.com>
wrote:

> Ben Turner pointed out to me on IRC yesterday that Visual Studio will have
> a hard time because of this, and he's right, but that's because the Visual
> Studio project generator is broken. It should add the UnifiedXXX.cpp files
> to the project, not FileIncludedInUnified.cpp. So that's solvable.

This has already been fixed! (Well, not for bindings files or ipdl files,
but those were pre-existing problems.)

-Nathan

[Chris Peterson]

On 1/16/15 12:17 PM, Ehsan Akhgari wrote:
>>> diff --git a/python/mozbuild/mozbuild/backend/recursivemake.py
>>> b/python/mozbuild/mozbuild/backend/recursivemake.py
>>> index 608f6f5..de754b6 100644
>>> --- a/python/mozbuild/mozbuild/backend/recursivemake.py
>>> +++ b/python/mozbuild/mozbuild/backend/recursivemake.py
>>> @@ -394,7 +394,7 @@ class RecursiveMakeBackend(CommonBackend):
>>> non_unified_var = var[len('UNIFIED_'):]
>>>
>>> files_per_unification = obj.files_per_unified_file
>>> - do_unify = files_per_unification > 1
>>> + do_unify = False
>>> # Sorted so output is consistent and we don't bump mtimes.
>>> source_files = list(sorted(obj.files))
>>
>> This is a little cryptic, but something similar to this does seem
>> preferable to keeping all of the code for --disable-unified-compilation.
>> It just needs to be discoverable by the people who would usefully
>> discover it.
>
> Can you think of something better/more obvious?

recursivemake.py could allow the user to override files_per_unification
with an environment variable like MOZ_FILES_PER_UNIFICATION or
MOZ_DISABLE_UNIFIED_COMPILATION (files_per_unification = 1). This would
not depend on mozconfig and configure support for
--disable-unified-compilation.


chris

[Mike Hommey]

> >>diff --git a/python/mozbuild/mozbuild/backend/recursivemake.py
> >>b/python/mozbuild/mozbuild/backend/recursivemake.py
> >>index 608f6f5..de754b6 100644
> >>--- a/python/mozbuild/mozbuild/backend/recursivemake.py
> >>+++ b/python/mozbuild/mozbuild/backend/recursivemake.py
> >>@@ -394,7 +394,7 @@ class RecursiveMakeBackend(CommonBackend):
> >> non_unified_var = var[len('UNIFIED_'):]
> >>
> >> files_per_unification = obj.files_per_unified_file
> >>- do_unify = files_per_unification > 1
> >>+ do_unify = False
> >> # Sorted so output is consistent and we don't bump mtimes.
> >> source_files = list(sorted(obj.files))
> >
> >This is a little cryptic, but something similar to this does seem
> >preferable to keeping all of the code for --disable-unified-compilation.
> >It just needs to be discoverable by the people who would usefully
> >discover it.
>
> Can you think of something better/more obvious?

Add this to toplevel moz.build:

FILES_PER_UNIFIED_FILE = 1
export('FILES_PER_UNIFIED_FILE')

Mike

[Ehsan Akhgari]

I prefer the environment variable.

But I urge everyone to please wait to see if we start getting fixes for
non-unified builds from now on before adding this.

[ISHIKAWA,chiaki]

On 2015/01/16 1:08, Jonathan Kew wrote:

> On 15/1/15 15:56, ISHIKAWA,chiaki wrote:
>
>> Debugging using gdb will be very difficult when the unified build
>> creates a source file on the fly (but it is removed, correct?).
>>
>> No sane compiler/debugger combination can help me do
>> the source level debugging if the source code that the compiler compiled
>> is gone by the time debugger tries to find it...
>>
>> Maybe I am missing something...
>
>

> This shouldn't be a problem. The "unified" source file simply #includes
> the real sources, and so the debugger will know the paths to those.
>
> JK
>

I see, thank you.

If things don't work out with -gsplit-dwarf [there still
can be some rough edges every now and then], I will
simply apply the trick mentioned in this thread to virtually disable
uniform build on my local build.

BTW, I recall that Joshua Cranmer's coverage report system may not work
well with uniform build the last time I checked.

TIA

CI

[Ehsan Akhgari]

Please file bugs about the specific issues that you encounter in the future.

[ISHIKAWA,chiaki]

I did a non-unified build and saw the expected failure.
This is a summary of what I saw.

Background:

I may need to modify and debug basic I/O routines on local PC, and so
want to avoid unnecessary compilation. I use ccache locally to make
sure I can avoid re-compilation of touched but not modified C++ source
files (files get touched and remain unmodified when I execute
"hg qpop" and "hg qpush" in successions to work on different patches.
Without ccache, I have to compile many files. ccache helps a lot.)

There is a different perspective on unified compilation.

Compiler farm users:
One time fast compilation　is very important.
So unified compilation is a win.
(I suspect precompiled headers, -pch, would be a good win, too.)

Developers who repeats "edit a small set of files, compile and link"
many times on local PC:

He/she may modify only a few files and want quick
turn around of the compile of a few files and link time.

Unified compilation actually compiles more lines than he/she wants
(because of the extra source lines included in unified source files
in which his/her modified files are also included.
(Correct? Am I missing something here?)
So he/she may not like unified compilation in such scenario.

He/she can live with a long initial compilation time of whole tree
after refreshing the source. He/she can do that overnight and
continue edit a few files, compile and link the next day, and there
the quick turn around of the compilation of the exactly the
modified files will be appreciated.
(actually I think I saw a slow down of ccache due to the more lines
of input code checked for identity.)

With this in mind, I checked whether the suggestion from Mike Hommey
on Jan 17 (JST) to disable unified compilation works to produce a
successful non-unified build.

>> Can you think of something better/more obvious?
>

>Add this to toplevel moz.build:
>
>FILES_PER_UNIFIED_FILE = 1
>export('FILES_PER_UNIFIED_FILE')
>

The build breaks as was predicted in the mailing list.
The tree is already rotten in the sense of correct INDIVIDUAL C++ source
files.

19 files fail to compile due to missing or incorrectly ordered (?)
declarations (I think).

(This is what was observed during C-C thunderbird build.)

Compilation fails for the following files.

mozilla/dom/base/DOMException.cpp
mozilla/dom/fetch/Request.cpp
mozilla/dom/ipc/TabChild.cpp
mozilla/dom/ipc/nsIContentChild.cpp
mozilla/dom/media/MediaManager.cpp
mozilla/dom/media/eme/MediaKeyStatusMap.cpp
mozilla/gfx/layers/apz/util/APZCCallbackHelper.cpp
mozilla/gfx/layers/apz/util/ChromeProcessController.cpp
mozilla/gfx/thebes/gfxFont.cpp
mozilla/gfx/thebes/gfxFontEntry.cpp
mozilla/gfx/thebes/gfxHarfBuzzShaper.cpp
mozilla/js/ipc/JavaScriptShared.cpp
mozilla/layout/xul/nsResizerFrame.cpp
mozilla/media/libstagefright/binding/SinfParser.cpp
mozilla/netwerk/base/nsBaseChannel.cpp
mozilla/tools/profiler/ProfileEntry.cpp
mozilla/widget/VsyncDispatcher.cpp
mozilla/xpcom/build/MainThreadIOLogger.cpp

mozilla/intl/icu/source/i18n/rbnf.cpp
<--- This may have failed because I forced "-DDEBUG" on
compiler command line and a buggy code inside #if DEBUG,#endif
gets compiled. So probably not related to unified compilation
at all.

After seeing this failure, I think the following is what a developer
tinkering with edit, compile and link cycle on a local PC can do to
minimize unnecessary compilation of extra lines:

Do a unified compilation across the tree, but maybe enables
FILES_PER_UNIFIED_FILE=1 in moz.build of the particular directory
where he/she needs to edit files, to avoid unnecessary compilation (of
included files).

And if he/she needs to work in a directory where a few files (among 19
files above) don't compile, he/she has to figure out what the exact
cause of the compilation and fix it to use a compile-a-file-at-a-time
compilation scheme if that is preferred.
He/she better submit patches to keep the tree in good shape.

In my case, I may have to modify a few files under
mozilla/netwerk/base/ in order to debug TB's failure to use buffered
output during message downloading. nsBaseChannel.cpp fails to compile
under that directory. It is not quite easy to figure out whether the
cause of the failure can be figured out by myself.

On the other hand, if the build result in only one unified-compilation
(if there is only one file being modified), then the absolute
compilation time is not that long. So a developer who performs local
edit, compile and like cycles often can live with it hopefully.

Any better suggestion?
If everybody has a VERY FAST CPU LOCALLY, maybe this is not an issue...

TIA

PS: I can send the compilation log on request.
The compilation problems seem to be easy to fix for someone who knows
the details of header inclusion relationship, etc. This is my optimistic
assumption, but in reality, it may be very complex.

PPS: I wonder if the source tree is re-organized for
unified-compilation, using -pch to use pre-compiled headers
now is now a very viable option. Isn't it?

[Mike Hommey]

On Tue, Feb 03, 2015 at 02:27:52PM +0900, ISHIKAWA,chiaki wrote:
> I did a non-unified build and saw the expected failure.
> This is a summary of what I saw.
>
> Background:
>
> I may need to modify and debug basic I/O routines on local PC, and so
> want to avoid unnecessary compilation. I use ccache locally to make
> sure I can avoid re-compilation of touched but not modified C++ source
> files (files get touched and remain unmodified when I execute
> "hg qpop" and "hg qpush" in successions to work on different patches.
> Without ccache, I have to compile many files. ccache helps a lot.)
>
> There is a different perspective on unified compilation.
>
> Compiler farm users:
> One time fast compilation　is very important.
> So unified compilation is a win.
> (I suspect precompiled headers, -pch, would be a good win, too.)
>
> Developers who repeats "edit a small set of files, compile and link"
> many times on local PC:
>
> He/she may modify only a few files and want quick
> turn around of the compile of a few files and link time.
>
> Unified compilation actually compiles more lines than he/she wants
> (because of the extra source lines included in unified source files
> in which his/her modified files are also included.
> (Correct? Am I missing something here?)
> So he/she may not like unified compilation in such scenario.

Here's my take on this: yes, we should optimize for build times when
code is modified.

But here's the thing: in most directories, unified compilation shouldn't
be making a huge difference. That is, compiling one unified source vs.
compiling one source shouldn't make a big difference. If it does (and it
does in some directories like js/src), then the number of unified
sources in the directory where it's a problem should be adjusted.

Mike

[ISHIKAWA,chiaki]

Mike, thank you for the comment.
I suspect this is indeed the case in many directories.
(I mean unless a change of a single file caused 20 or 30 files to be
included into a unified source, then it is an overhead certainly. But so
far, the upper-bound of single change of a file is less than a couple of
minutes including the link with -gsplit-dwarf.)

I will report if I find a file, when touched, causes an extraordinarily
long compilation time (by including many of the source files during
unified compilation).

By the way, I saw Unified_binding_*.cpp files during compilation, and I
suspect they are different types of unified compilation since this
"unified_binding" compilation seems to occur no matter what the setting
of FILES_PER_UNIFIED_FILE.

TIA

CI