Developers often run QEMU without installing. The bundle mechanism
allows to look up files which should be present in installation even in
such a situation.

It is a general mechanism and can find any files located relative
to the installation tree. The build tree must have a new directory,
qemu-bundle, to represent what files the installation tree would
have for reference by the executables.

v4:
* Add Daniel P. Berrangé to CC. Hopefully this helps merging his patch:
  https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg02276.html
* Rebased to the latest QEMU.

v3:
* Note that the bundle mechanism is for any files located relative to the
  installation tree including but not limited to datadir. (Peter Maydell)
* Fix "bridge" typo (Philippe Mathieu-Daudé)

v2: Rebased to the latest QEMU.

Akihiko Odaki (4):
  cutils: Introduce bundle mechanism
  datadir: Use bundle mechanism
  ui/icons: Use bundle mechanism
  net: Use bundle mechanism

 .travis.yml                 |  2 +-
 include/net/net.h           |  2 +-
 include/qemu/cutils.h       | 19 +++++++++++++++++++
 meson.build                 | 13 ++++++++++---
 net/tap.c                   |  6 +++++-
 pc-bios/keymaps/meson.build |  3 +++
 pc-bios/meson.build         | 17 +++++++++--------
 qemu-options.hx             |  4 ++--
 scripts/oss-fuzz/build.sh   |  2 +-
 softmmu/datadir.c           | 35 ++++++++++++-----------------------
 tests/qtest/fuzz/fuzz.c     | 15 ---------------
 tests/vm/fedora             |  2 +-
 tests/vm/freebsd            |  2 +-
 tests/vm/netbsd             |  2 +-
 tests/vm/openbsd            |  2 +-
 ui/cocoa.m                  | 29 ++++++++++++++++-------------
 ui/gtk.c                    |  6 +++++-
 ui/icons/meson.build        | 36 ++++++++++++++++++++++++++++--------
 ui/sdl2.c                   | 18 +++++++++++-------
 util/cutils.c               | 33 +++++++++++++++++++++++++++++++++
 20 files changed, 160 insertions(+), 88 deletions(-)

-- 
2.32.1 (Apple Git-133)

On 6/14/22 23:07, Akihiko Odaki wrote:
> Developers often run QEMU without installing. The bundle mechanism
> allows to look up files which should be present in installation even in
> such a situation.
> 
> It is a general mechanism and can find any files located relative
> to the installation tree. The build tree must have a new directory,
> qemu-bundle, to represent what files the installation tree would
> have for reference by the executables.
> 
> v4:
> * Add Daniel P. Berrangé to CC. Hopefully this helps merging his patch:
>    https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg02276.html
> * Rebased to the latest QEMU.
> 
> v3:
> * Note that the bundle mechanism is for any files located relative to the
>    installation tree including but not limited to datadir. (Peter Maydell)
> * Fix "bridge" typo (Philippe Mathieu-Daudé)
> 
> v2: Rebased to the latest QEMU.

I like the idea, but I have a couple issues with the implementation:

- at the meson level, there is some repetition of mkdir and ln 
run_commands.  Perhaps you could just fill in a dictionary, and then do 
something like

   created_paths = {}
   foreach source, dest: var
     path = fs.parent(qemu_bundledir / dest)
     created_paths += {path: true}
   endforeach
   run_command('mkdir', '-p', created_paths.keys())
   foreach source, dest: var
     run_command('ln', '-sf', meson.project_source_root() / source,
                 qemu_bundledir / dest)
   endforeach

at the end of the toplevel meson.build.

- at the code level, it seems to me that this could reuse a lot of the 
logic of get_relocated_path().  In particular, I would include $prefix 
in the qemu_bundledir, so that the files in the bundle directory would 
look like qemu-bundle/usr/share/qemu/bios.bin: just like an install that 
uses DESTDIR.  Then, if an uninstalled QEMU somehow returns 
$exec_path/qemu-bundle/$prefix/$bindir for qemu_get_exec_dir() instead 
of $exec_path, then get_relocated_path() will automatically return the 
correct paths from qemu-bundle/.

Thanks,

Paolo

On Wed, Jun 15, 2022 at 10:39:29AM +0200, Paolo Bonzini wrote:
> On 6/14/22 23:07, Akihiko Odaki wrote:
> > Developers often run QEMU without installing. The bundle mechanism
> > allows to look up files which should be present in installation even in
> > such a situation.
> > 
> > It is a general mechanism and can find any files located relative
> > to the installation tree. The build tree must have a new directory,
> > qemu-bundle, to represent what files the installation tree would
> > have for reference by the executables.
> > 
> > v4:
> > * Add Daniel P. Berrangé to CC. Hopefully this helps merging his patch:
> >    https://mail.gnu.org/archive/html/qemu-devel/2022-06/msg02276.html
> > * Rebased to the latest QEMU.
> > 
> > v3:
> > * Note that the bundle mechanism is for any files located relative to the
> >    installation tree including but not limited to datadir. (Peter Maydell)
> > * Fix "bridge" typo (Philippe Mathieu-Daudé)
> > 
> > v2: Rebased to the latest QEMU.
> 
> I like the idea, but I have a couple issues with the implementation:
> 
> - at the meson level, there is some repetition of mkdir and ln run_commands.
> Perhaps you could just fill in a dictionary, and then do something like
> 
>   created_paths = {}
>   foreach source, dest: var
>     path = fs.parent(qemu_bundledir / dest)
>     created_paths += {path: true}
>   endforeach
>   run_command('mkdir', '-p', created_paths.keys())
>   foreach source, dest: var
>     run_command('ln', '-sf', meson.project_source_root() / source,
>                 qemu_bundledir / dest)
>   endforeach

Per my other reply, IMHO, all the meson changes are redundant.

I've just sent a series that illustrates how we can improve the
qemu_find_file method so it correctly copes with install dir
vs build dir being different layouts, and be extensible to
any types of file (bios, keymaps, icons, helper exes, and
more).

With 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 :|

On Wed, Jun 15, 2022 at 06:07:42AM +0900, Akihiko Odaki wrote:
> Developers often run QEMU without installing. The bundle mechanism
> allows to look up files which should be present in installation even in
> such a situation.
> 
> It is a general mechanism and can find any files located relative
> to the installation tree. The build tree must have a new directory,
> qemu-bundle, to represent what files the installation tree would
> have for reference by the executables.

I don't think this is an attractive approach to the problem,
because it results in us adding a bunch of meson rules to
simulate 'make install' within the build dir. This is undesirable
clutter IMHO, and can be solved more simply by just modifying the
qemu_find_file() method.

The core problem is the impl of qemu_find_file is taking the wrong
approach, in several ways, but mostly because of its use of a single
'data_dirs' array for all types of file. This is bad because it
has the assumption that build dir and install dir layouts match,
and second because when we add extra firmware data dirs, we don't
want this used for non-firmware files.

We need to separate out the handling of different types of resources
for this to work correctly.

With 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 :|

On 6/15/22 10:30, Daniel P. Berrangé wrote:
> I don't think this is an attractive approach to the problem,
> because it results in us adding a bunch of meson rules to
> simulate 'make install' within the build dir. This is undesirable
> clutter IMHO, and can be solved more simply by just modifying the
> qemu_find_file() method.
> 
> The core problem is the impl of qemu_find_file is taking the wrong
> approach, in several ways, but mostly because of its use of a single
> 'data_dirs' array for all types of file. This is bad because it
> has the assumption that build dir and install dir layouts match,
> and second because when we add extra firmware data dirs, we don't
> want this used for non-firmware files.
> 
> We need to separate out the handling of different types of resources
> for this to work correctly.

In some sense this is what Akihiko did - instead of separating them in 
qemu_find_file(), the "pre-install" layout separates them in the 
filesystem.  While I had remarks on the implementation I think it's a 
sensible approach.

The pre-install directory could even be created as a custom_target, 
using the JSON files from Meson introspection.

Paolo

On Wed, Jun 15, 2022 at 01:02:08PM +0200, Paolo Bonzini wrote:
> On 6/15/22 10:30, Daniel P. Berrangé wrote:
> > I don't think this is an attractive approach to the problem,
> > because it results in us adding a bunch of meson rules to
> > simulate 'make install' within the build dir. This is undesirable
> > clutter IMHO, and can be solved more simply by just modifying the
> > qemu_find_file() method.
> > 
> > The core problem is the impl of qemu_find_file is taking the wrong
> > approach, in several ways, but mostly because of its use of a single
> > 'data_dirs' array for all types of file. This is bad because it
> > has the assumption that build dir and install dir layouts match,
> > and second because when we add extra firmware data dirs, we don't
> > want this used for non-firmware files.
> > 
> > We need to separate out the handling of different types of resources
> > for this to work correctly.
> 
> In some sense this is what Akihiko did - instead of separating them in
> qemu_find_file(), the "pre-install" layout separates them in the filesystem.
> While I had remarks on the implementation I think it's a sensible approach.
> 
> The pre-install directory could even be created as a custom_target, using
> the JSON files from Meson introspection.

Doing that is more complicated than just refactoring qemu_find_file,
such that its search locations can be tailored per file type, just
by setting a couple variables in the code IMHO.

With 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 :|