swaywm / wlroots

A modular Wayland compositor library
https://gitlab.freedesktop.org/wlroots/wlroots/
MIT License
2.15k stars 341 forks source link

Use system paths for header directories instead of including them directly #1156

Closed sdilts closed 6 years ago

sdilts commented 6 years ago

Due to the way that external libraries' header files are included in the wlroots headers, using wlroots with a simple build system is greatly complicated. In the current system, subdirectories of external components are added to the include path of wlroots. For example, this means that wlroots developers can write

#include <wayland-server.h>

in public header files instead of

#include <wayland/wayland-server.h>

The problem with this is that every single project that uses wlroots must also add /usr/include/wayland, or the system's equivalent, to the include path in order for their project to compile. This is the same for the other external libraries that are included in this manner,

Forcing developers to add unused libraries to the include path complicates the build process unnecessarily. To see why, look at rootson and its dependency on pixman. rootson never directly calls anything from the pixman library, yet it still requires the pixman header directory to be added to the include path.

ddevault commented 6 years ago

I don't really see this as a problem. It's not terribly hard to enumerate the few dependencies which make it to our headers and add them to your include path.

ddevault commented 6 years ago

Also, we use the include paths specified by pkg-config, which is the Right Way to do it.

emersion commented 6 years ago

Note that most of the time, if you include a header that depends on say pixman, you'll most likely need to link to pixman anyway to be able to use the pixman_region32_t we give you.

sdilts commented 6 years ago

Nice! I didn't know about pkg-config. It fixes my issue. Thanks! I do have two more questions though.

  1. Although most users of this project probably have more experience than I do, would it help to put something on the readme about this? I.e. wlroots works with pkg-config?

  2. I just tried compiling the project again. Although pkg-config is reporting the correct result for libinput and meson finds it as well, it's not being added to the include path, and doesn't compile. Can you help me here, or should I create another issue? Starting from scratch doesn't fix the issue.

ddevault commented 6 years ago

Although most users of this project probably have more experience than I do, would it help to put something on the readme about this? I.e. wlroots works with pkg-config?

No, this is basic Unix programming literacy knowledge.

I just tried compiling the project again. Although pkg-config is reporting the correct result for libinput and meson finds it as well, it's not being added to the include path, and doesn't compile. Can you help me here, or should I create another issue? Starting from scratch doesn't fix the issue.

It sounds like you might want to ask for help in ##workingset on freenode or something similar. We're not an intro to programming on Unix class.

sdilts commented 6 years ago

Thanks!

martinetd commented 6 years ago

On the other hand the first message here points to a problem in our .pc file, if we use #include <wayland-server.h> in our headers then we need to move wayland-server from Requires.private to Requires. . . .

This is done with something like that:

diff --git a/meson.build b/meson.build
index 5fd89832..88b72003 100644
--- a/meson.build
+++ b/meson.build
@@ -219,6 +219,7 @@ pkgconfig.generate(
        version: meson.project_version(),
        filebase: meson.project_name(),
        name: meson.project_name(),
+       requires: wayland_server,
        description: 'Wayland compositor library',
 )

@sdilts: I don't use wlroots so I don't know if there are other include directories that you are missing, please fiddle with that requires field (it can take a list of dependencies) and once you have something you're happy with open a PR with it

martinetd commented 6 years ago

Heads up on this, there's been some discussion on the fedora lists so I looked into it a bit more, and the behaviour of Requires.private is dependant on the pkg-config implementation (freedesktop's or pkgconf)

freedesktop has a bug open about it, it would seem the plan is to add a new Requires.internal flag for static linking, and then use Requires.private for cflags as well, so we wouldn't need to change anything.

pkgconf already has this implemented so that seems like they agreed with the principle, and you can just switch to pkgconf or patch your .pc file locally until freedesktop's get this done if you depend on this.

As such, I'm closing the bug back -- note that at some point meson might move the dependencies from Requires.private to Requires.internal and when that happens we'll need to list what's needed for headers again, but for now we don't need to worry about it. If someone cares about supporting both pkg-config feel free to reopen this