Bug 92630

Created attachment 155288 [details] Patch

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.

Created attachment 155293 [details] Patch

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 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 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?

(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?

(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.

Created attachment 155324 [details] Patch

Created attachment 155329 [details] Patch

(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.