Build part of toolkit/crashreporter in unified mode

RESOLVED FIXED in mozilla29

Status

()

Toolkit
Crash Reporting
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: bjacob, Assigned: Away for a while)

Tracking

unspecified
mozilla29
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 8335040 [details] [diff] [review]
Build part of toolkit/crashreporter in unified mode

This is just a conservative switch from SOURCES to UNIFIED_SOURCES in each dir below crashreporter/. One could get much higher benefits by unifying the myriad of tiny subdirs there, but I wanted to do only simple clean things for now.

Also, I didn't know what to do with HOST_SOURCES so I left them unchanged. Lots more left to gain there.

On my Thinkpad W520:

before:

real    0m13.642s
user    0m43.775s
sys     0m4.040s

after:

real    0m15.516s
user    0m25.714s
sys     0m2.424s

Since we generally agree that 'user' is the most useful metric there because it measures best how a directory slows down the overall build, here we're talking about saving 18 sec of user time.
Attachment #8335040 - Flags: review?(ehsan)
(Reporter)

Comment 1

4 years ago
Comment on attachment 8335040 [details] [diff] [review]
Build part of toolkit/crashreporter in unified mode

Still many failures on try, cancelling review for now.
Attachment #8335040 - Flags: review?(ehsan)
(Reporter)

Comment 2

4 years ago
The failures:
   https://tbpl.mozilla.org/?tree=Try&rev=ccbf9cc54773
not sure if the modest gains here are worth so much effort. I might be giving up. Closing for now so I don't prevent other people from jumping on this.
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → WONTFIX
(Assignee)

Comment 3

4 years ago
Not sure why you wontfixed this?
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(Reporter)

Comment 4

4 years ago
Because the cost/benefit ratio went too low with different build issues remaining on multiple platforms, see the try push in comment 2.
(Assignee)

Comment 5

4 years ago
(In reply to comment #4)
> Because the cost/benefit ratio went too low with different build issues
> remaining on multiple platforms, see the try push in comment 2.

But do you oppose somebody else coming up with a patch which builds here?
(Reporter)

Comment 6

4 years ago
No objection at all! I just didn't want to give the wrong impression that I was working on it.
(Assignee)

Updated

4 years ago
Assignee: nobody → ehsan
(Assignee)

Updated

4 years ago
Attachment #8335040 - Attachment is obsolete: true
(Assignee)

Comment 7

4 years ago
Created attachment 8339677 [details] [diff] [review]
Build crashreporter in unified mode; r=ted
(Assignee)

Updated

4 years ago
Attachment #8339677 - Flags: review?(ted)
(Assignee)

Comment 11

4 years ago
Created attachment 8340021 [details] [diff] [review]
Patch (v2)

This should build on all platforms:

https://tbpl.mozilla.org/?tree=Try&rev=c75453ab9e92
Attachment #8339677 - Attachment is obsolete: true
Attachment #8339677 - Flags: review?(ted)
Attachment #8340021 - Flags: review?(ted)
Comment on attachment 8340021 [details] [diff] [review]
Patch (v2)

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

This is okay except for that Makefile.in change. What's up with that?

::: toolkit/crashreporter/InjectCrashReporter.h
@@ +3,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +#ifndef InjectCrashReporter_h
> +#define InjectCrashReporter_h

Huh, wonder how we missed that?

::: toolkit/crashreporter/client/crashreporter_win.cpp
@@ +36,5 @@
>  #define WM_UPLOADCOMPLETE WM_APP
>  
> +// Thanks, Windows.h :(
> +#undef min
> +#undef max

There's a NOMINMAX you can define to make Windows.h not define these.

::: toolkit/crashreporter/google-breakpad/src/common/Makefile.in
@@ +29,5 @@
>  ifneq (WINNT,$(OS_TARGET))
>  # Headers from this directory are included as "common/header.h". Having
>  # -I$(srcdir) on the command line makes us use common/memory.h when
>  # <memory.h> is included from system headers, which is not intended.
> +INCLUDES = -I$(srcdir) $(LOCAL_INCLUDES) -I$(DIST)/include

So uh, this now directly contradicts the comment above.
Attachment #8340021 - Flags: review?(ted) → review-
(Assignee)

Comment 13

4 years ago
Created attachment 8343157 [details] [diff] [review]
Build crashreporter in unified mode; r=ted
(Assignee)

Updated

4 years ago
Attachment #8343157 - Flags: review?(ted)
(Assignee)

Updated

4 years ago
Attachment #8340021 - Attachment is obsolete: true
(Assignee)

Comment 15

4 years ago
Ted: ping?
Comment on attachment 8343157 [details] [diff] [review]
Build crashreporter in unified mode; r=ted

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

Sorry for the review lag, thanks for cleaning that up!
Attachment #8343157 - Flags: review?(ted) → review+
https://hg.mozilla.org/mozilla-central/rev/94c2cc056794
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29

Updated

4 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.