Last Comment Bug 787658 - Some Pymake builds error out in dom/bindings, seemingly caused by output corruption caused by a build race
: Some Pymake builds error out in dom/bindings, seemingly caused by output corr...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Build Config (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla18
Assigned To: Siddharth Agarwal [:sid0] (inactive)
:
Mentors:
Depends on:
Blocks: 593585 787498 799095
  Show dependency treegraph
 
Reported: 2012-09-01 05:27 PDT by Siddharth Agarwal [:sid0] (inactive)
Modified: 2012-10-22 20:54 PDT (History)
10 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed


Attachments
Reduced testcase (445 bytes, patch)
2012-09-01 16:32 PDT, Siddharth Agarwal [:sid0] (inactive)
no flags Details | Diff | Splinter Review
Recalculate mtime once the target is built (3.98 KB, patch)
2012-09-01 18:43 PDT, Siddharth Agarwal [:sid0] (inactive)
khuey: review+
akeybl: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Siddharth Agarwal [:sid0] (inactive) 2012-09-01 05:27:42 PDT
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] 2012-09-01 05:52:16 PDT
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] 2012-09-01 05:53:05 PDT
(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] 2012-09-01 05:56:01 PDT
export-subdirs: PARALLEL_DIRS_targets
        $(foreach dir,$(DIRS),$(call SUBMAKE,export,$(dir)))

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

would work.
Comment 4 Siddharth Agarwal [:sid0] (inactive) 2012-09-01 16:04:51 PDT
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 Siddharth Agarwal [:sid0] (inactive) 2012-09-01 16:32:05 PDT
Created attachment 657614 [details] [diff] [review]
Reduced testcase
Comment 6 Siddharth Agarwal [:sid0] (inactive) 2012-09-01 18:43:27 PDT
Created attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

I've also gotten rid of realmtime since it seemed pointless.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-09-01 19:18:18 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=14903660&tree=Firefox
Comment 8 Siddharth Agarwal [:sid0] (inactive) 2012-09-02 03:00:58 PDT
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 11 Ryan VanderMeulen [:RyanVM] 2012-09-02 19:07:14 PDT
https://tbpl.mozilla.org/php/getParsedLog.php?id=14912741&tree=Firefox
Comment 14 Nicholas Nethercote [:njn] 2012-09-03 16:27:44 PDT
FWIW, you can identify these failed builds by looking for this string:  "TestDictionary.webidl' failed, return code 1".
Comment 18 Siddharth Agarwal [:sid0] (inactive) 2012-09-04 16:21:16 PDT
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 21 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-09-04 18:13:52 PDT
Comment on attachment 657620 [details] [diff] [review]
Recalculate mtime once the target is built

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

fun
Comment 23 Siddharth Agarwal [:sid0] (inactive) 2012-09-05 02:20:24 PDT
Thanks Ryan!
Comment 24 Siddharth Agarwal [:sid0] (inactive) 2012-10-08 05:46:36 PDT
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] 2012-10-10 15:49:35 PDT
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] 2012-10-22 20:54:04 PDT
https://hg.mozilla.org/releases/mozilla-beta/rev/e630a6baafe1

Note You need to log in before you can comment on or make changes to this bug.