[PATCH weston 1/4] tests: Don't rely on build directory layout

[Emil Velikov]

On 20 June 2018 at 11:51, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Tue, 19 Jun 2018 18:30:13 +0100
> Emil Velikov <emil.l.velikov at gmail.com> wrote:
>
>> Hi Pekka,
>>
>> On 18 June 2018 at 15:40, Pekka Paalanen <ppaalanen at gmail.com> wrote:
>> > From: Daniel Stone <daniels at collabora.com>
>> >
>> > 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 = &mapping[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.velikov at collabora.com>

-Emil