Closed Bug 787658 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(firefox17 fixed, firefox18 fixed)

RESOLVED FIXED
mozilla18
Tracking Status
firefox17 --- fixed
firefox18 --- fixed

People

(Reporter: rain1, Assigned: rain1)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
(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...
export-subdirs: PARALLEL_DIRS_targets
        $(foreach dir,$(DIRS),$(call SUBMAKE,export,$(dir)))

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

would work.
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.
Attached patch Reduced testcase (obsolete) — Splinter Review
I've also gotten rid of realmtime since it seemed pointless.
Assignee: nobody → sagarwal
Attachment #657614 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #657620 - Flags: review?(gps)
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.
FWIW, you can identify these failed builds by looking for this string:  "TestDictionary.webidl' failed, return code 1".
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.
Attachment #657620 - Flags: review?(ted.mielczarek)
Attachment #657620 - Flags: review?(khuey)
Attachment #657620 - Flags: review?(benjamin)
Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

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

fun
Attachment #657620 - Flags: review?(khuey) → review+
https://hg.mozilla.org/users/bsmedberg_mozilla.com/pymake/rev/d0ee78addf4e
https://hg.mozilla.org/mozilla-central/rev/ce16c39e01fb
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attachment #657620 - Flags: review?(ted.mielczarek)
Attachment #657620 - Flags: review?(gps)
Attachment #657620 - Flags: review?(benjamin)
Thanks Ryan!
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.
Attachment #657620 - Flags: approval-mozilla-aurora?
Blocks: 799095
Attachment #657620 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
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.
Attachment #657620 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.