Page MenuHomePhabricator
Create Task
Maniphest T8761

Standardize external dependency handling
Open, TrivialPublic
Actions

Description

TL;DR

What I want to decide with this task:

  • Whether we use or not _dep suffix for external dependencies' variable names;
  • Where should they be declared/created and how to be used (see my idea below).

Suffixes

I've noticed that some external dependencies use the convention of _dep suffix on their dependency-object variable names, others don't. Since both are acceptable conventions, I just opened this task just to decide on which we shall follow.

Dependencies with _dep suffix:

  • Dbus (actually for some reason it is declared as just dep, not even dbus nor dbus_dep);
  • threads_dep;
  • scim_dep;
  • sys_dep.

Dependencies without suffix:

  • jpeg;
  • crypto;
  • lua;
  • lua_ffi;
  • check;
  • gnutls;
  • gstreamer;
  • rsvg;
  • x11;
  • xcb;
  • x11_xcb;
  • libmount;
  • udev;
  • valgrind;
  • unwind;
  • iconv;
  • bullet;
  • png;
  • tiff;
  • webp;
  • webpdemux;
  • libopenjp2;
  • json;
  • hyphen;
  • libtbm;
  • libdri2;
  • xshmfence;
  • xcb;
  • x11_xcb;
  • xcb_sync;
  • xcb_dri3;
  • libxkbcommon;
  • lz4;
  • _wayland_protocols;
  • wayland_client.

There are very few dependencies with _dep, so I could simply rename them to not have that suffix and would be fine. But if anyone feels that having the suffix for every external dependency is ok, I'm full OK with it as well (as I said, both are common conventions in many many projects using meson, and is pretty accepted by meson team).

Definition and usage

Apart from the dependencies cited in the last point, there are dependencies that are just thrown into a list:

  • ('avahi-client') -------- from ecore_avahi_deps at lib/ecore_avahi
  • ('dbus-1') -------- from eldbus_ext_deps at lib/eldbus
  • ('egl') -------- from gl_deps at lib/evas
  • ('egl') -------- from wl2_test_gl_deps at tests/ecore_wl2
  • ('fontconfig') -------- from evas_ext_none_static_deps at lib/evas
  • ('freetype2') -------- from evas_ext_none_static_deps at lib/evas
  • ('fribidi') -------- from evas_ext_none_static_deps at lib/evas
  • ('gbm') -------- from engine_deps at modules/evas/engines/gl_drm
  • ('gl') -------- from gl_deps at lib/evas
  • ('gl') -------- from wl2_test_gl_deps at tests/ecore_wl2
  • ('glesv2') -------- from gl_deps at lib/evas
  • ('glib-2.0') -------- from ecore_ext_deps at lib/ecore
  • ('glib-2.0') -------- from mod_deps at modules/ecore_imf/scim
  • ('gstreamer-1.0') -------- from generic_deps at generic/evas/gst
  • ('gthread-2.0') -------- from ecore_ext_deps at lib/ecore
  • ('harfbuzz') -------- from evas_ext_none_static_deps at lib/evas
  • ('ibus-1.0') -------- from mod_deps at modules/ecore_imf/ibus
  • ('glib-2.0') -------- from mod_deps at modules/ecore_imf/ibus
  • ('gbm') -------- from ecore_drm_pub_deps at lib/ecore_drm
  • ('libdrm', '>= 2.4') -------- from ecore_drm_pub_deps at lib/ecore_drm
  • ('libinput', '>= 1.6.0') -------- from ecore_drm_pub_deps at lib/ecore_drm
  • ('wayland-cursor', '>= 1.8.0 ') -------- from ecore_drm_pub_deps at lib/ecore_drm
  • ('xkbcommon', '>= 0.3.0') -------- from ecore_drm_pub_deps at lib/ecore_drm
  • ('libinput', '>=1.7.0') -------- from elput_ext_deps at lib/elput
  • ('libpulse') -------- from ecore_audio_ext_deps at lib/ecore_audio
  • ('libraw') -------- from generic_deps at generic/evas/raw
  • ('libspectre') -------- from generic_deps at generic/evas/ps
  • ('libudev') -------- from elput_ext_deps at lib/elput
  • ('libvncserver') -------- from engine_deps at modules/ecore_evas/vnc_server
  • ('pixman-1') -------- from evas_ext_none_static_deps at lib/evas
  • ('poppler-cpp') -------- from generic_deps at generic/evas/pdf
  • ('sdl2') -------- from ecore_sdl_ext_deps at lib/ecore_sdl
  • ('sdl2') -------- from engine_deps at modules/ecore_evas/engines/sdl
  • ('sndfile') -------- from ecore_audio_deps at lib/ecore_audio
  • ('tslib') -------- from ecore_fb_ext_deps at lib/ecore_fb
  • ('wayland-client') -------- from ecore_wl2_ext_deps at lib/ecore_wl2
  • ('wayland-server') -------- from ecore_wl2_ext_deps at lib/ecore_wl2
  • ('xkbcommon') -------- from ecore_wl2_ext_deps at lib/ecore_wl2
  • ('wayland-client') -------- from ecore_buffer_ext_deps at lib/ecore_buffer
  • ('wayland-server') -------- from ecore_buffer_ext_deps at lib/ecore_buffer
  • ('wayland-client', '>= 1.8.0') -------- from ecore_wayland_pub_deps at lib/ecore_wayland
  • ('wayland-cursor', '>= 1.8.0 ') -------- from ecore_wayland_pub_deps at lib/ecore_wayland
  • ('xkbcommon', '>= 0.5.0') -------- from ecore_wayland_pub_deps at lib/ecore_wayland
  • ('uuid') -------- from ecore_wayland_pub_deps at lib/ecore_wayland
  • ('wayland-egl') -------- from engine_deps at modules/evas/engines/wayland_egl
  • ('wayland-server', '>= 1.11.0') -------- from efl_canvas_wl_ext_deps at lib/efl_canvas_wl
  • ('xkbcommon', '>= 0.6.0') -------- from efl_canvas_wl_ext_deps at lib/efl_canvas_wl
  • ('xkbcommon', '>= 0.6.0') -------- from efl_canvas_wl_ext_deps at lib/efl_canvas_wl
  • ('xkbcommon', '>=0.3.0') -------- from elput_ext_deps at lib/elput
  • ('xkbcommon-x11') -------- from efl_canvas_wl_ext_deps at lib/efl_canvas_wl
  • ('zlib') -------- from emile_ext_deps at lib/emile
  • ('zlib') -------- from generic_deps at generic/evas/xcf

So my points on these:

  1. About versions: as it can be seen, some of them repeat -- sometimes a full repetition of dependency('lib'), sometimes even the version differs. Should there be a difference in versions for these ones? i.e. Does ecore_wayland made to be specifically compatible with wayland-client >= 1.8.0 while ecore_wl2 and ecore_buffer can work with "any" installed version?
  2. About "where to define": I think it could be a problem by having two places where the same dependency is declared: if we change version constraints that should apply for the whole EFL, for example, we would have to remember to change in every other place where it is defined. There's already some external dependencies being checked (and thus defined) in header_checks. My suggestion would be:
    • If the dependency is being defined only in that place and not expected to be used anywhere else, then it's pretty ok to define in the file it is used.
    • If the dependency will be used for 2 or more places, define it in a variable in a common place and use that variable. The place could be header_checks itself or in main meson.build.

Related Objects

jptiz added a project: efl.Jun 23 2020, 10:48 AM
jptiz claimed this task.
jptiz added subscribers: bu5hm4n, jptiz.
Herald closed this task as Invalid. · View Herald TranscriptJun 23 2020, 10:48 AM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added a subscriber: devilhorns. · View Herald Transcript

This ticket has been closed as spam because it lacks a description. If this ticket is not spam, please reopen it after adding a description.

jptiz renamed this task from Standardize external dependency variable names to Standardize external dependency handling.Jun 24 2020, 2:34 PM
jptiz reopened this task as Open.
jptiz updated the task description. (Show Details)
jptiz removed a project: Restricted Project.
jptiz updated the task description. (Show Details)
bu5hm4n added a comment.Jun 25 2020, 2:32 AM

First Half of the issue

I think you can just remove the _dep prefix from these 4 vars ... no big deal, there is really no thought behind that scheme, just make sure the names do not collide :)
  1. That topic is a bit more complex, some libs might be turned on sometimes, others sometimes might not. Hence different version checks. In general there is no real documentation onto what the minimal version is, some versions checks also have been dropped in the move from autotools to meson, I checked a few of them, and sometimes things did not compile, sometimes bugs or behavior changes in other deps caused bugs. In general I would say that its the best approch here to take the newest thing your distro / os brings, and check if it works (Sidenote: there are *a lot* of versions from harfbuzz where we are hitting bugs, so we need a higher version than that, a version check alone cannot check that easily ...)
  1. Having 2 places where we are defining a dependency mostly has the cause that we are using that dependency twice, in different libs, so this is mostly just fine ? Sometimes you can for example enable elput, but not efl-canvas-wl. I have to admit that i prefer to have things at a place where we are using them, and not shuffel the version checks we have over the tree. Using the same dependency multiple times is also not harmfull at all, meson will check the result of the first call for the second call, so the object returned will be the same, and will not be much more than a variable assignment (Btw. You can see that by (cached) in the meson log :))
jptiz triaged this task as Trivial priority.Jun 29 2020, 12:49 PM
jptiz added a comment.EditedJun 29 2020, 2:13 PM
In T8761#153094, @bu5hm4n wrote:

First Half of the issue

I think you can just remove the _dep prefix from these 4 vars ... no big deal, there is really no thought behind that scheme, just make sure the names do not collide :)

OK :D

  1. That topic is a bit more complex, some libs might be turned on sometimes, others sometimes might not. Hence different version checks. In general there is no real documentation onto what the minimal version is, some versions checks also have been dropped in the move from autotools to meson, I checked a few of them, and sometimes things did not compile, sometimes bugs or behavior changes in other deps caused bugs. In general I would say that its the best approch here to take the newest thing your distro / os brings, and check if it works

So, to see if I got it: in general, EFL doesn't constraint some dependencies' versions, but specific libs/modules need some deps to be from at least a specific minimum version, is it? Which would mean that what I said here:

Does ecore_wayland made to be specifically compatible with wayland-client >= 1.8.0 while ecore_wl2 and ecore_buffer can work with "any" installed version?

is true (replacing "made to be" with "happened to be" because of those bugs/issues you pointed out)?

(Sidenote: there are *a lot* of versions from harfbuzz where we are hitting bugs, so we need a higher version than that, a version check alone cannot check that easily ...)

Maybe we could blacklist some versions in that case and use it in version kwarg from dependency, but as I don't know how many versions create these bugs I'm afraid that it could make a really big piece of code just listing blacklisted versions (not a big issue to be honest, but it is noisy a lot).

  1. Having 2 places where we are defining a dependency mostly has the cause that we are using that dependency twice, in different libs, so this is mostly just fine ?

Yeah, indeed it is. Maybe I'm just overthinking about the problem of "fix a dependency declaration here but forget there". I started worrying about this specially after we started using meson subprojects, which lead to the zlib change in D12034 (declaring in one place so we could ensure we're getting the fallbacked dependency).

Sometimes you can for example enable elput, but not efl-canvas-wl.

I actually think I didn't get what you meant by that.

I have to admit that i prefer to have things at a place where we are using them, and not shuffel the version checks we have over the tree.

Yay

Using the same dependency multiple times is also not harmfull at all, meson will check the result of the first call for the second call, so the object returned will be the same, and will not be much more than a variable assignment (Btw. You can see that by (cached) in the meson log :))

Indeed. Yeah, I know about meson caching things (it even caches get_option and some lots of things), the thing was more related to the problem I stated now (two places to give maintance for the same thing).

jptiz added a comment.Jun 29 2020, 2:17 PM

My mind just messed up some phrases in the last comment. Edited to make more sense :^D.