Bug 190328 – [WPE] Fix CMake rules in for TestWebKitAPIBase library building in developer mode

• Status: RESOLVED
• Resolution: FIXED
• Component: CMake

[Pablo Saavedra]

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()'

[Pablo Saavedra]

Created attachment 351720 [details]
patch

[Don Olmstead]

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.

[Michael Catanzaro]

Comment on attachment 351720 [details]
patch

Thanks!

Please respond to Don's comments.

[Pablo Saavedra]

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

[Pablo Saavedra]

Created attachment 353025 [details]
patch

[Michael Catanzaro]

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

[Pablo Saavedra]

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

[Pablo Saavedra]

Created attachment 353036 [details]
patch

[Don Olmstead]

This LGTM now. Thank you for updating it I think its much better now.

[WebKit Commit Bot]

Comment on attachment 353036 [details]
patch

Clearing flags on attachment: 353036

Committed r237390: <https://trac.webkit.org/changeset/237390>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Loïc Yhuel]

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)

[Adrian Perez]

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

[Adrian Perez]

(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 =)