Bug 141027

Summary: [EFL][GTK] Fix the build after r179326
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Csaba Osztrogonác <ossy>
Status: RESOLVED FIXED    
Severity: Blocker CC: aestes, andersca, ap, bdakin, beidson, benjamin, cgarcia, clopez, cmarcelo, commit-queue, darin, enrica, gyuyoung.kim, hyatt, kling, mitz, mjs, mrowe, ossy, sam, simon.fraser, thorton
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 141033    
Bug Blocks: 140673    
Attachments:
Description Flags
Patch
mitz: review-
patch for EWS
none
Patch none

Description Csaba Osztrogonác 2015-01-29 00:11:37 PST
SSIA

I started working on it.
Comment 1 Csaba Osztrogonác 2015-01-29 00:32:56 PST
I won't be available in the following 2.5-3 hours. If anybody is 
interested in fixing the build, feel free to pick it up.
Comment 2 Carlos Alberto Lopez Perez 2015-01-29 02:59:30 PST
Regarding the availability of DTrace on Linux check: https://bugs.webkit.org/show_bug.cgi?id=140673#c18
Comment 3 Carlos Garcia Campos 2015-01-29 03:07:12 PST
(In reply to comment #2)
> Regarding the availability of DTrace on Linux check:
> https://bugs.webkit.org/show_bug.cgi?id=140673#c18

I think this should be guarded by #ifdefs anyway, and make dtrace and uuid as optional deps if we eventually support it.
Comment 4 Carlos Garcia Campos 2015-01-29 03:59:04 PST
Created attachment 245616 [details]
Patch

Add ENABLE(DTRACE)
Comment 5 Csaba Osztrogonác 2015-01-29 04:10:57 PST
note: I reverted my previous change, removed uuid from the 
dependency list: http://trac.webkit.org/changeset/179341
Comment 6 mitz 2015-01-29 07:11:30 PST
Comment on attachment 245616 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245616&action=review

> Source/WebKit2/ChangeLog:8
> +        Add ENABLE_DTRACE only enabled in Cocoa platforms to guard the

JavaScriptCore uses HAVE(DTRACE). I think that’s also more appropriate here, since it’s not about enabling a WebKit feature, but about whether a platform feature is available.
Comment 7 Csaba Osztrogonác 2015-01-29 07:16:20 PST
(In reply to comment #6)
> Comment on attachment 245616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245616&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Add ENABLE_DTRACE only enabled in Cocoa platforms to guard the
> 
> JavaScriptCore uses HAVE(DTRACE). I think that’s also more appropriate here,
> since it’s not about enabling a WebKit feature, but about whether a platform
> feature is available.

It is a Mac only hack in JavasScriptCore:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/Configurations/Base.xcconfig

Any hint how to make it cross platform and work inside WebKit2 too?
Comment 8 Csaba Osztrogonác 2015-01-29 07:23:35 PST
https://trac.webkit.org/changeset/41350 was a dirty Mac only hack, which
moved it from Platform.h to xcconfig.

But now HAVE_DTRACE is true unconditonally since:
https://trac.webkit.org/changeset/132860/trunk/Source/JavaScriptCore/Configurations/Base.xcconfig

I think, it's high time to move it back to the end of Platform.h.
Comment 9 Csaba Osztrogonác 2015-01-29 07:39:26 PST
(In reply to comment #8)
> https://trac.webkit.org/changeset/41350 was a dirty Mac only hack, which
> moved it from Platform.h to xcconfig.
> 
> But now HAVE_DTRACE is true unconditonally since:
> https://trac.webkit.org/changeset/132860/trunk/Source/JavaScriptCore/
> Configurations/Base.xcconfig
> 
> I think, it's high time to move it back to the end of Platform.h.

new bug report for moving this back to Platform.h: bug141033
Comment 10 Carlos Garcia Campos 2015-01-29 07:44:26 PST
(In reply to comment #6)
> Comment on attachment 245616 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=245616&action=review
> 
> > Source/WebKit2/ChangeLog:8
> > +        Add ENABLE_DTRACE only enabled in Cocoa platforms to guard the
> 
> JavaScriptCore uses HAVE(DTRACE). I think that’s also more appropriate here,
> since it’s not about enabling a WebKit feature, but about whether a platform
> feature is available.

I used ENABLE with the idea of being able to disable it for production builds
Comment 11 Csaba Osztrogonác 2015-01-29 08:27:30 PST
Created attachment 245623 [details]
patch for EWS

patch for EWS, not for landing, s/ENABLE(DTRACE)/HAVE(DTRACE)/g in the patch of Carlos + fix from bug141033
Comment 12 Csaba Osztrogonác 2015-01-29 09:08:37 PST
Created attachment 245626 [details]
Patch

bug141033 already fixed, patch for final review
Comment 13 Csaba Osztrogonác 2015-01-29 09:11:13 PST
ping for review?

EFL and GTK build is still broken - since 15 hours!
Comment 14 Alexey Proskuryakov 2015-01-29 09:37:40 PST
Comment on attachment 245626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245626&action=review

> Source/WebKit2/ChangeLog:9
> +        Guard MessageRecorder implementation with USE(DTRACE)
> +        for platforms not supporting DTrace.

This patch breaks dtrace support on Mac, because only JavaScriptCore project turns on ENABLE(DTRACE).
Comment 15 Alexey Proskuryakov 2015-01-29 09:38:45 PST
Comment on attachment 245626 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=245626&action=review

>> Source/WebKit2/ChangeLog:9
>> +        for platforms not supporting DTrace.
> 
> This patch breaks dtrace support on Mac, because only JavaScriptCore project turns on ENABLE(DTRACE).

I see, this is only part of the fix. Seems OK to me.
Comment 16 WebKit Commit Bot 2015-01-29 10:23:44 PST
Comment on attachment 245626 [details]
Patch

Clearing flags on attachment: 245626

Committed r179346: <http://trac.webkit.org/changeset/179346>
Comment 17 WebKit Commit Bot 2015-01-29 10:23:51 PST
All reviewed patches have been landed.  Closing bug.