Summary: | [WPE] Fix CMake rules in for TestWebKitAPIBase library building in developer mode | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pablo Saavedra <psaavedra> | ||||||||
Component: | CMake | Assignee: | 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
Pablo Saavedra
2018-10-06 04:40:45 PDT
Created attachment 351720 [details]
patch
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 on attachment 351720 [details]
patch
Thanks!
Please respond to Don's comments.
(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. Created attachment 353025 [details]
patch
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 (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. Created attachment 353036 [details]
patch
This LGTM now. Thank you for updating it I think its much better now. Comment on attachment 353036 [details] patch Clearing flags on attachment: 353036 Committed r237390: <https://trac.webkit.org/changeset/237390> All reviewed patches have been landed. Closing bug. 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) (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. (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 =) |