Re: [PATCH v3 4/4] net: convert to use qemu_find_file to locate bridge helper

[Akihiko Odaki]

On 2022/06/15 21:04, Daniel P. Berrangé wrote:
> On Wed, Jun 15, 2022 at 01:42:58PM +0200, Paolo Bonzini wrote:
> > On 6/15/22 12:52, Daniel P. Berrangé wrote:
> > > +    case QEMU_FILE_TYPE_HELPER:
> > > +        rel_install_dir = "";
> > > +        rel_build_dir = "";
> > > +        default_install_dir = default_helper_dir;
> > > +        break;
> > > +
> >
> > You're replacing ad hoc rules in Akihiko's meson.build with an ad hoc enum +
> > the corresponding "case"s here in qemu_find_file().  There is duplication
> > anyway, in this case between Meson and QEMU (plus QEMU needs to know about
> > two filesystem layouts).
>
> IMHO this is simpler to deal with than the meson additions, and also
> avoids the confusion of having files appearing in two places in the
> build dir.

Thanks to Paolo's suggestion to unify the common code to build the 
bundle tree, the required code for each bundled file is just a statement 
now (something like: bundles += { destination: source }) in the v6. 
Doing everything in Meson also allows to reuse the knowledge of the 
build tree Meson already has. I do no longer think my patch series are 
complicated more than yours. It even has less lines now.

There is still a room for improvements though. Particularly, the 
installing code and bundle-tree code are still duplicate. For example, 
pc-bios/meson.build now has the following code:
install_data(blobs, install_dir: qemu_datadir)

foreach f : blobs
  bundles += { qemu_datadir / f: meson.current_source_dir() / f }
endforeach

It would be nice if it can be written like:
foreach f : blobs
  bundle_data(qemu_datadir / f, f)
endforeach

Unfortunately Meson does not allow this.

Another problem is that the top-level meson.build is somewhat clutter. 
In my opinion, it is a persistent problem of the build system but I 
don't have a solution.

Anyway, I think my patch series is as close to the ideal as Meson 
currently allows.

The confusion caused by the files appearing in two places in the
build tree should be minimal. qemu-bundle is implemented entirely with 
symbolic links. If you know what is a symbolic link, you also know it is 
an alias and the files appear in different places, and I expect everyone 
hacking QEMU knows symbolic link.

> If we really want to have the build dir look just like the install
> dir though, why write custom meson commands per file type at all,
> instead add a rule that always invokes
>
>     DESTDIR=$(BUILDDIR)/vroot ninja install
>
> to populate a dir that's guaranteed identical to the install layout

Unfortunately Meson cannot define a rule which will be always invoked as 
far as I know.

Regards,
Akihiko Odaki

> Regards,
> Daniel