Bug 92630 - Regression(r123983): CMake build broken when unit tests are enabled
Summary: Regression(r123983): CMake build broken when unit tests are enabled
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Thiago Marcos P. Santos
URL:
Keywords:
Depends on:
Blocks: 83579
  Show dependency treegraph
 
Reported: 2012-07-30 04:38 PDT by Thiago Marcos P. Santos
Modified: 2012-08-20 04:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (11.49 KB, patch)
2012-07-30 07:42 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (11.49 KB, patch)
2012-07-30 08:09 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (11.06 KB, patch)
2012-07-30 11:19 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff
Patch (10.89 KB, patch)
2012-07-30 11:41 PDT, Thiago Marcos P. Santos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Thiago Marcos P. Santos 2012-07-30 04:38:55 PDT
We need to update the forwarding headers generation for the API utests.
Comment 1 Thiago Marcos P. Santos 2012-07-30 07:42:43 PDT
Created attachment 155288 [details]
Patch
Comment 2 Thiago Marcos P. Santos 2012-07-30 07:51:41 PDT
I have some rants about this newly introduced macro:

- We are duplicating a functionality provided by generate-forwarding-headers.pl which is tested and used by all the ports.

- The logic implemented by the macro is to generate forwarding headers for all .h on the paths you pass as argument. The perl script detects which header needs to be forwarded. If someone #includes a new header that is not in a folder at the list, will break our build.

- The patch get rid of targets, which means that CMake will generate the forwarding headers every time it reads this file, no matter what you are building.
Comment 3 Thiago Marcos P. Santos 2012-07-30 08:09:45 PDT
Created attachment 155293 [details]
Patch
Comment 4 Chris Dumez 2012-07-30 08:15:01 PDT
Comment on attachment 155293 [details]
Patch

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

> Source/WebKit2/CMakeLists.txt:575
> +    ${WEBCORE_DIR}/dom

We also need ${WEBCORE_DIR}/dom/default
Comment 5 Chris Dumez 2012-07-30 08:16:10 PDT
Comment on attachment 155293 [details]
Patch

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

>> Source/WebKit2/CMakeLists.txt:575
>> +    ${WEBCORE_DIR}/dom
> 
> We also need ${WEBCORE_DIR}/dom/default

Well, technically not yet, so it's fine. I can add it later.
Comment 6 Patrick R. Gansterer 2012-07-30 09:52:58 PDT
Comment on attachment 155293 [details]
Patch

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

(In reply to comment #2)
> I have some rants about this newly introduced macro:
It's more or less with every build system, that we duplicate functionality. :-(
But the adding different targets and store the name in ForwardingHeadersForTestWebKitAPI_NAME is IMHO a much much biger hack. This doesn't scale very well and duplicates the functionality about different CMake ports. ADD_CUSTOM_COMMAND might be an acceptable solution, but ADD_CUSTOM_TARGET isn't very logical when you open a generated VisualStudioSolution. Also adding the dependencies across the WebKit2 CMake code does not seam like a clean solution. BTW: Current solution works for the WinApple port (https://bugs.webkit.org/attachment.cgi?id=155222&action=review).

> Source/WebKit2/CMakeLists.txt:613
> +WEBKIT_CREATE_FORWARDING_HEADERS(WebCore DIRECTORIES ${WebCore_FORWARDING_HEADERS_DIRECTORIES})
> +WEBKIT_CREATE_FORWARDING_HEADERS(JavaScriptCore DIRECTORIES ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES})

Why do you need them? Simple include directories should be enough?
And if you need them really the should go into the JavaScriptCore/WebCore CMakeLists.txt
Is it possible, that you post some make error messages?
Comment 7 Thiago Marcos P. Santos 2012-07-30 10:41:57 PDT
(In reply to comment #6)
> (From update of attachment 155293 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155293&action=review
> 
> (In reply to comment #2)
> > I have some rants about this newly introduced macro:
> It's more or less with every build system, that we duplicate functionality. :-(
> But the adding different targets and store the name in ForwardingHeadersForTestWebKitAPI_NAME is IMHO a much much biger hack. This doesn't scale very well and duplicates the functionality about different CMake ports. ADD_CUSTOM_COMMAND might be an acceptable solution, but ADD_CUSTOM_TARGET isn't very logical when you open a generated VisualStudioSolution. Also adding the dependencies across the WebKit2 CMake code does not seam like a clean solution. BTW: Current solution works for the WinApple port (https://bugs.webkit.org/attachment.cgi?id=155222&action=review).
> 
> > Source/WebKit2/CMakeLists.txt:613
> > +WEBKIT_CREATE_FORWARDING_HEADERS(WebCore DIRECTORIES ${WebCore_FORWARDING_HEADERS_DIRECTORIES})
> > +WEBKIT_CREATE_FORWARDING_HEADERS(JavaScriptCore DIRECTORIES ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES})
> 
> Why do you need them? Simple include directories should be enough?

We need them because in a lot of places we use forwarding headers like WebCore/Stuff.h and JavascriptCore/Stuff.h

> And if you need them really the should go into the JavaScriptCore/WebCore CMakeLists.txt

No, it should not. You don't need forwarding headers for building WebCore and JSC. You need them only for WK2.

> Is it possible, that you post some make error messages?

Yes, I can arrange that for you. But I'm curious how can you build wincairo? Are you really doing a clean build?
Comment 8 Patrick R. Gansterer 2012-07-30 10:45:27 PDT
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 155293 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155293&action=review
> > 
> > (In reply to comment #2)
> > > I have some rants about this newly introduced macro:
> > It's more or less with every build system, that we duplicate functionality. :-(
> > But the adding different targets and store the name in ForwardingHeadersForTestWebKitAPI_NAME is IMHO a much much biger hack. This doesn't scale very well and duplicates the functionality about different CMake ports. ADD_CUSTOM_COMMAND might be an acceptable solution, but ADD_CUSTOM_TARGET isn't very logical when you open a generated VisualStudioSolution. Also adding the dependencies across the WebKit2 CMake code does not seam like a clean solution. BTW: Current solution works for the WinApple port (https://bugs.webkit.org/attachment.cgi?id=155222&action=review).
> > 
> > > Source/WebKit2/CMakeLists.txt:613
> > > +WEBKIT_CREATE_FORWARDING_HEADERS(WebCore DIRECTORIES ${WebCore_FORWARDING_HEADERS_DIRECTORIES})
> > > +WEBKIT_CREATE_FORWARDING_HEADERS(JavaScriptCore DIRECTORIES ${JavaScriptCore_FORWARDING_HEADERS_DIRECTORIES})
> > 
> > Why do you need them? Simple include directories should be enough?
> 
> We need them because in a lot of places we use forwarding headers like WebCore/Stuff.h and JavascriptCore/Stuff.h

Doesn't the ForwardingHeaders directory work there?

> > And if you need them really the should go into the JavaScriptCore/WebCore CMakeLists.txt
> 
> No, it should not. You don't need forwarding headers for building WebCore and JSC. You need them only for WK2.

Wrong: Apple ports need them

> > Is it possible, that you post some make error messages?
> 
> Yes, I can arrange that for you. But I'm curious how can you build wincairo? Are you really doing a clean build?

I never built WK2 with anything else than CMake, but I'll try with a clean build.
Comment 9 Thiago Marcos P. Santos 2012-07-30 11:19:46 PDT
Created attachment 155324 [details]
Patch
Comment 10 Thiago Marcos P. Santos 2012-07-30 11:41:42 PDT
Created attachment 155329 [details]
Patch
Comment 11 Thiago Marcos P. Santos 2012-07-30 11:42:10 PDT
(In reply to comment #10)
> Created an attachment (id=155329) [details]
> Patch

Added as a reference of what are the folders needed by EFL port for the new implementation.