| Message ID | 20220614210746.78911-1-akihiko.odaki@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | cutils: Introduce bundle mechanism | expand |
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
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
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