[Mesa-dev] [PATCH mesa 03/10] egl+glx: turn LIBGL_ALWAYS_SOFTWARE into a boolean

[Emil Velikov]

On 8 September 2017 at 18:36, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
> On Friday, 2017-09-08 08:13:54 -0700, Kenneth Graunke wrote:
>> On Friday, September 8, 2017 6:33:44 AM PDT Emil Velikov wrote:
>> > Hi Eric,
>> >
>> > On 8 September 2017 at 13:40, Eric Engestrom <eric.engestrom at imgtec.com> wrote:
>> > > Instead of setting based on set/unset, allow users to use boolean values.
>> > > In the docs, use `ALWAYS=true` instead of `ALWAYS=1` as it's clearer IMO.
>> > >
>> > I'm a bit weary about this series. In the past we had cases where
>> > people will set the variable to empty string and/or 0 and expect
>> > things to work.
>> >
>> > IIRC the older Intel QA team was using it, not sure about now. Either
>> > way it's great to confirm with a few people before pushing this.
>>
>> That team is gone.  I like the consistency of using env_var_as_boolean
>> across the codebase - it might break a few things, but I think that's
>> okay.  Most people don't use this, and most of those that do use =1.
>
> Agreed.
>
>>
>> > That aside, env_var_as_boolean() treats 'yes', '1' and 'true' in
>> > identical manner. Perhaps we can state what a "boolean variable" is
>> > and use that reference through the documentation?
>> >
>> > -Emil
>>
>> That seems like a good idea.
>
> Yes indeed, and I'll do that, but as a follow up patch.
Sure thing.

> I'm thinking of adding something like this at the top of envvar.html:
>
>         There are different kind of environment variables:
>           - BOOL: expects 1/true/yes or 0/false/no.
>           - INT: expects an integer value.
>           - ENUM: expects an string, but only a limited set of values is accepted.
>           - PATH: expects a file or folder path.
>           - NAME: expects a string name (like a driver name), but not a full path.
>           - NON-NULL: any value enables it (including empty string); use `unset` to disable.
>         Note: some variables expect more complicated values, like
>         MESA_EXTENSION_OVERRIDE.
>
> and then each env var in the doc would get an annotation:
>         LIBGL_ALWAYS_SOFTWARE (BOOL)
>         LIBGL_SHOW_FPS (INT)
This should really be a bool, but that's for another time.

>         EGL_LOG_LEVEL (ENUM: debug, info, warning, fatal)
>         EGL_DRIVERS_PATH (PATH)
>         EGL_DRIVER (PATH or NAME)
>         EGL_PLATFORM (NAME)
> ... etc.
>
> Hopefully, this should be clear enough. The last option (NON-NULL) would
> be applied to the old-style toggles.
>
Seems pretty clear over here. Although damn man... that's really nice
and comprehensive.

> For now, there seems to be another issue; I sent this series to travis,
> and EGL is failing to link [1] because every exported symbol in mesautil
> appears twice; some sort of double-link issue?
> I don't know if I'm tired or thick, but I can't figure it out :]
>
> [1] https://travis-ci.org/1ace/mesa/jobs/273302410#L6797
>
> I'd appreciate any help :)
>
I've spotted it - libEGL_common conditionally uses libmesautil.la (for
the wayland code).
I'd just make that unconditionally and omit it from libEGL/libEGL_mesa.

With the above fix, the patch is
Reviewed-by: Emil Velikov <emil.velikov at collabora.com>

-Emil