Bug 190328

Summary: [WPE] Fix CMake rules in for TestWebKitAPIBase library building in developer mode
Product: WebKit Reporter: Pablo Saavedra <psaavedra>
Component: CMakeAssignee: Pablo Saavedra <psaavedra>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, aperez, bugs-noreply, commit-queue, don.olmstead, Hironori.Fujii, loic.yhuel, mcatanzaro, sam, sbarati, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Other   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch none

Description Pablo Saavedra 2018-10-06 04:40:45 PDT
This situation happens using a cross-compiled toolchain to build WPE with delevoper mode ON (API tests, layout tests, ...):


  . /Tools/Scripts/build-webkit --release --wpe  --cmakeargs="-DBUILD_SHARED_LIBS=ON" --no-experimental-features



The current cmake rules doesn't include required object files and librearies to get a succesful build: More especifically:

  Tools/wpe/backends/CMakeFiles/WPEToolingBackends.dir/HeadlessViewBackend.cpp.o
  Tools/wpe/backends/CMakeFiles/WPEToolingBackends.dir/ViewBackend.cpp.o

and

  /home/igalia/psaavedra/toolchain_env/wandboard_mesa/sysroots/armv7ahf-neon-poky-linux-gnueabi/usr/lib/libWPEBackend-fdo-0.1.so


This is the error:

[2885/2972] Linking CXX shared library lib/libTestWebKitAPIBase.so
FAILED: lib/libTestWebKitAPIBase.so 
...
Tools/TestWebKitAPI/CMakeFiles/TestWebKitAPIBase.dir/wpe/PlatformWebViewWPE.cpp.o: In function `TestWebKitAPI::PlatformWebView::initialize(OpaqueWKPageConfiguration const*)':
/home/igalia/psaavedra/WebKit/WebKitBuild/Release/../../Tools/TestWebKitAPI/wpe/PlatformWebViewWPE.cpp:69: undefined reference to `WPEToolingBackends::HeadlessViewBackend::HeadlessViewBackend(unsigned int, unsigned int)'
/home/igalia/psaavedra/WebKit/WebKitBuild/Release/../../Tools/TestWebKitAPI/wpe/PlatformWebViewWPE.cpp:70: undefined reference to `WPEToolingBackends::ViewBackend::backend() const'
Tools/TestWebKitAPI/CMakeFiles/TestWebKitAPIBase.dir/wpe/PlatformWebViewWPE.cpp.o: In function `TestWebKitAPI::PlatformWebView::~PlatformWebView()':
/home/igalia/psaavedra/WebKit/WebKitBuild/Release/../../Tools/TestWebKitAPI/wpe/PlatformWebViewWPE.cpp:64: undefined reference to `WPEToolingBackends::HeadlessViewBackend::~HeadlessViewBackend()'
Comment 1 Pablo Saavedra 2018-10-06 04:43:07 PDT
Created attachment 351720 [details]
patch
Comment 2 Don Olmstead 2018-10-07 14:08:39 PDT
Comment on attachment 351720 [details]
patch

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

Informal review. Just need to move these things into the right places.

> Tools/TestWebKitAPI/CMakeLists.txt:288
> +    if (PORT STREQUAL "WPE")
> +        find_package(WPEBackend-fdo REQUIRED)
> +        list(APPEND TestWebKitAPI_LIBRARIES ${WPEBACKEND_FDO_LIBRARIES})
> +        list(APPEND TestWebKitAPIBase_SOURCES
> +            ${TOOLS_DIR}/wpe/backends/ViewBackend.cpp
> +            ${TOOLS_DIR}/wpe/backends/HeadlessViewBackend.cpp
> +        )
> +    endif ()

There is a PlatformWPE.cmake file in this directory. This shouldn't be in the root CMake file.

> Tools/TestWebKitAPI/CMakeLists.txt:299
> +    if (PORT STREQUAL "WPE")
> +        target_link_libraries(TestWebKitAPIBase WPEBackend-fdo-0.1)
> +    endif ()

This also should be doable in the PlatformWPE.cmake file.
Comment 3 Michael Catanzaro 2018-10-15 12:00:38 PDT
Comment on attachment 351720 [details]
patch

Thanks!

Please respond to Don's comments.
Comment 4 Pablo Saavedra 2018-10-23 04:15:20 PDT
(In reply to Don Olmstead from comment #2)
> Comment on attachment 351720 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=351720&action=review
> 
> Informal review. Just need to move these things into the right places.
> 
> > Tools/TestWebKitAPI/CMakeLists.txt:288
> > +    if (PORT STREQUAL "WPE")
> > +        find_package(WPEBackend-fdo REQUIRED)
> > +        list(APPEND TestWebKitAPI_LIBRARIES ${WPEBACKEND_FDO_LIBRARIES})
> > +        list(APPEND TestWebKitAPIBase_SOURCES
> > +            ${TOOLS_DIR}/wpe/backends/ViewBackend.cpp
> > +            ${TOOLS_DIR}/wpe/backends/HeadlessViewBackend.cpp
> > +        )
> > +    endif ()
> 
> There is a PlatformWPE.cmake file in this directory. This shouldn't be in
> the root CMake file.
> 
> > Tools/TestWebKitAPI/CMakeLists.txt:299
> > +    if (PORT STREQUAL "WPE")
> > +        target_link_libraries(TestWebKitAPIBase WPEBackend-fdo-0.1)
> > +    endif ()
> 
> This also should be doable in the PlatformWPE.cmake file.

Hi, I'm sorry for the delay. I was out for 2 weeks. 

Thanks for the review. OK with your suggestions I will try to fix it ASAP.
Comment 5 Pablo Saavedra 2018-10-24 00:10:19 PDT
Created attachment 353025 [details]
patch
Comment 6 Michael Catanzaro 2018-10-24 08:04:15 PDT
Comment on attachment 353025 [details]
patch

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

Looks good, just please change the casing of the variable names. I see the style is a bit inconsistent in this file, but generally we use this style:

> Tools/TestWebKitAPI/CMakeLists.txt:88
> +    set(test_webKit_api_base_LIBRARIES

TestWebKitAPIBase_LIBRARIES

> Tools/TestWebKitAPI/CMakeLists.txt:95
> +    set(test_webKit_api_base_SOURCES

TestWebKitAPIBase_SOURCES
Comment 7 Pablo Saavedra 2018-10-24 08:36:23 PDT
(In reply to Michael Catanzaro from comment #6)
> Comment on attachment 353025 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=353025&action=review
> 
> Looks good, just please change the casing of the variable names. I see the
> style is a bit inconsistent in this file, but generally we use this style:
> 
> > Tools/TestWebKitAPI/CMakeLists.txt:88
> > +    set(test_webKit_api_base_LIBRARIES
> 
> TestWebKitAPIBase_LIBRARIES
> 
> > Tools/TestWebKitAPI/CMakeLists.txt:95
> > +    set(test_webKit_api_base_SOURCES
> 
> TestWebKitAPIBase_SOURCES

Yes it is a bit inconsistent. I changed the style to match with the latest changes introduced in the file.
Comment 8 Pablo Saavedra 2018-10-24 08:38:08 PDT
Created attachment 353036 [details]
patch
Comment 9 Don Olmstead 2018-10-24 08:49:35 PDT
This LGTM now. Thank you for updating it I think its much better now.
Comment 10 WebKit Commit Bot 2018-10-24 10:44:28 PDT
Comment on attachment 353036 [details]
patch

Clearing flags on attachment: 353036

Committed r237390: <https://trac.webkit.org/changeset/237390>
Comment 11 WebKit Commit Bot 2018-10-24 10:44:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Loïc Yhuel 2019-02-21 06:56:37 PST
Any reason why TestWebKitAPIBase_LIBRARIES is constructed with an explicit WPEBackend-fdo-0.1, instead of ${WPEBACKEND_FDO_LIBRARIES} ?

If WPEBackend-fdo is compiled separately and installed in /usr/local, only the latter works.
CMake sets WPEBACKEND_FDO_LIBRARIES=/usr/local/lib64/libWPEBackend-fdo-0.1.so, which allows a build with only :
 - PKG_CONFIG_PATH=/usr/local/lib64/pkgconfig so cmake finds wpe/wpebackend-fdo
 - LD_LIBRARY_PATH=/usr/local/lib64 for gtkdoc (wpe-webextensions-0.1-scan seems to be built without rpath unlike the CMake-generated binaries)
Comment 13 Adrian Perez 2019-02-21 08:14:47 PST
(In reply to Loïc Yhuel from comment #12)
> Any reason why TestWebKitAPIBase_LIBRARIES is constructed with an explicit
> WPEBackend-fdo-0.1, instead of ${WPEBACKEND_FDO_LIBRARIES} ?

No reason. Moreover: I agree with you and I am convinced that
currently this is wrong.

I have just been hit by this while trying to make a 2.23.90 release
tarball during the distcheck, and I have a local patch which I'll be
landing momentarily.
Comment 14 Adrian Perez 2019-02-21 08:22:20 PST
(In reply to Adrian Perez from comment #13)
> (In reply to Loïc Yhuel from comment #12)
> > Any reason why TestWebKitAPIBase_LIBRARIES is constructed with an explicit
> > WPEBackend-fdo-0.1, instead of ${WPEBACKEND_FDO_LIBRARIES} ?
> 
> No reason. Moreover: I agree with you and I am convinced that
> currently this is wrong.
> 
> I have just been hit by this while trying to make a 2.23.90 release
> tarball during the distcheck, and I have a local patch which I'll be
> landing momentarily.

See bug #194901 for the patch =)