Closed Bug 893798 Opened 7 years ago Closed 7 years ago

Bug 861587 caused lots of build log spam

Categories

(Firefox Build System :: General, defect)

x86_64
Windows 7
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla25

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch [m-c] fix the echo-to-build-log (obsolete) — Splinter Review
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 :-)
Attachment #775648 - Flags: review?(joey)
Attachment #775648 - Flags: review?(bzbarsky)
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.
Attachment #775648 - Flags: review?(joey) → review-
(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 :-)
Attachment #775648 - Flags: review?(bzbarsky)
Attachment #775648 - Flags: review-
Attachment #775648 - Flags: review+
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
Attachment #775648 - Flags: review+
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.  ;)
Also, was there a reason to not fix the ParserResults.pkl and .BindingGen rules while you were here?
(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)
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?
(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.
Attached patch [m-c] v2Splinter Review
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.
Attachment #775648 - Attachment is obsolete: true
Attachment #775730 - Flags: review?(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
Attachment #775730 - Flags: review?(bzbarsky) → review+
> Bugs opened for the few problems noticed.

For the record, that would be bug 893875 and bug 893864.
https://hg.mozilla.org/mozilla-central/rev/b1b391784e53
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.