WebIDL headers changes not correctly picked up by the build system somehow

RESOLVED DUPLICATE of bug 928195

Status

defect
RESOLVED DUPLICATE of bug 928195
6 years ago
Last year

People

(Reporter: bzbarsky, Assigned: glandium)

Tracking

unspecified
mozilla27
x86
Windows 7
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

I just ran into this on inbound, on Windows only.  Steps to reproduce:

1)  Pull https://hg.mozilla.org/integration/mozilla-inbound/rev/17411d6192e4
2)  Build.
3)  Apply https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/8fa0cbe4a54d
4)  Apply https://hg.mozilla.org/integration/mozilla-inbound/raw-rev/ccedb8b1cefc
5)  Build.

EXPECTED RESULTS: Builds

ACTUAL RESULTS:
:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\dom\bindings\DOMTransactionEventBinding.cpp(19) : error C2084: function 'mozilla::dom::DOMTransactionEventInit::DOMTransactionEventInit(void)' already has a body
e:\builds\moz2_slave\m-in-w32-000000000000000000000\build\obj-firefox\dom\bindings\DOMTransactionEventBinding.cpp(372) : error C2039: 'FastDOMTransactionEventInit' : is not a member of 'mozilla::dom::detail'

and tons of other errors.

That first error says that the .cpp being used is from after the changes but the .h is from _before_ the changes, since one of the changes was to uninline the constructor.  I have no idea how that would happen, since the bindings aren't even including the exported version of the headers but the local version...  and it only happens on Windows.
I can reproduce, and the problem is scary. What happens, essentially is that the install-manifests rule is running after the build. Which makes no sense. And interestingly, which doesn't happen when not building through mach.
(In reply to Mike Hommey [:glandium] from comment #1)
> which doesn't happen when not building through mach.

well, in my VM. tbpl builds are not using mach and they hit it, so...
Looks like python native commands scheduling is completely off: while the commands for the install manifests are shown at the beginning of the build, they're actually not run until the very end. Removing the use of python native commands for install manifests fixed it for me. That's kind of scary for every other use of python native commands.
This might be a red herring. It looks like I've been fooled by pymake's output not being consistent at all, and only displaying the native python commands output way after they are finished.
(In reply to Boris Zbarsky [:bz] (Vacation Oct 12 - Oct 27) from comment #0)
> That first error says that the .cpp being used is from after the changes but
> the .h is from _before_ the changes, since one of the changes was to
> uninline the constructor.  I have no idea how that would happen, since the
> bindings aren't even including the exported version of the headers but the
> local version...  and it only happens on Windows.

So it turns out your assertion is wrong.

GeneratedEventClasses.h, which is included by many files (I guess all the .cpp files generated from webidls containing 'HeaderFile="GeneratedEventClasses.h"'), includes mozilla/dom/many_headers.h, which are included from dist/include, obviously.

And since the dependencies in .deps are on the full paths, and the INSTALL_TARGETS are on ../../dist/include, make doesn't know it needs to update them.
(In reply to Mike Hommey [:glandium] from comment #5)
> And since the dependencies in .deps are on the full paths, and the
> INSTALL_TARGETS are on ../../dist/include, make doesn't know it needs to
> update them.

This, in turn, is likely to be a reason for many other clobber needs.
Blocks: 890744, 923545
Posted patch Use an absolute path for DIST (obsolete) — Splinter Review
All .pp files are using absolute paths, so we need the INSTALL_TARGETS destinations using DIST to have an absolute path, for more than webidl generated files.
Attachment #815269 - Flags: review?(gps)
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
(In reply to Mike Hommey [:glandium] from comment #7)
> Created attachment 815269 [details] [diff] [review]
> Use an absolute path for DIST
> 
> All .pp files are using absolute paths, so we need the INSTALL_TARGETS
> destinations using DIST to have an absolute path, for more than webidl
> generated files.

https://tbpl.mozilla.org/?tree=Try&rev=20a3a8565f01
> GeneratedEventClasses.h

Ah, good catch!  And very good catch on the deps/install stuff.  Thank you!
Comment on attachment 815269 [details] [diff] [review]
Use an absolute path for DIST

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

The number of dependency bugs related to path normalization is somewhat frightening. Good catch on this.
Attachment #815269 - Flags: review?(gps) → review+
Duplicate of this bug: 923978
(In reply to Mike Hommey [:glandium] from comment #4)
> This might be a red herring. It looks like I've been fooled by pymake's
> output not being consistent at all, and only displaying the native python
> commands output way after they are finished.

pymake's output is kind of whack. It will schedule each command in a recipe as a separate "task." If you have a rule with 100 targets and a recipe with 2 commands, it appears to schedule the first command for *all* the targets then will schedule the second command. In practice, we have a number of recipes where the first command is "echo $@." This means pymake will misleadingly print out all the targets before it actively starts processing the meat of the command. Extremely misleading.

GNU make, by contrast, seems to schedule each recipe as one unit or at least iterates through all commands in a recipe before moving on to the next target.
Duplicate of this bug: 923545
https://hg.mozilla.org/mozilla-central/rev/cff5a22acb7c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Backed out on suspicion of causing failures like:
https://tbpl.mozilla.org/php/getParsedLog.php?id=28990634&tree=Mozilla-Central

remote:   https://hg.mozilla.org/mozilla-central/rev/500765bd8dd9
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 872934
Since doing this broadly failed, let's do this in a more limited manner. It actually should be enough, since the problem that needs fixing here is generated headers being installed in dist/include before compiling sources in the same directory they are being generated from. IPDL files are not handled with INSTALL_TARGETS-like dependencies (and are not included from dist/include), and I don't think we have other such pattern in the tree.
Attachment #817801 - Flags: review?(gps)
Attachment #815269 - Attachment is obsolete: true
Depends on: 926906
Attachment #817801 - Flags: review?(gps) → review+
https://hg.mozilla.org/mozilla-central/rev/bbc0f0fd8e88
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
This appears not to be fully fixed, since well after 
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=dc2b71e57211
landed (which added readonly attribute weakReferent, and built clean on all platforms, as did the next two pushes), ehsan got this failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29366148&tree=Mozilla-Inbound
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 928195
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.