Bug 154450 – [cmake] Moved library setup code to WEBKIT_FRAMEWORK macro.

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Konstantin Tokarev]

Moved library setup code to WEBKIT_FRAMEWORK macro.

[Konstantin Tokarev]

Created attachment 271750 [details]
Patch

[Konstantin Tokarev]

Created attachment 271785 [details]
Patch

[Konstantin Tokarev]

Created attachment 271798 [details]
Patch

[Konstantin Tokarev]

Created attachment 271855 [details]
Patch

[Konstantin Tokarev]

Created attachment 271860 [details]
Patch

[Michael Catanzaro]

Comment on attachment 271860 [details]
Patch

I think the value of moving this duplicate code to a macro probably outweighs the value of being explicit, so I'm OK with this. I want to give Alex and Martin a chance to chime in, though.

I'm not sure about getting rid of WebKit2_DERIVED_SOURCES, because I think we might want to set different CPPFLAGS there to silence some warnings we currently have. And Alex has a patch introducing WebCore_DERIVED_SOURCES in bug #151399.

[Konstantin Tokarev]

(In reply to comment #6)
> I think the value of moving this duplicate code to a macro probably
> outweighs the value of being explicit, so I'm OK with this.

It seems to me that there is established set of "standard" variables for each WebKit target that configure it. So this patch would not only reduce code size, but would also help to make target behavior more consistent in future. I'm planning to submit similar patches for binaries and non-framework libraries.

Also, it would simplify addition of new configurable properties, e.g. in Qt port we would like to override precompiled header files via variables like WebCore_PRECOMPILED_HEADER.

 I want to give
> Alex and Martin a chance to chime in, though.
> 
> I'm not sure about getting rid of WebKit2_DERIVED_SOURCES, because I think
> we might want to set different CPPFLAGS there to silence some warnings we
> currently have. And Alex has a patch introducing WebCore_DERIVED_SOURCES in
> bug #151399.

Note that he adds WebCore_DERIVEDSOURCES, not WebCore_DERIVED_SOURCES, breaking "convention" of WebKit2. If we really need to use different flags on derived sources, I will add ${${_target}_DERIVED_SOURCES} to source list of target.

[Konstantin Tokarev]

Created attachment 271864 [details]
Patch

[Konstantin Tokarev]

Restored DERIVED_SOURCES.

[Alex Christensen]

Comment on attachment 271864 [details]
Patch

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

> Source/WebKit2/CMakeLists.txt:-789
> -    set_target_properties(WebKit2 PROPERTIES OUTPUT_NAME ${WebKit2_OUTPUT_NAME})

I was under the impression set_target_properties only worked after the target had been made with add_library already.  This is only used on Windows to my knowledge, and Windows EWS would report a successful build with different output names, which we would not want.

[Konstantin Tokarev]

Comment on attachment 271864 [details]
Patch

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

>> Source/WebKit2/CMakeLists.txt:-789
>> -    set_target_properties(WebKit2 PROPERTIES OUTPUT_NAME ${WebKit2_OUTPUT_NAME})
> 
> I was under the impression set_target_properties only worked after the target had been made with add_library already.  This is only used on Windows to my knowledge, and Windows EWS would report a successful build with different output names, which we would not want.

GTK+ port uses WebKit2_OUTPUT_NAME, and I've verified that it works.

[Alex Christensen]

Comment on attachment 271864 [details]
Patch

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

>>> Source/WebKit2/CMakeLists.txt:-789
>>> -    set_target_properties(WebKit2 PROPERTIES OUTPUT_NAME ${WebKit2_OUTPUT_NAME})
>> 
>> I was under the impression set_target_properties only worked after the target had been made with add_library already.  This is only used on Windows to my knowledge, and Windows EWS would report a successful build with different output names, which we would not want.
> 
> GTK+ port uses WebKit2_OUTPUT_NAME, and I've verified that it works.

ok, sounds good.  I'll take a close look at the output after this lands, but I don't anticipate any problems, then.

[WebKit Commit Bot]

Comment on attachment 271864 [details]
Patch

Clearing flags on attachment: 271864

Committed r196947: <http://trac.webkit.org/changeset/196947>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.