Closed Bug 698005 Opened 9 years ago Closed 8 years ago

Followup to build system changes in bug 683229 (Simple Profiler System)

Categories

(Firefox Build System :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED WORKSFORME

People

(Reporter: emorley, Assigned: BenWa)

References

Details

Attachments

(1 file)

Bug 683229 added the Simple Profiler System along with a few build changes, which after asking on #pymake, were thought best to be tweaked in a followup. Breaking out from the original bug to ease merging.

* The MOZ_ENABLE_PROFILER_SPS configure option isn't used and can be removed from configure + config/autoconf.mk.in + the define in tools/profiler/Makefile.in
* The way the Android-only parts are disabled is inconsistent with how it's normally done elsewhere

Thanks :-)
Assignee: nobody → bgirard
Attached patch patchSplinter Review
Comment on attachment 570284 [details] [diff] [review]
patch

Just removing something I checked in recently.

Doing a try build since I tend to goof build changes.
Attachment #570284 - Flags: review?(khuey)
Try run for 44407d43abaf is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=44407d43abaf
Results (out of 121 total builds):
    success: 117
    warnings: 4
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/b56girard@gmail.com-44407d43abaf
There was one other thing I didn't like about your original patch. You build in the tools/profiler directory unconditionally, but only compile the source there for Android. That seems really weird. Do you use the header files unconditionally? I would prefer that you either:
a) Skip compiling the directory entirely on platforms where it's not going to be used
or
b) Compile it on all platforms, disable at runtime or by #define on platforms where it's not going to be used.

Also, disabling it by checking the widget toolkit in a Makefile is not fantastic. I'd prefer if you kept the makefile variable, and did a configure-time check to set it, something like:
if test "$MOZ_WIDGET_TOOLKIT" = "android"; then
  MOZ_SPS = 1
fi
(In reply to Ted Mielczarek [:ted, :luser] from comment #5)

Thanks for the feedback, I tried doing the best with my understanding of the build system.

> There was one other thing I didn't like about your original patch. You build
> in the tools/profiler directory unconditionally, but only compile the source
> there for Android. That seems really weird. Do you use the header files
> unconditionally? I would prefer that you either:
> a) Skip compiling the directory entirely on platforms where it's not going
> to be used

This was my original target however the header file must be found in all platforms. Where the profiler is disabled the macros are defined to empty. If I skipped the directory then those OS would fail to pick up the header files. Can I just move the header files?

> or
> b) Compile it on all platforms, disable at runtime or by #define on
> platforms where it's not going to be used.
> 
> Also, disabling it by checking the widget toolkit in a Makefile is not
> fantastic. I'd prefer if you kept the makefile variable, and did a
> configure-time check to set it, something like:
> if test "$MOZ_WIDGET_TOOLKIT" = "android"; then
>   MOZ_SPS = 1
> fi

If we can't do a) then I will do b).
I've just noticed as well, tools/profiler/sps/Makefile appears to be NPOTB, is this intentional?
(In reply to Ed Morley [:edmorley] from comment #7)
> I've just noticed as well, tools/profiler/sps/Makefile appears to be NPOTB,
> is this intentional?

Jeff is porting SPS to Mac and will be fixing this.
Blocks: 713227
I think this has been fixed by the ports to other platform:
1) The android check is gone
2) We build SPS on all tier 1 platforms but keep the define to disable the profiler if required.
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → WORKSFORME
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.