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, saam, sam, ysuzuki
Priority: P2    
Version: WebKit Local Build   
Hardware: Other   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
none
patch none

Pablo Saavedra
Reported 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()'
Attachments
patch (2.01 KB, patch)
2018-10-06 04:43 PDT, Pablo Saavedra
no flags
patch (2.92 KB, patch)
2018-10-24 00:10 PDT, Pablo Saavedra
no flags
patch (2.90 KB, patch)
2018-10-24 08:38 PDT, Pablo Saavedra
no flags
Pablo Saavedra
Comment 1 2018-10-06 04:43:07 PDT
Don Olmstead
Comment 2 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.
Michael Catanzaro
Comment 3 2018-10-15 12:00:38 PDT
Comment on attachment 351720 [details] patch Thanks! Please respond to Don's comments.
Pablo Saavedra
Comment 4 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.
Pablo Saavedra
Comment 5 2018-10-24 00:10:19 PDT
Michael Catanzaro
Comment 6 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
Pablo Saavedra
Comment 7 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.
Pablo Saavedra
Comment 8 2018-10-24 08:38:08 PDT
Don Olmstead
Comment 9 2018-10-24 08:49:35 PDT
This LGTM now. Thank you for updating it I think its much better now.
WebKit Commit Bot
Comment 10 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>
WebKit Commit Bot
Comment 11 2018-10-24 10:44:30 PDT
All reviewed patches have been landed. Closing bug.
Loïc Yhuel
Comment 12 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)
Adrian Perez
Comment 13 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.
Adrian Perez
Comment 14 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 =)
Note You need to log in before you can comment on or make changes to this bug.