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

RESOLVED FIXED in mozilla30

Status

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Trunk
mozilla30
x86_64
Linux
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

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.
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?
(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.
(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.
mozilla-config.h is required in the sdk.
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).
Duplicate of this bug: 965268
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.
Blocks: 950298
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
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?
Attachment #8369876 - Flags: review?(gps) → review+
(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?
https://hg.mozilla.org/mozilla-central/rev/f3fe1eed6529
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
(In reply to neil@parkwaycc.co.uk 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?
(In reply to Michael Shal [:mshal] from comment #13)
> (In reply to neil@parkwaycc.co.uk 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
(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?
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.