Re: [Qemu-devel] [PATCH v4 01/12] ui: add keycodemapdb repository as a GIT submodule

[Daniel P. Berrange]

On Tue, Aug 15, 2017 at 06:47:22PM +0800, Fam Zheng wrote:
> On Tue, 08/15 11:04, Daniel P. Berrange wrote:
> > On Tue, Aug 15, 2017 at 10:36:04AM +0100, Daniel P. Berrange wrote:
> > > The https://gitlab.com/keycodemap/keycodemapdb/ repo contains a
> > > data file mapping between all the different scancode/keycode/keysym
> > > sets that are known, and a tool to auto-generate lookup tables for
> > > different combinations.
> > > 
> > > It is used by GTK-VNC, SPICE-GTK and libvirt for mapping keys.
> > > Using it in QEMU will let us replace many hand written lookup
> > > tables with auto-generated tables from a master data source,
> > > reducing bugs. Adding new QKeyCodes will now only require the
> > > master table to be updated, all ~20 other tables will be
> > > automatically updated to follow.
> > > 
> > > Signed-off-by: Daniel P. Berrange <address@hidden>
> > > ---
> > >  .gitignore                    |  2 ++
> > >  .gitmodules                   |  3 +++
> > >  configure                     |  2 ++
> > >  tests/docker/Makefile.include | 11 +++++++----
> > >  tests/docker/run              |  4 +++-
> > >  ui/Makefile.objs              | 18 ++++++++++++++++++
> > >  ui/keycodemapdb               |  1 +
> > >  7 files changed, 36 insertions(+), 5 deletions(-)
> > >  create mode 160000 ui/keycodemapdb
> > > 
> > 
> > > diff --git a/configure b/configure
> > > index dd73cce62f..bd373ec2b4 100755
> > > --- a/configure
> > > +++ b/configure
> > > @@ -5258,6 +5258,8 @@ echo_version() {
> > >      fi
> > >  }
> > >  
> > > +(cd $source_path && git submodule update --init ui/keycodemapdb)
> > > +
> > 
> > Urgh, no, this won't work because of course you don't have to
> > have a git checkout when running configure.
> > 
> > Any suggestions on the "best" way to ensure that the ui/keycodemapdb
> > git submodule is always checked out, without requiring developers to
> > do something manually ?
> > 
> > I could try
> > 
> >   (cd $source_path && test -d .git && git submodule update --init 
> > ui/keycodemapdb)
> > 
> > but wonder if there's a better way ?
> 
> I think the way dtc handles this is okay: probe headers/libs, if failed, hint
> the "git submodule update" command in the error message.  This is also a
> familiar/friendly user interface to developers.

I don't think that's acceptable in this case. Few people will ever need
to setup the dtc submodule as distros commonly have that available.

For the keycodemapdb *EVERY* single person who builds QEMU from git will
need it, and they'll also need to update it periodically when the submodule
hash changes. So we need to have it automated IMHO.

> (If you looks at the test snippet that patchew runs, there is an explicit
> submodule command:
> 
>     #!/bin/bash
>     set -e
>     git submodule update --init dtc
>     # Let docker tests dump environment info
>     export SHOW_ENV=1
>     export J=8
>     time make address@hidden
>     time make address@hidden
>     time make address@hidden
> 
> as the libfdt devel package is not available in every docker images.)

IMHO that is a bad approach because someone typing
'make address@hidden' is  going to be missing the dtc
module. You'll see in this case I extended the docker/Makefile
to always pull in keycodemapdb, not rely on someone remembering
todo it manually.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|