Boris Zbarsky [:bzbarsky] Reporter
	Description • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 1 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 2 • 11 years ago

(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...

	Mike Hommey [:glandium] Assignee
	Comment 3 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 4 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 5 • 11 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 6 • 11 years ago

(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.

	Mike Hommey [:glandium] Assignee
	Comment 7 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 8 • 11 years ago

(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

	Boris Zbarsky [:bzbarsky] Reporter
	Comment 9 • 11 years ago

> GeneratedEventClasses.h

Ah, good catch!  And very good catch on the deps/install stuff.  Thank you!

	Gregory Szorc [:gps]
	Comment 10 • 11 years ago

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.

	Gregory Szorc [:gps]
	Comment 11 • 11 years ago

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

	Gregory Szorc [:gps]
	Comment 13 • 11 years ago

(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.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 15 • 11 years ago

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

	Ed Morley [:emorley]
	Comment 16 • 11 years ago

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

	Mike Hommey [:glandium] Assignee
	Comment 17 • 11 years ago

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.

	Mike Hommey [:glandium] Assignee
	Comment 18 • 11 years ago

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

	Carsten Book [:Tomcat]
	Comment 19 • 11 years ago

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

	Randell Jesup [:jesup] (needinfo me)
	Comment 20 • 11 years ago

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