787658 - Some Pymake builds error out in dom/bindings, seemingly caused by output corruption caused by a build race

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Rain [:rain1] (inactive)]

e.g. https://tbpl.mozilla.org/php/getParsedLog.php?id=14886515&tree=Mozilla-Inbound&full=1

It seems like ParserResults.pkl's being built multiple times via dom/bindings/test. However it shouldn't be built at all in there, since it should already be up-to-date per after building in dom/bindings.

Comment 1

[Mike Hommey [:glandium]]

Indeed, the easy fix is to remove these rules that build stuff in .. altogether. Unfortunately, the normal behaviour is for subdirectories to be built before their parent, so by the time we enter dom/bindings/test, dom/bindings is *not* built. And considering subdirectories being built before their parent is an expected behaviour in a lot of other directories...

Something that would work is to do:
stuff: $(binding_header_files)
export::
        $(MAKE) stuff

and place that before the rules.mk include in dom/bindings/Makefile.in

Unfortunately, this would break when ted removes double-colon rules. I don't think their replacement would allow this, btw.

Comment 2

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #1)
> Indeed, the easy fix is to remove these rules that build stuff in ..
> altogether. Unfortunately, the normal behaviour is for subdirectories to be
> built before their parent, so by the time we enter dom/bindings/test,
> dom/bindings is *not* built. And considering subdirectories being built
> before their parent is an expected behaviour in a lot of other directories...
> 
> Something that would work is to do:
> stuff: $(binding_header_files)
> export::
>         $(MAKE) stuff
> 
> and place that before the rules.mk include in dom/bindings/Makefile.in

gah, no, that wouldn't work either, because of the PARALLEL_DIRS rules...

Comment 3

[Mike Hommey [:glandium]]

export-subdirs: PARALLEL_DIRS_targets
        $(foreach dir,$(DIRS),$(call SUBMAKE,export,$(dir)))

export:
        $(MAKE) stuff
        $(MAKE) export-subdirs

would work.

Comment 4

[Rain [:rain1] (inactive)]

So the fact that ParserResults.pkl's being remade all the time is indeed a bug in Pymake.

When a target is first considered and make() is called on it, it resolves (gets the actual path to) its dependencies: http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/file/tip/pymake/data.py#l1259. resolvevpath (called by resolvedeps) gets and stores the mtime on the Target object.

So when the target for ParserResults.pkl is considered, in resolvedeps it considers its dependency CanvasRenderingContext2D.webidl. This target has mtime = mtime of its dependency, so it is remade. Now when a target is remade its mtime is reset: http://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/file/tip/pymake/data.py#l1206

At this point there's nothing that actually recalculates the mtime of CanvasRenderingContext2D.webidl. Thus its mtime remains none and Pymake thinks ParserResults.pkl needs to be remade.

Comment 5

[Rain [:rain1] (inactive)]

Attachment: Reduced testcase

Comment 6

[Rain [:rain1] (inactive)]

Attachment: Recalculate mtime once the target is built

I've also gotten rid of realmtime since it seemed pointless.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14903660&tree=Firefox

Comment 8

[Rain [:rain1] (inactive)]

This patch works fine on try.

I believe the library (-l) stuff in resolvevpath isn't relevant here because it
can never be a target, only a prerequisite.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14908642&tree=Mozilla-Inbound

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14910361&tree=Mozilla-Inbound

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14912741&tree=Firefox

Comment 12

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14918103&tree=Mozilla-Inbound

Comment 13

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14919553&tree=Mozilla-Inbound

Comment 14

[Nicholas Nethercote [inactive]]

FWIW, you can identify these failed builds by looking for this string:  "TestDictionary.webidl' failed, return code 1".

Comment 15

[Ed Morley [:emorley]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14948393&tree=Firefox

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14955035&tree=Mozilla-Inbound

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14955906&tree=Mozilla-Inbound

Comment 18

[Rain [:rain1] (inactive)]

Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

Sorry for the spam. gps said he's not feeling well. I was hoping someone else could get to it, because quite a few windows builds are failing this way.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14957503&tree=Mozilla-Inbound

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://tbpl.mozilla.org/php/getParsedLog.php?id=14957976&tree=Mozilla-Inbound

Comment 21

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

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

fun

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/d0ee78addf4e
https://hg.mozilla.org/mozilla-central/rev/ce16c39e01fb

Comment 23

[Rain [:rain1] (inactive)]

Thanks Ryan!

Comment 24

[Rain [:rain1] (inactive)]

Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

[Approval Request Comment]
Bug caused by (feature/regressing bug #): not a regression
User impact if declined: we'll still need to keep using GNU make for a year from now
Testing completed (on m-c, etc.): Yes, been in production for several weeks
Risk to taking this patch (and alternatives if risky): This patch is somewhat non-trivial, but it's been used on m-c for several weeks and hasn't had any problems. The patch also includes a test. I'd consider it low but not zero risk. The alternative is to keep using GNU make.
String or UUID changes made by this patch: None.

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

[Triage Comment]
Very low risk, Callek let us know that if anything seems fishy we can easily back this out.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e630a6baafe1