893798 - Bug 861587 caused lots of build log spam

Status: RESOLVED FIXED

(Firefox Build System :: General, defect)

Description

[Justin Wood (:Callek)]

Attachment: [m-c] fix the echo-to-build-log

As directed in https://bugzilla.mozilla.org/show_bug.cgi?id=861587#c57 we caused a LOT of log spam here...

This patch fixes it.

Whoever-gets-to-review-first. :bz because he recently copped to fast review turnaround in newsgroups! :joey because he's on my team and I can nag him if need be :-)

Comment 1

[Joey Armstrong [:joey]]

Comment on attachment 775648 [details] [diff] [review]
[m-c] fix the echo-to-build-log

Having to explicitly touch timestamp files like this is usually a good sign that build dependencies are broken.  Best to spend time tracking down why deps are bad rather than forcing them to become stale at arbitrary times.

if binding_dependency_trackers is touched after header_files or cpp_files a up to date { but not both } deps will be forced stale work to be performed again during a nop build.

At a minimum deps should be reversed:

.BindingGen: header_files cpp_files

so .BindingGen will be updated *after* both cpp and header files were regenerated.

Comment 2

[Ed Morley [:emorley]]

(In reply to Joey Armstrong [:joey] from comment #1)
> Comment on attachment 775648 [details] [diff] [review]
> [m-c] fix the echo-to-build-log
> 
> Having to explicitly touch timestamp files like this is usually a good sign
> that build dependencies are broken.  Best to spend time tracking down why
> deps are bad rather than forcing them to become stale at arbitrary times.
> 
> if binding_dependency_trackers is touched after header_files or cpp_files a
> up to date { but not both } deps will be forced stale work to be performed
> again during a nop build.
> 
> At a minimum deps should be reversed:
> 
> .BindingGen: header_files cpp_files
> 
> so .BindingGen will be updated *after* both cpp and header files were
> regenerated.

Potentially valid points, but not applicable to the bug summary. Please file another bug :-)

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 775648 [details] [diff] [review]
[m-c] fix the echo-to-build-log

Please move that comment _after_ the "pick up" comment and phrase it like so:

  # The rule just brings the tracker up to date, if it's ....

r=me with that

Comment 4

[Boris Zbarsky [:bzbarsky]]

Joey, I'd love to hear more details on how the setup here can be improved.  It's definitely suboptimal in various ways.... just tricky to get right as you improve it.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Also, was there a reason to not fix the ParserResults.pkl and .BindingGen rules while you were here?

Comment 6

[Joey Armstrong [:joey]]

(In reply to Boris Zbarsky (:bz) from comment #5)
> Also, was there a reason to not fix the ParserResults.pkl and .BindingGen
> rules while you were here?

I've not read through all the comments & logic yet but I would concur with mshal that adding a single dependency like this may contribute to unexpected rebuilding.  Esp if there are problem deps, if anything is wrong we rebuild.

The .BindingGen dep visible in Callek's patch will likely be a dep problem because header and cpp are able to update the dep asynchronously.  That allows PP regeneration .


Oh I just noticed something, this will definitely be a problem:

$(globalgen_targets): ParserResults.pkl

   [ ... ]

ParserResults.pkl: $(globalgen_dependencies)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Note that comment 5 was about this bug: comments in the actual rebuild rules.

If you see other problems, please file a separate bug with a clear problem description, ok?

Comment 8

[Joey Armstrong [:joey]]

(In reply to Boris Zbarsky (:bz) from comment #7)
> Note that comment 5 was about this bug: comments in the actual rebuild rules.
> 
> If you see other problems, please file a separate bug with a clear problem
> description, ok?

Bugs opened for the few problems noticed.

Comment 9

[Justin Wood (:Callek)]

Attachment: [m-c] v2

I had no special reason for not doing the other rules, other than I missed them.

I also made the spaces (for line-continued commands) into tabs, primarily to align better in local editors if local tab-spacing is different than authors.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 775730 [details] [diff] [review]
[m-c] v2

>+# the rule just brings the tracker up to date, if it's out of date, so that

"The rule:

>+#Running GlobalGen.py updates ParserResults.pkl as a side-effect

Space after '#', please.

>+# Then, Pass our long lists through files to try to avoid blowing out the

"pass"

r=me

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Bugs opened for the few problems noticed.

For the record, that would be bug 893875 and bug 893864.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[Ed Morley [:emorley]]

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