Re: [PATCH weston 1/4] tests: Don't rely on build directory layout
On 20 June 2018 at 11:51, Pekka Paalanen wrote:
> On Tue, 19 Jun 2018 18:30:13 +0100
> Emil Velikov wrote:
>
>> Hi Pekka,
>>
>> On 18 June 2018 at 15:40, Pekka Paalanen wrote:
>> > From: Daniel Stone
>> >
>> > Rather than having a hardcoded dependency on the build-directory layout,
>> > use an explicit module-map environment variable, which rewrites requests
>> > for modules and helper/libexec binaries to specific paths.
>> >
>> > This will help with migration to Meson where setting up the paths
>> > according to autotools would be painful and unnecessary.
>> >
>> Surely one didn't need a build system, to make nice improvements.
>
> The driving force behind a change should be stated in the commit
> message. Granted, this paragraph was added by me, Daniel wrote only the
> first paragraph. I didn't see the first paragraph as sufficient
> rationale. The patch also allows loading external backends which we
> specifically do not want to support at the time.
>
Right, this extra information seems a lot better selling point than
build system A/B.
>> I guess, sometimes, tools do need to give us a wake-up call.
>>
>>
>> > +WL_EXPORT size_t
>> > +weston_module_path_from_env(const char *name, char *path, size_t path_len)
>> > +{
>> > + const char *mapping = getenv("WESTON_MODULE_MAP");
>> > + const char *end;
>> > +
>> strlen(name) once and use it throughout?
>>
>
> Might as well, since 'name' doesn't change.
>
>>
>> > + if (!mapping)
>> > + return 0;
>> > +
>> > + end = mapping + strlen(mapping);
>> > + while (mapping < end && *mapping) {
>> > + const char *filename, *next;
>> > +
>> > + /* early out: impossibly short string */
>> > + if ((size_t)(end - mapping) < strlen(name) + 1)
>> > + return 0;
>> > +
>> > + filename = [strlen(name) + 1];
>> > + next = strchrnul(mapping, ';');
>> > +
>> > + if (strncmp(mapping, name, strlen(name)) == 0 &&
>> > + mapping[strlen(name)] == '=') {
>> > + size_t file_len = next - filename; /* no trailing
>> > NUL */
>> > + if (file_len >= path_len)
>> > + return 0;
>> > + strncpy(path, filename, file_len);
>> > + path[file_len] = '\0';
>> A simple stat() can, more or less, ensure that we're not giving random
>> rubbish to the caller.
>> Worth it, or more of an overkill?
>
> If a mapping exists, we need to return the mapped-to path. If we return
> nothing, then the caller will use the default path and a broken module
> map could go unnoticed if there actually is something in the default
> path.
>
Hmm good point - shout early and loud is a good thing to have.
The transient fallbacks tend to do more harm than good.
With the "allow external modules" in the commit message + const
[int,ssize_t] foo = strlen(name), the patch is
Reviewed-by: Emil Velikov
-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] tests: Don't rely on build directory layout
On Tue, 19 Jun 2018 18:30:13 +0100
Emil Velikov wrote:
> Hi Pekka,
>
> On 18 June 2018 at 15:40, Pekka Paalanen wrote:
> > From: Daniel Stone
> >
> > Rather than having a hardcoded dependency on the build-directory layout,
> > use an explicit module-map environment variable, which rewrites requests
> > for modules and helper/libexec binaries to specific paths.
> >
> > This will help with migration to Meson where setting up the paths
> > according to autotools would be painful and unnecessary.
> >
> Surely one didn't need a build system, to make nice improvements.
The driving force behind a change should be stated in the commit
message. Granted, this paragraph was added by me, Daniel wrote only the
first paragraph. I didn't see the first paragraph as sufficient
rationale. The patch also allows loading external backends which we
specifically do not want to support at the time.
> I guess, sometimes, tools do need to give us a wake-up call.
>
>
> > +WL_EXPORT size_t
> > +weston_module_path_from_env(const char *name, char *path, size_t path_len)
> > +{
> > + const char *mapping = getenv("WESTON_MODULE_MAP");
> > + const char *end;
> > +
> strlen(name) once and use it throughout?
>
Might as well, since 'name' doesn't change.
>
> > + if (!mapping)
> > + return 0;
> > +
> > + end = mapping + strlen(mapping);
> > + while (mapping < end && *mapping) {
> > + const char *filename, *next;
> > +
> > + /* early out: impossibly short string */
> > + if ((size_t)(end - mapping) < strlen(name) + 1)
> > + return 0;
> > +
> > + filename = [strlen(name) + 1];
> > + next = strchrnul(mapping, ';');
> > +
> > + if (strncmp(mapping, name, strlen(name)) == 0 &&
> > + mapping[strlen(name)] == '=') {
> > + size_t file_len = next - filename; /* no trailing
> > NUL */
> > + if (file_len >= path_len)
> > + return 0;
> > + strncpy(path, filename, file_len);
> > + path[file_len] = '\0';
> A simple stat() can, more or less, ensure that we're not giving random
> rubbish to the caller.
> Worth it, or more of an overkill?
If a mapping exists, we need to return the mapped-to path. If we return
nothing, then the caller will use the default path and a broken module
map could go unnoticed if there actually is something in the default
path.
Thanks,
pq
pgpviFqZ5hZpz.pgp
Description: OpenPGP digital signature
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
Re: [PATCH weston 1/4] tests: Don't rely on build directory layout
Hi Pekka,
On 18 June 2018 at 15:40, Pekka Paalanen wrote:
> From: Daniel Stone
>
> Rather than having a hardcoded dependency on the build-directory layout,
> use an explicit module-map environment variable, which rewrites requests
> for modules and helper/libexec binaries to specific paths.
>
> This will help with migration to Meson where setting up the paths
> according to autotools would be painful and unnecessary.
>
Surely one didn't need a build system, to make nice improvements.
I guess, sometimes, tools do need to give us a wake-up call.
> +WL_EXPORT size_t
> +weston_module_path_from_env(const char *name, char *path, size_t path_len)
> +{
> + const char *mapping = getenv("WESTON_MODULE_MAP");
> + const char *end;
> +
strlen(name) once and use it throughout?
> + if (!mapping)
> + return 0;
> +
> + end = mapping + strlen(mapping);
> + while (mapping < end && *mapping) {
> + const char *filename, *next;
> +
> + /* early out: impossibly short string */
> + if ((size_t)(end - mapping) < strlen(name) + 1)
> + return 0;
> +
> + filename = [strlen(name) + 1];
> + next = strchrnul(mapping, ';');
> +
> + if (strncmp(mapping, name, strlen(name)) == 0 &&
> + mapping[strlen(name)] == '=') {
> + size_t file_len = next - filename; /* no trailing NUL
> */
> + if (file_len >= path_len)
> + return 0;
> + strncpy(path, filename, file_len);
> + path[file_len] = '\0';
A simple stat() can, more or less, ensure that we're not giving random
rubbish to the caller.
Worth it, or more of an overkill?
-Emil
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/wayland-devel
[PATCH weston 1/4] tests: Don't rely on build directory layout
From: Daniel Stone
Rather than having a hardcoded dependency on the build-directory layout,
use an explicit module-map environment variable, which rewrites requests
for modules and helper/libexec binaries to specific paths.
This will help with migration to Meson where setting up the paths
according to autotools would be painful and unnecessary.
Signed-off-by: Daniel Stone
v2:
Fixed ivi_layout-test-plugin.c:wet_module_init().
Do not change the lookup name of ivi-layout.ivi.
Improved documentation of weston_module_path_from_env() and made it cope
with map strings that a) do not end with a semicolon, and b) have
multiple consecutive semicolons.
Let WESTON_MODULE_MAP be printed into the test log so that it is easier
to run tests manually.
Signed-off-by: Pekka Paalanen
---
compositor/main.c | 26 +-
compositor/text-backend.c | 6 +
compositor/weston-screenshooter.c | 8 +++---
compositor/weston.h | 3 +++
desktop-shell/shell.c | 9 ++-
libweston/compositor.c| 57 +++
libweston/compositor.h| 3 +++
tests/ivi_layout-test-plugin.c| 16 +--
tests/weston-tests-env| 17
9 files changed, 108 insertions(+), 37 deletions(-)
diff --git a/compositor/main.c b/compositor/main.c
index 86e3782f..a801be87 100644
--- a/compositor/main.c
+++ b/compositor/main.c
@@ -652,7 +652,6 @@ weston_create_listening_socket(struct wl_display *display,
const char *socket_na
WL_EXPORT void *
wet_load_module_entrypoint(const char *name, const char *entrypoint)
{
- const char *builddir = getenv("WESTON_BUILD_DIR");
char path[PATH_MAX];
void *module, *init;
size_t len;
@@ -661,10 +660,8 @@ wet_load_module_entrypoint(const char *name, const char
*entrypoint)
return NULL;
if (name[0] != '/') {
- if (builddir)
- len = snprintf(path, sizeof path, "%s/.libs/%s",
builddir,
- name);
- else
+ len = weston_module_path_from_env(name, path, sizeof path);
+ if (len == 0)
len = snprintf(path, sizeof path, "%s/%s", MODULEDIR,
name);
} else {
@@ -701,7 +698,6 @@ wet_load_module_entrypoint(const char *name, const char
*entrypoint)
return init;
}
-
WL_EXPORT int
wet_load_module(struct weston_compositor *compositor,
const char *name, int *argc, char *argv[])
@@ -732,6 +728,24 @@ wet_load_shell(struct weston_compositor *compositor,
return 0;
}
+WL_EXPORT char *
+wet_get_binary_path(const char *name)
+{
+ char path[PATH_MAX];
+ size_t len;
+
+ len = weston_module_path_from_env(name, path, sizeof path);
+ if (len > 0)
+ return strdup(path);
+
+ len = snprintf(path, sizeof path, "%s/%s",
+ weston_config_get_libexec_dir(), name);
+ if (len >= sizeof path)
+ return NULL;
+
+ return strdup(path);
+}
+
static int
load_modules(struct weston_compositor *ec, const char *modules,
int *argc, char *argv[], int32_t *xwayland)
diff --git a/compositor/text-backend.c b/compositor/text-backend.c
index 4d8c085b..54242427 100644
--- a/compositor/text-backend.c
+++ b/compositor/text-backend.c
@@ -1047,14 +1047,10 @@ text_backend_configuration(struct text_backend
*text_backend)
struct weston_config *config = wet_get_config(text_backend->compositor);
struct weston_config_section *section;
char *client;
- int ret;
section = weston_config_get_section(config,
"input-method", NULL, NULL);
- ret = asprintf(, "%s/weston-keyboard",
- weston_config_get_libexec_dir());
- if (ret < 0)
- client = NULL;
+ client = wet_get_binary_path("weston-keyboard");
weston_config_section_get_string(section, "path",
_backend->input_method.path,
client);
diff --git a/compositor/weston-screenshooter.c
b/compositor/weston-screenshooter.c
index 70afed4a..0994cb4f 100644
--- a/compositor/weston-screenshooter.c
+++ b/compositor/weston-screenshooter.c
@@ -117,12 +117,10 @@ screenshooter_binding(struct weston_keyboard *keyboard,
{
struct screenshooter *shooter = data;
char *screenshooter_exe;
- int ret;
- ret = asprintf(_exe, "%s/%s",
- weston_config_get_libexec_dir(),
- "/weston-screenshooter");
- if (ret < 0) {
+
+ screenshooter_exe = wet_get_binary_path("weston-screenshooter");
+ if (!screenshooter_exe) {
weston_log("Could not construct screenshooter path.\n");
return;
}
diff --git
