Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmake: Use an imported library for libwpe #59

Merged
merged 1 commit into from Jul 30, 2019

Conversation

@aperezdc
Copy link
Member

commented Jul 26, 2019

This modifies FindWPE.cmake to make it create an imported library target, which is then passed to target_link_libraries(). The advantage of this approach is that adding the imported library as a dependency will automatically propagate the compiler flags and include directories when building source files, and also propagate all the needed linker flags and library names for the linker command line.

Note that how find_path() and find_library() invocations are done in FindWPE.cmake is subtly changed: pkg-config is considered to be the “source of truth” for how to use a library, but we still want to fall back to let CMake try and find it in case pkg-config was not able to. Therefore, the output variables are renamed to make it clearer that they will contain only one directory (WPE_INCLUDEDIR) and one library (WPE_LIBRARY):

  • If pkg-config was successful, the include/library directories obtained from it are used as hints where to find the files, but the actual compiler and linker options are taken from the PkgConfig::WPE imported library target elsewhere.
  • Else, if pkg-config failed to return something there is no imported library target defined, but it is still possible that find_path() and find_library() manage to find something. In this case the imported library target is constructed by hand with the paths found.

Lastly, WPE_INCLUDEDIR and WPE_LIBRARY are used to determine whether findind the library was successful (they are passed as required to FIND_PACKAGE_HANDLE_STANDARD_ARGS), because they have to be always defined to some valid path regardless of whether pkg-config is providing the information or CMake found the paths by itself.

# figured out where the header and library are, create the PkgConfig::WPE
# imported target anyway with the found paths.
#
if (WPE_LIBRARY AND NOT TARGET PkgConfig::WPE)

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 26, 2019

Collaborator

Doesn't this seem like a lot of extra work for the insane case that libwpe is installed but the pkg-config is mysteriously broken? I think if pkg-config doesn't return a result then we should just trust that it's not found?

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 26, 2019

Author Member

We need this as a fallback for systems like Windows where getting pkg-config to work is a hit-and-miss task. As you pointed out on IRC in theory pkgconf works on Windows, but I don't know how widespread it is (or how well known it is that it works) so it seems better to keep the fallback and avoid breaking people's build setups around.

I would still want to revisit this topic in the future, try things, and see how far we could get with pkgconf, but I would say that such thing would be out of the scope of this PR.

@@ -59,7 +58,7 @@ set(WPEBACKEND_FDO_LIBRARIES
${GLIB_LIBRARIES}
${WAYLAND_LIBRARIES}
${WAYLAND_EGL_LIBRARIES}
${WPE_LIBRARIES}
PkgConfig::WPE

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 26, 2019

Collaborator

As discussed on IRC, this naming is lame. Don suggested creating an alias in FindWPE.cmake so nothing outside the find module needs to write PkgConfig. Perhaps WPE::libwpe.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 26, 2019

Author Member

I'll be updating this later to use WPE::libwpe 👌

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 29, 2019

Author Member

Well, I have been trying the following, which creates an alias:

add_library(WPE::libwpe ALIAS PkgConfig::PC_WPE)

…but then PkgConfig::PC_WPE needs be be a global target. With CMake ≥3.13 we can pass GLOBAL to pkg_check_modules() but that won't work with older versions, and AFAIU we want to support CMake 3.10.x because that's what our older supported distributions ship.

It will take me a little longer, but it seems for compatibility with older versions I am going to need manually defining a new imported library target (not an alias), and then copy the properties from PkgConfig::PC_WPE to it.

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 29, 2019

Collaborator

It will take me a little longer, but it seems for compatibility with older versions I am going to need manually defining a new imported library target (not an alias), and then copy the properties from PkgConfig::PC_WPE to it.

Well it would be silly to do this much manual work for every find module in WebKit. The goal here is to use this as a prototype for restructuring all of our find modules, so I think we should talk with Don before resorting to such a workaround.

But my interpretation of our findings thus far is "lol find modules have never worked properly and there is no idiomatic or canonical way to write them," so I guess I should lower my expectations.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 29, 2019

Author Member

This was dramatically easier than expected in the end. Basically, when the library is found and the WPE::libwpe imported library target needs to be defined, boils down to:

add_library(WPE::libwpe INTERFACE IMPORTED)
if (TARGET PkgConfig::WPE)  # Defined by pkg_check_modules, if module found.
    target_link_libraries(WPE::libwpe INTERFACE PkgConfig::WPE)
else ()  # Use the paths from find_path/find_library
    set_property(TARGET WPE::libwpe PROPERTY
        INTERFACE_LINK_LIBRARIES "${WPE_LIBRARY}")
    set_property(TARGET WPE::libwpe PROPERTY
       INTERFACE_INCLUDE_DIRECTORIES "${WPE_INCLUDE_DIR}")
endif ()

This avoids needing ALIAS at all, because dependencies are transitive: using WPE::libwpe transitively adds the flags from PkgConfig::WPE as needed.

As a bonus, with this approach the Find<xyz>.cmake can add and modify properties to WPE::libwpe on top of what pkg_check_modules() has already configured for PkgConfig::WPE—right now I cannot think of an use-case, but the possibility is there.

This comment has been minimized.

Copy link
@mcatanzaro

mcatanzaro Jul 29, 2019

Collaborator

But doesn't it discard CFLAGS_OTHER and LDFLAGS_OTHER? It's just not enough.

This comment has been minimized.

Copy link
@aperezdc

aperezdc Jul 29, 2019

Author Member

The PkgConfig::WPE imported target carries all the flags (including _CFLAGS_OTHER and _LDFLAGS_OTHER) if I am reading the FindPkgConfig.cmake file included with CMake correctly. That means that “depending on” WPE::libwpe will bring the flags to the compiler and linker command lines, because it in turn depends on PkgConfig::WPE.

In the fallback case where pkg_check_modules() failed, but find_path() and find_library() succeeded, we only know where the library and the headers are so that is why only the INTERFACE_LINK_LIBRARIES and INTERFACE_INCLUDE_DIRECTORIES are set—we do not know anything else.

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 26, 2019

Once we have this landed, it would be nice to bring back the changes to other source trees which have a FindWPE.cmake like WebKit, Cog, the FDO backend, and so on. I think we can start slowly from there, and go changing one Find<XYZ>.cmake file at a time.

@mcatanzaro

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

cmake: Use an imported library for libwpe
This modifies FindWPE.cmake to make it create the WPE::libwpe imported
library target, which is then passed to target_link_libraries(). The
advantage of this approach is that adding the imported library as a
dependency will automatically propagate the compiler flags and include
directories when building source files, and also propagate all the
needed linker flags and library names for the linker command line.

Note that how find_path() and find_library() invocations are done in
FindWPE.cmake is subtly changed: pkg-config is considered to be the
"source of truth" for how to use a library, but we still want to fall
back to let CMake try and find it in case pkg-config was not able to.
Therefore, the output variables are renamed to make it clearer that
they will contain only one directory (WPE_INCLUDEDIR) and one library
(WPE_LIBRARY):

 - If pkg-config was successful, the include/library directories
   obtained from it are used as hints where to find the files, but the
   actual compiler and linker options are taken from the PkgConfig::WPE
   imported library target.

 - Else, if pkg-config failed to return something there is no imported
   library target defined, but it is still possible that find_path() and
   find_library() manage to find something. In this case the imported
   library target is constructed by hand with the paths found.

Lastly, WPE_INCLUDEDIR and WPE_LIBRARY are used to determine whether
findind the library was successful (they are passed as required to
FIND_PACKAGE_HANDLE_STANDARD_ARGS), because they have to be always
defined to some valid path regardless of whether pkg-config is providing
the information or CMake found the paths by itself.

@aperezdc aperezdc force-pushed the cmake-libwpe-importedtarget branch from e161744 to 32f3ea3 Jul 29, 2019

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

I am quite satisfied about how this is looking like, but would like to have some feedback before merging this. In particular, some critique from someone like @donny-dont who knows CMake better 😄

@aperezdc aperezdc requested review from zdobersek and clopez Jul 29, 2019

@mcatanzaro

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

OK I guess this look good to me.

Since the goal is to use this as an example for rewriting all of WebKit's find modules, we should make sure we have consensus on it before committing. Don is already CCed. Anyone know how to CC Fujii? CC: @annulen

@annulen

This comment has been minimized.

Copy link

commented Jul 29, 2019

CMake code looks good, but on high level this change makes no sense. libwpe is built with cmake, so you should add a couple of lines into libwpe project to install cmake module file, and do find_package(WPE) here, and remove FindWPE.cmake.

@mcatanzaro

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

CMake code looks good, but on high level this change makes no sense. libwpe is built with cmake, so you should add a couple of lines into libwpe project to install cmake module file, and do find_package(WPE) here, and remove FindWPE.cmake.

Well I don't want to do that for the same reason WebKit doesn't ship a FindWebKit.cmake: because that turns CMake into a public API, rather than an implementation detail. Projects should use pkg-config to find libwpe.

Let's preserve our ability to merge #3 without applications using libwpe noticing. :)

@annulen

This comment has been minimized.

Copy link

commented Jul 29, 2019

FWIW, nothing prevents you from generating cmake module from Meson build

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

I would rather not generate a CMake module for libwpe (nor for our other modules) for one simple reason: it seems better to have a single source of truth, than two (CMake module and pkg-config module) which get out of sync. A side reason is that most of the GNU/Linux ecosystem relies on pkg-config, so even if we shipped a CMake module, we cannot stop providing the pkg-config one; many tools can use a .pc file, only (?) CMake can use CMake modules. I presume that one reason is that CMake modules are scripts (one could DoS a build by creating an infinite loop!), while a .pc file is declarative format which just carries data (much saner!).

Another reason is that if we move smaller modules like libwpe to Meson, I presume we'll still want to keep using CMake for WebKit itself for a longer time. This PR is initial legwork to improve how we do things with CMake and the idea is to apply the improvements to the WebKit source tree as well—it's just much easier to experiment in a smaller project like libwpe. It's not like we want to completely dive in into the CMake ecosystem.

@annulen

This comment has been minimized.

Copy link

commented Jul 29, 2019

Disclaimer: I'm not here to make you "dive in into the CMake ecosystem" or anything like that.

it seems better to have a single source of truth, than two (CMake module and pkg-config module)

Source of truth is your build system which generates these files

which get out of sync

I cannot imagine how is it possible, provided that all of these files are artifacts of the same install process (ok, except maybe Debian people who can voluntarily remove some of your installed files). For example, in Qt we ship pkg-config files, qmake modules and cmake modules, generated from the same build, and they are not getting out of sync (because it's not possible)

I presume that one reason is that CMake modules are scripts (one could DoS a build by creating an infinite loop!)

Only if you put infinite loop into your script.

Note that if you are shipping only .pc files, any CMake project using your library has to copy your find modules into their project (and this is the place where files can actually get out of sync), or reinvent the wheel.

@mcatanzaro

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

Note that if you are shipping only .pc files, any CMake project using your library has to copy your find modules into their project (and this is the place where files can actually get out of sync), or reinvent the wheel.

No they don't... they can just use pkg_check_modules() directly if desired and save the trouble. I've seen CMake projects that do this directly in CMakeLists.txt and have no need for find modules at all. That seems like an ideal to me, but Don doesn't want to do that with WebKit due to some difficulty using pkg-config on Windows, so here we are.

Anyway, that's a deficiency of CMake, and projects using CMake are a very small minority, so it doesn't really matter.

@annulen

This comment has been minimized.

Copy link

commented Jul 29, 2019

Ok

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Note that if you are shipping only .pc files, any CMake project using your library has to copy your find modules into their project (and this is the place where files can actually get out of sync), or reinvent the wheel.

No they don't... they can just use pkg_check_modules() directly if desired and save the trouble. I've seen CMake projects that do this directly in CMakeLists.txt and have no need for find modules at all. […]

For example Cog uses pkg_check_modules() directly—it just assumes an Unix-y build environment and that pkg-config is always available because it does not work on Windows (which is a non-goal currently).

[…] That seems like an ideal to me, but Don doesn't want to do that with WebKit due to some difficulty using pkg-config on Windows, so here we are.

Exactly, we are trying to accommodate the needs of everybody who are using WPE WebKit and related components. On the bright side, we only have to maintain the Find<xyz>.cmake files working cross-platform for libwpe, WPEBackend-fdo, and WebKit… which we have a good grasp on, so it's not a big deal to share copies of the files among them.

@aperezdc aperezdc requested a review from mcatanzaro Jul 30, 2019

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

By the way, I suppose we can merge this now and continue with some follow-ups in the same vein. Or is there some other concern we may need to address first?

@annulen
Copy link

left a comment

Why are WPE_INCLUDE_DIRS and WPE_LIBRARIES renamed? These variables remain public, and previous ones follow existing convention

@aperezdc

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

Why are WPE_INCLUDE_DIRS and WPE_LIBRARIES renamed? These variables remain public, and previous ones follow existing convention

One important thing to kep in mind is that reading the .pc file may return multiple libraries and/or multiple include directories. This can happen easily e.g. if using static builds (all the .a archives of the dependencies will be returned as well) or when the .pc file has dependencies which themselves add include directories.

The result variables for find_path() and find_library() will contain only one path: using WPE_INCLUDE_DIRS and WPE_LIBRARIES would be misleading, and also would overwrite the values obtained from pkg-config (which, as mentioned, can actually contain multiple items).

Overwriting the variables which can contain multiple values with the result of a search which is always one value will cause build issues (see WebPlatformForEmbedded/libwpe#48 for a fix we needed recently for such a situation).

Therefore, I want to have different variable names for the results obtained from pkg-config and for the results obtained from find_path()/find_library() because their semantics are different.

@aperezdc aperezdc merged commit ece5827 into master Jul 30, 2019

@aperezdc aperezdc deleted the cmake-libwpe-importedtarget branch Jul 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.