[Mesa-dev] [PATCH v2 07/24] mapi: move genCommon.py to src/mapi/new

[Emil Velikov]

On Fri, 14 Dec 2018 at 14:33, Erik Faye-Lund
<erik.faye-lund at collabora.com> wrote:
>
> On Fri, 2018-12-14 at 14:04 +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov at collabora.com>
> >
> > The helper will also be used by the new Khronos gl.xml aware
> > generator.
> >
> > v2: Move existing one, instead of duplicating it.
> >
> > Suggested-by: Kyle Brenneman <kbrenneman at nvidia.com>
> > Signed-off-by: Emil Velikov <emil.velikov at collabora.com>
> > ---
> >  src/egl/Makefile.am                         | 6 ++++--
> >  src/egl/generate/gen_egl_dispatch.py        | 3 +++
> >  src/egl/meson.build                         | 4 ++--
> >  src/mapi/Makefile.am                        | 1 +
> >  src/mapi/meson.build                        | 1 +
> >  src/{egl/generate => mapi/new}/genCommon.py | 0
> >  6 files changed, 11 insertions(+), 4 deletions(-)
> >  rename src/{egl/generate => mapi/new}/genCommon.py (100%)
> >
> > diff --git a/src/egl/Makefile.am b/src/egl/Makefile.am
> > index 7269912d96f..6ce51936b16 100644
> > --- a/src/egl/Makefile.am
> > +++ b/src/egl/Makefile.am
> > @@ -136,8 +136,10 @@ libEGL_common_la_LIBADD += \
> >       $(LIBDRM_LIBS) \
> >       $(CLOCK_LIB)
> >
> > -GLVND_GEN_DEPS = generate/gen_egl_dispatch.py \
> > -     generate/egl.xml generate/eglFunctionList.py
> > generate/genCommon.py \
> > +GLVND_GEN_DEPS = $(top_scrdir)/src/mapi/new/generate/genCommon.py \
> > +     generate/gen_egl_dispatch.py \
> > +     generate/eglFunctionList.py \
> > +     generate/egl.xml \
> >       generate/egl_other.xml
> >
> >  PYTHON_GEN = $(AM_V_GEN)$(PYTHON) $(PYTHON_FLAGS)
> > diff --git a/src/egl/generate/gen_egl_dispatch.py
> > b/src/egl/generate/gen_egl_dispatch.py
> > index eeb3f3f9a5a..728799e9754 100644
> > --- a/src/egl/generate/gen_egl_dispatch.py
> > +++ b/src/egl/generate/gen_egl_dispatch.py
> > @@ -38,6 +38,9 @@ import imp
> >  import sys
> >  import textwrap
> >
> > +import os
> > +NEWAPI = os.path.join(os.path.dirname(__file__), "..", "..", "mapi",
> > "new")
> > +sys.path.insert(0, NEWAPI)
> >  import genCommon
> >
> >  def main():
> > diff --git a/src/egl/meson.build b/src/egl/meson.build
> > index 89bac2cd633..33f24dec5e2 100644
> > --- a/src/egl/meson.build
> > +++ b/src/egl/meson.build
> > @@ -69,7 +69,7 @@ g_egldispatchstubs_c = custom_target(
> >    command : [
> >      prog_python, '@INPUT0@', 'source', '@INPUT1@', '@INPUT2@',
> > '@INPUT3@'
> >    ],
> > -  depend_files : files('generate/genCommon.py'),
> > +  depend_files : glapi_gen_mapi_py,
> >    capture : true,
> >  )
> >
> > @@ -83,7 +83,7 @@ g_egldispatchstubs_h = custom_target(
> >    command : [
> >      prog_python, '@INPUT0@', 'header', '@INPUT1@', '@INPUT2@',
> > '@INPUT3@'
> >    ],
> > -  depend_files : files('generate/genCommon.py'),
> > +  depend_files : glapi_gen_mapi_py,
> >    capture : true,
> >  )
> >
>
> I don't think I understand this change. Why does this script no longer
> depend on genCommon.py, just because it moved?
>
> I would expect something like this:
>
> ---8<---
> genCommon_py = files(join_paths('..', '..', 'mapi', 'new',
>                                 'genCommon.py'))
>
> <snip>
>
>   depend_files : [glapi_gen_mapi_py, genCommon_py],
> ---8<---
>
> (It's probably cleaner to define genCommon_py in some other meson-file
> closer to the source, but this should illustrate my point)
>
Thanks for the review Erik - you're a star.
I've addressed the above - v3 of this patch and 17/24 are out.

> > diff --git a/src/mapi/Makefile.am b/src/mapi/Makefile.am
> > index 97ebdeb1d7f..b46ed814700 100644
> > --- a/src/mapi/Makefile.am
> > +++ b/src/mapi/Makefile.am
> > @@ -31,6 +31,7 @@ pkgconfigdir = $(libdir)/pkgconfig
> >  pkgconfig_DATA =
> >
> >  EXTRA_DIST = \
> > +     new/genCommon.py \
> >       es1api/ABI-check \
> >       es2api/ABI-check \
> >       mapi_abi.py \
>
> If this is added to EXTRA_DIST, I kinda would have expected it to be
> removed from some EXTRA_DIST-variable in the old location... Why do we
> need to introduce distribution of this? Don't we just distribute the
> built API?
>
This is mostly for clarity sake.

Before this patch the script is used (by the makefile in the same
folder) and thus implicitly shipped. Now since is not used within this
file, it's not immediately clear that it will be shipped.
By default we want to ship the generators, for clarity and
reproducibility et al.

Autotools itself, tends to go the extra mile of shipping the generated
artefacts.
Idea is that one shouldn't need [too much] third party tools - ideally
only a compiler toolchain and dependencies (headers, libraries,
pkg-config files).

Whether autotools is going a wise thing (or not) is another topic.

Thanks
Emil