959519 - All signaling and a couple gfx/layers files are rebuilt every time

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Mike Hommey [:glandium]]

What happens is that all these files depend on mozilla-config.h because a couple headers contain direct #include "mozilla-config.h" (which in itself is debatable).

Most objects are built with -include $(DEPTH)/mozilla-config.h, which results in $(DEPTH)/mozilla-config.h in the object dependencies. But for file that include those headers that contain the #include "mozilla-config.h", those dependencies end up on $(DEPTH)/dist/include/mozilla-config.h.

But, at the beginning of the build, dist/include is blown up, and mozilla-config.h is installed when traversing config/. (which, strangely, copies the file instead of symlinking it)

So every time dist/include/mozilla-config.h is new, and we rebuild all those files that depend on it, which is, all of signaling and a couple unified files under gfx/layers.

Comment 1

[Mike Hommey [:glandium]]

We unfortunately currently have no way to express "export file that is a configure subst file" in moz.build, and using EXPORTS (obviously) fails because mozilla-config.h doesn't exist in the source directory.

How about a GENERATED_EXPORTS variable?

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> How about a GENERATED_EXPORTS variable?

Mmmmm that would have different semantics from the other GENERATED_ variables, since it would expect the generated files to be there after configure and before make export...

Arguably, we could remove those extra #include "mozilla-config.h", but chances are some would be reintroduced later, especially when you see that in all likeliness, one of them was added by using IWYU.

Comment 3

[Michael Shal [:mshal]]

(In reply to Mike Hommey [:glandium] from comment #2)
> Arguably, we could remove those extra #include "mozilla-config.h", but
> chances are some would be reintroduced later, especially when you see that
> in all likeliness, one of them was added by using IWYU.

Why do we need to export mozilla-config.h in the first place? If we didn't do that, then those files that manually include it would fail to compile, so we wouldn't have to worry about future IWYU runs or other accidental inclusions.

Comment 4

[Mike Hommey [:glandium]]

mozilla-config.h is required in the sdk.

Comment 5

[Gregory Szorc [:gps]]

Do we have a tracking bug of issues that could be fixed through static analysis? Ugh.

I think GENERATED_EXPORTS would solve this nicely. Alternatively, we could also store an attribute on each entry in EXPORTS to denote it is generated. But that may require a nasty patch set (see bug 853594).

Comment 7

[Mike Hommey [:glandium]]

So, turns out this is an issue that's triggered by the changes in bug 950298. The bug was existing, but it didn't show up on incremental builds, but it does now. The problem is that when nsinstall is not available, we're using nsinstall.py, but we're not using the same flags as if we were using nsinstall, which causes us to copy files instead of creating symlinks, or not keep timestamps.

Comment 8

[Mike Hommey [:glandium]]

Attachment: Keep timestamps when copying files with nsinstall.py while nsinstall is not ready to be used yet

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8369876 [details] [diff] [review]
Keep timestamps when copying files with nsinstall.py while nsinstall is not ready to be used yet

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

What could possibly go wrong?

Comment 10

[Mike Hommey [:glandium]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3fe1eed6529

Comment 11

[[e-mail]]

(In reply to Michael Shal from comment #3)
> (In reply to Mike Hommey from comment #2)
> > Arguably, we could remove those extra #include "mozilla-config.h", but
> > chances are some would be reintroduced later, especially when you see that
> > in all likeliness, one of them was added by using IWYU.
> 
> Why do we need to export mozilla-config.h in the first place? If we didn't
> do that, then those files that manually include it would fail to compile, so
> we wouldn't have to worry about future IWYU runs or other accidental
> inclusions.

Wouldn't removing its include guards cause accidental manual re-inclusion to fail spectacularly?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f3fe1eed6529

Comment 13

[Michael Shal [:mshal]]

(In reply to [e-mail] from comment #11)
> Wouldn't removing its include guards cause accidental manual re-inclusion to
> fail spectacularly?

It sounded like a good idea to me, but it doesn't seem to actually fail. It looks like gcc & clang only warn about macro re-definitions if a macro is #defined to a new value, not if there are two #defines with the same value. Even if it did, we would need -Werror enabled in the directory for the build to stop. Or was there some other way you were expecting it to break?

Comment 14

[Trevor Saunders (:tbsaunde)]

(In reply to Michael Shal [:mshal] from comment #13)
> (In reply to [e-mail] from comment #11)
> > Wouldn't removing its include guards cause accidental manual re-inclusion to
> > fail spectacularly?
> 
> It sounded like a good idea to me, but it doesn't seem to actually fail. It
> looks like gcc & clang only warn about macro re-definitions if a macro is
> #defined to a new value, not if there are two #defines with the same value.
> Even if it did, we would need -Werror enabled in the directory for the build
> to stop. Or was there some other way you were expecting it to break?

there's an easy enough way to make it break

#ifdef MOZILLA_CONFIG_H
#error "should only be included once!"
#endif
#define MOZILLA_CONFIG_H 1

Comment 15

[Michael Shal [:mshal]]

(In reply to Trevor Saunders (:tbsaunde) from comment #14)
> there's an easy enough way to make it break
> 
> #ifdef MOZILLA_CONFIG_H
> #error "should only be included once!"
> #endif
> #define MOZILLA_CONFIG_H 1

Oh yeah, makes sense :). Since this bug has been fixed in a different way, is there any benefit to doing this aside from removing superfluous #includes?