[Mesa-maintainers] [Mesa-dev] [PATCH 06/28] configure: rename --with-{egl-, }platforms

Tue Dec 13 18:19:07 UTC 2016

On 13 December 2016 at 18:06, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Tue, Dec 13, 2016 at 12:33 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>> On 13 December 2016 at 16:35, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>> On Tue, Dec 13, 2016 at 11:30 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>> On 13 December 2016 at 16:19, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>> On Tue, Dec 13, 2016 at 11:14 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>> On 13 December 2016 at 16:09, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>> On Tue, Dec 13, 2016 at 11:08 AM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>>> On 13 December 2016 at 15:25, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>>>> On Thu, Dec 8, 2016 at 2:21 PM, Emil Velikov <emil.l.velikov at gmail.com> wrote:
>>>>>>>>>> From: Emil Velikov <emil.velikov at collabora.com>
>>>>>>>>>>
>>>>>>>>>> Since day 1, Vulkan has depended on --with-egl-platforms to select the
>>>>>>>>>> platforms build.
>>>>>>>>>>
>>>>>>>>>> With earlier commits, we've attributed for that internally by renaming
>>>>>>>>>> the [internal] conditionals in our build. At the same time having the
>>>>>>>>>> --enable-egl and --with-egl-platforms dependency for Vulkan makes no
>>>>>>>>>> sense.
>>>>>>>>>>
>>>>>>>>>> Since where' on the bridge we want to make the conditional generic, thus
>>>>>>>>>> others (such as the gallium VL targets) can honour the respective
>>>>>>>>>> platforms.
>>>>>>>>>>
>>>>>>>>>> Cc: mesa-maintainers at lists.freedesktop.org
>>>>>>>>>> Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
>>>>>>>>>> ---
>>>>>>>>>> Should we keep the old configure and error out when anyone provides a
>>>>>>>>>> non-default value ? This way they'll have direct feedback how to move
>>>>>>>>>> forward. Otherwise they'll likely miss the warning message.
>>>>>>>>>
>>>>>>>>> I can't tell what problem this is solving. OTOH this is creating the
>>>>>>>>> problem that everyone's configure lines will need fixing.
>>>>>>>> This need fixing (builder intervention) either way, I'm afraid. Question is
>>>>>>>>  a) should we expect people to read the release notes, or
>>>>>>>>  b) get their attention with an error message, providing an example
>>>>>>>> in order to do the right thing.
>>>>>>>
>>>>>>> (c) leave things alone if this isn't fixing anything other than aesthetics.
>>>>>> It's not aesthetics... atm if you want just Vulkan you still need
>>>>>> --enable-egl --with-egl-platforms=x11,wayland*.
>>>>>> Otherwise it won't have any winsys integration... Not sure if it'll
>>>>>> build/link properly even :-(
>>>>>
>>>>> So error the build and say you don't support that configuration if you
>>>>> enable vulkan and don't enable any winsys stuff. Or make it so that
>>>>> --enable-egl sets the default value of --with-egl-platforms to
>>>>> whatever. Don't break everyone's builds, bisects, etc.
>>>>>
>>>> Sounds like you've not looked at the lovely stuff we have in there. I
>>>> welcome you to do so.
>>>>
>>>> That aside:
>>>> Your suggestions contradict each other - on one hand you're saying
>>>> "error out" in a what is buggy yet valid usecase. On the other hand
>>>> you're against AC_MSG_ERROR since it will break X/Y usecase.
>>>
>>> I thought you said it didn't currently work. If it doesn't work, or is
>>> unsupportable, error out. If it currently works, and is supportable -
>>> keep it working.
>>>
>> If it works is by sheer magic. Personally I would stay away from
>> calling it "supported".
>> Furthermore close to nobody has any knowledge of the
>>
>>>>
>>>> Also "everyone" is a mild overstatement, but I guess we all know that ;-)
>>>
>>> Perhaps I'm misunderstanding. This will break everyone that has
>>> --with-egl-platforms in their configure line, no? If not, disregard my
>>> comments.
>>>
>>> I imagine that nearly everyone that does development on mesa has that
>>> flag in their configure line. That means any bisect will require
>>> changing the configure line as you cross this commit on the bisect.
>>> This is very annoying. Sometimes it's required, but it shouldn't be
>>> done lightly.
>>>
>> Agreed we shouldn't do these lightly and as mentioned a few times
>> months ago "if we have a flag day we can fix the world" sadly we
>> cannot.
>>
>>> So I'll ask again because it's still unclear to me - what *problem* is
>>> this fixing? I.e. what currently doesn't work that you are trying to
>>> make work?
>>>
>> Ok let me try again, starting with the mandatory backstory:
>> [Note: a fair bit of this is in the cover letter... still]
>>
>> Currently there's no clear way to select which platform you want to
>> use for Vulkan - one "inherits" it from the EGL platform selection.
>> At the same time you do _not_ need EGL to have Vulkan. Thus as you
>> disable EGL (and therefore its platform selection) you'll end up with
>> Vulkan driver(s) without _any_ winsys support - not something that's
>> intuitive and/or you'd would want most of the time.
>>
>> Side note: Vulkan requires DRI3 with X11, yet it's missing the check
>> or any of the dependency tracking/management. Thus it fails at build
>> or link time - depending on how lucky you get.
>> This has been fixed as standalone patch(es) which should be ok for stable.
>>
>> Now combine that with the fact that we want a similar logic for the VL
>> targets (omx, vaapi and maybe vdpau/xvmc/other one day) you'll end up
>> with the following options
>>  a) provide a --with-foo-platforms=...
>> Here we'll end up with [even greather] combinatoric explosion,
>> hundreds of lines of build glue and nearly impossible to test/ensure
>> that mesa isn't constantly broken.
>> There is the part of "building EGL w/o wayland while Vulkan with
>> wayland" is the style of thing that we do _not_ support [for example
>> gallium drivers building/using graw/gallivm], nor something people
>> [normally] go for.
>>
>>  b) or, have a single --with-platforms=
>> Giving us a uniform way to control the winsys/platforms. It is _very_
>> annoying about the rename, but it handles a lot more nuisances in a
>> brief and easy to understand manner.
>> I doubt many builders will be excited about reading and managing
>> another 3-4 new configure toggles...
>>
>> A workaround which just make to mind:
>>  - keep --with-egl-platforms around and it's heuristics (which will
>> also be applied for --with-platforms).
>>  - keep the --with-egl-platforms decisions for EGL only, and let one
>> override it via --with-platforms. Warn as the latter happens.
>>  - warn for the deprecation of --with-egl-platforms
>
> And really --with-platforms is --with-egl-platforms +
> --with-vulkan-platforms (which I realize doesn't presently exist).
>
> And the reason you don't want to add it is that you're afraid someone
> will do something like
>
> --with-egl-platforms=drm --with-vulkan-platforms=x11
>
> and it's a pain to make it so that EGL only has drm and vulkan only
> has x11? And eventually one might also want --with-vdpau-platforms,
> --with-vaapi-platforms, --with-whoknowswhat-platforms, and you don't
> want to keep adding them, since the chances that you *really* want
> those to be different for legitimate reasons is ... low.
>
> Is that right?
>
Precisely.

If there is a legitimate reason, one can do a second mesa build... as
we do for DRI vs XLIB powered libGL, classic vs gallium OSMesa, etc.
Afacit having things configurable at such state is not supported by
either of the three build systems nor something that will scale - be
that from code or test POV.

Thanks
Emil