Re: [PATCH] Update scripts/meson-buildoptions.sh
On 1/7/23 10:02, Paolo Bonzini wrote: On 1/3/23 20:31, Stefan Hajnoczi wrote: The other problem with this file is that it appears to be generated differently depending on the host distro (specifically the default value for the --libdir option). That also would seem to nudge towards "don't commit a generated file". I wasn't aware of the libdir default that Peter mentioned, but the same issue would happen for release tarballs so "do not commit it" would have to be extended to "do not ship it", that is do everything in Python. This in turn goes back to the reason for the buildoptions.sh approach: the path to the Python binary is not known when "configure --help" prints the help message. If the user does not have a python3 or meson binary in the path, requiring "configure --meson=... --help" or "configure --python=... --help" is not hideous I guess, but not pretty either. I think an extremely early error message for missing python (and pointer to --python) is perfectly reasonable. I imagine it would be very rare, a case of forgetting to install all of the build dependencies into a fresh container. r~
Re: [PATCH] Update scripts/meson-buildoptions.sh
On 1/3/23 20:31, Stefan Hajnoczi wrote: The other problem with this file is that it appears to be generated differently depending on the host distro (specifically the default value for the --libdir option). That also would seem to nudge towards "don't commit a generated file". I wasn't aware of the libdir default that Peter mentioned, but the same issue would happen for release tarballs so "do not commit it" would have to be extended to "do not ship it", that is do everything in Python. This in turn goes back to the reason for the buildoptions.sh approach: the path to the Python binary is not known when "configure --help" prints the help message. If the user does not have a python3 or meson binary in the path, requiring "configure --meson=... --help" or "configure --python=... --help" is not hideous I guess, but not pretty either. Paolo: Is the meson-buildoptions.sh approach a temporary solution or something long-term? Maybe everything can be migrated to meson eventually so that ./configure and meson-buildoptions.sh are no longer necessary? It is long-term. Meson is only used to build the emulators and that part will move entirely out of configure soon, but other parts of the build (especially tests/tcg and firmware, which need to build docker containers for cross-compilation) are separate and there's no plan to use anything but configure/Makefile for the overall orchestration. While I have noticed stale meson-buildoptions.sh it's never happened to me. I ascribed it to someone not noticing the dirty file rather than a bug; it should be possible to add a test to CI that catches it. Paolo
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Mon, Jan 02, 2023 at 11:41:13AM +0100, Alessandro Di Federico wrote: > Note: `Makefile` relies on modification dates in the source tree to > detect changes to `meson_options.txt`. However, git does not track > those. Therefore, the following was necessary to regenerate > `meson-buildoptions.sh`: > > touch meson_options.txt > cd "$BUILD_DIR" > make update-buildoptions > > Signed-off-by: Alessandro Di Federico > --- > scripts/meson-buildoptions.sh | 18 -- > 1 file changed, 12 insertions(+), 6 deletions(-) The discussion we're having is independent of this patch: Reviewed-by: Stefan Hajnoczi signature.asc Description: PGP signature
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Tue, 3 Jan 2023 at 12:31, Peter Maydell wrote: > > On Tue, 3 Jan 2023 at 16:12, Alessandro Di Federico wrote: > > > > On Tue, 3 Jan 2023 10:51:36 -0500 > > Stefan Hajnoczi wrote: > > > > > QEMU's Makefile used to a use a technique where it generated > > > "timestamp" files and used cmp(1) to check if rebuilding was > > > necessary: > > > 1. Always generate meson-buildoptions.sh-timestamp. > > > > `meson-buildoptions.sh-timestamp` would be the full expected output, > > right? It's not just a date or something. > > AFAIU that would make sure that if nothing changed in the output you > > don't trigger other targets depending on `meson-buildoptions.sh`. It's > > a solution for a different problem. > > > > The problem with always rebuilding `meson-buildoptions.sh` is that we > > spend 1 extra second on every build, even those that doesn't need to > > rebuild anything else. > > Not unacceptable, but I think we should strive not to commit generated > > files and move the file to the build directory, unless there's a reason > > why this is not viable that I'm not seeing. > > The other problem with this file is that it appears to > be generated differently depending on the host distro > (specifically the default value for the --libdir option). > That also would seem to nudge towards "don't commit a > generated file". Paolo: Is the meson-buildoptions.sh approach a temporary solution or something long-term? Maybe everything can be migrated to meson eventually so that ./configure and meson-buildoptions.sh are no longer necessary? Stefan
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Tue, 3 Jan 2023 at 16:12, Alessandro Di Federico wrote: > > On Tue, 3 Jan 2023 10:51:36 -0500 > Stefan Hajnoczi wrote: > > > QEMU's Makefile used to a use a technique where it generated > > "timestamp" files and used cmp(1) to check if rebuilding was > > necessary: > > 1. Always generate meson-buildoptions.sh-timestamp. > > `meson-buildoptions.sh-timestamp` would be the full expected output, > right? It's not just a date or something. > AFAIU that would make sure that if nothing changed in the output you > don't trigger other targets depending on `meson-buildoptions.sh`. It's > a solution for a different problem. > > The problem with always rebuilding `meson-buildoptions.sh` is that we > spend 1 extra second on every build, even those that doesn't need to > rebuild anything else. > Not unacceptable, but I think we should strive not to commit generated > files and move the file to the build directory, unless there's a reason > why this is not viable that I'm not seeing. The other problem with this file is that it appears to be generated differently depending on the host distro (specifically the default value for the --libdir option). That also would seem to nudge towards "don't commit a generated file". thanks -- PMM
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Tue, 3 Jan 2023 10:51:36 -0500 Stefan Hajnoczi wrote: > QEMU's Makefile used to a use a technique where it generated > "timestamp" files and used cmp(1) to check if rebuilding was > necessary: > 1. Always generate meson-buildoptions.sh-timestamp. `meson-buildoptions.sh-timestamp` would be the full expected output, right? It's not just a date or something. AFAIU that would make sure that if nothing changed in the output you don't trigger other targets depending on `meson-buildoptions.sh`. It's a solution for a different problem. The problem with always rebuilding `meson-buildoptions.sh` is that we spend 1 extra second on every build, even those that doesn't need to rebuild anything else. Not unacceptable, but I think we should strive not to commit generated files and move the file to the build directory, unless there's a reason why this is not viable that I'm not seeing. -- Alessandro Di Federico rev.ng Labs
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Tue, 3 Jan 2023 at 10:26, Alessandro Di Federico wrote: > > On Tue, 3 Jan 2023 09:37:51 -0500 > Stefan Hajnoczi wrote: > > > I don't understand the issue. Can you describe the steps that cause > > meson-buildoptions.sh to become out-of-sync with meson_options.txt? > > > > This will continue to be a problem in the future. Is there a way to > > fix it permanently? > > In Makefile we have: > > $(SRC_PATH)/scripts/meson-buildoptions.sh: > $(SRC_PATH)/meson_options.txt > > (Cc'ing Paolo since he's the author of this line) > > This means make will regenerate > `$(SRC_PATH)/scripts/meson-buildoptions.sh` if its last modification > date is older than `$(SRC_PATH)/meson_options.txt`. > > However these files are in the source directory, so this will behave > properly only under certain circumstances. > > For instance if, for some reason, someone committed a new version of > `meson_options.txt` but not of `meson-buildoptions.sh`, a fresh clone > of the repo will not have the dates set correctly to trigger the > Makefile rule above: > > $ ls -ln scripts/meson* > -rw-r--r-- 1 1000 1000 28913 Jan 3 15:58 scripts/meson-buildoptions.sh > -rw-r--r-- 1 1000 100091 Jan 3 15:58 scripts/meson.build > > This is because git does not update file dates depending on the last > commit changing them. > > This, on top of the fact that invoking `ninja` does not trigger > regeneration (which works for most other use cases), leads to a good > chance of forgetting to update meson-buildoptions.sh. > > We could add the target to ninja to mitigate the risk, but still, the > dates problem remains. > > An alternative solution would be to avoid committing generated files and > simply regenerating it every time. > > On my machine `meson.py introspect --buildoptions` + > `scripts/meson-buildoptions.py` take 1.070s. > `./configure --help` takes 0.162s, so it's a bit sad. > On the other hand an actual invocation of configure can take > significantly longer (`./configure` takes 29.150s on my machine). > > To avoid re-running it every time we could invoke `make > update-buildoptions` in `configure` but keep > `scripts/meson-buildoptions.sh` in the build directory. QEMU's Makefile used to a use a technique where it generated "timestamp" files and used cmp(1) to check if rebuilding was necessary: 1. Always generate meson-buildoptions.sh-timestamp. 2. If cmp meson-buildoptions.sh-timestamp meson-buildoptions.sh detects a difference, cp meson-buildoptions.sh-timestamp meson-buildoptions.sh. 3. Let make handle dependencies on meson-buildoptions.sh as usual. You can find examples by grepping for -timestamp in the git log -p output. I think this would solve the problem? Stefan
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Tue, 3 Jan 2023 09:37:51 -0500 Stefan Hajnoczi wrote: > I don't understand the issue. Can you describe the steps that cause > meson-buildoptions.sh to become out-of-sync with meson_options.txt? > > This will continue to be a problem in the future. Is there a way to > fix it permanently? In Makefile we have: $(SRC_PATH)/scripts/meson-buildoptions.sh:$(SRC_PATH)/meson_options.txt (Cc'ing Paolo since he's the author of this line) This means make will regenerate `$(SRC_PATH)/scripts/meson-buildoptions.sh` if its last modification date is older than `$(SRC_PATH)/meson_options.txt`. However these files are in the source directory, so this will behave properly only under certain circumstances. For instance if, for some reason, someone committed a new version of `meson_options.txt` but not of `meson-buildoptions.sh`, a fresh clone of the repo will not have the dates set correctly to trigger the Makefile rule above: $ ls -ln scripts/meson* -rw-r--r-- 1 1000 1000 28913 Jan 3 15:58 scripts/meson-buildoptions.sh -rw-r--r-- 1 1000 100091 Jan 3 15:58 scripts/meson.build This is because git does not update file dates depending on the last commit changing them. This, on top of the fact that invoking `ninja` does not trigger regeneration (which works for most other use cases), leads to a good chance of forgetting to update meson-buildoptions.sh. We could add the target to ninja to mitigate the risk, but still, the dates problem remains. An alternative solution would be to avoid committing generated files and simply regenerating it every time. On my machine `meson.py introspect --buildoptions` + `scripts/meson-buildoptions.py` take 1.070s. `./configure --help` takes 0.162s, so it's a bit sad. On the other hand an actual invocation of configure can take significantly longer (`./configure` takes 29.150s on my machine). To avoid re-running it every time we could invoke `make update-buildoptions` in `configure` but keep `scripts/meson-buildoptions.sh` in the build directory. -- Alessandro Di Federico rev.ng Labs
Re: [PATCH] Update scripts/meson-buildoptions.sh
On Mon, 2 Jan 2023 at 05:42, Alessandro Di Federico via wrote: > > Note: `Makefile` relies on modification dates in the source tree to > detect changes to `meson_options.txt`. However, git does not track > those. Therefore, the following was necessary to regenerate > `meson-buildoptions.sh`: > > touch meson_options.txt > cd "$BUILD_DIR" > make update-buildoptions I don't understand the issue. Can you describe the steps that cause meson-buildoptions.sh to become out-of-sync with meson_options.txt? This will continue to be a problem in the future. Is there a way to fix it permanently? Stefan
Re: [PATCH] Update scripts/meson-buildoptions.sh
On 02/01/2023 11.41, Alessandro Di Federico wrote:
Note: `Makefile` relies on modification dates in the source tree to
detect changes to `meson_options.txt`. However, git does not track
those. Therefore, the following was necessary to regenerate
`meson-buildoptions.sh`:
touch meson_options.txt
cd "$BUILD_DIR"
make update-buildoptions
Signed-off-by: Alessandro Di Federico
---
scripts/meson-buildoptions.sh | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index aa6e30ea911..0f71e92dcba 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -10,6 +10,9 @@ meson_options_help() {
printf "%s\n" ' affects only QEMU, not tools like
qemu-img)'
printf "%s\n" ' --datadir=VALUE Data file directory [share]'
printf "%s\n" ' --disable-coroutine-pool coroutine freelist (better
performance)'
+ printf "%s\n" ' --disable-hexagon-idef-parser'
+ printf "%s\n" ' use idef-parser to automatically
generate TCG'
+ printf "%s\n" ' code for the Hexagon frontend'
printf "%s\n" ' --disable-install-blobs install provided firmware blobs'
printf "%s\n" ' --docdir=VALUE Base directory for documentation
installation'
printf "%s\n" ' (can be empty) [share/doc]'
@@ -40,7 +43,8 @@ meson_options_help() {
printf "%s\n" ' --enable-trace-backends=CHOICES'
printf "%s\n" ' Set available tracing backends
[log] (choices:'
printf "%s\n" '
dtrace/ftrace/log/nop/simple/syslog/ust)'
- printf "%s\n" ' --firmwarepath=VALUESsearch PATH for firmware files
[share/qemu-firmware]'
+ printf "%s\n" ' --firmwarepath=VALUESsearch PATH for firmware files
[share/qemu-'
+ printf "%s\n" ' firmware]'
printf "%s\n" ' --iasl=VALUE Path to ACPI disassembler'
printf "%s\n" ' --includedir=VALUE Header file directory [include]'
printf "%s\n" ' --interp-prefix=VALUEwhere to find shared libraries
etc., use %M for'
@@ -93,7 +97,7 @@ meson_options_help() {
printf "%s\n" ' glusterfs Glusterfs block device driver'
printf "%s\n" ' gnutls GNUTLS cryptography support'
printf "%s\n" ' gtk GTK+ user interface'
- printf "%s\n" ' gtk-clipboard clipboard support for GTK (EXPERIMENTAL,
MAY HANG)'
+ printf "%s\n" ' gtk-clipboard clipboard support for the gtk UI
(EXPERIMENTAL, MAY HANG)'
printf "%s\n" ' guest-agent Build QEMU Guest Agent'
printf "%s\n" ' guest-agent-msi Build MSI package for the QEMU Guest Agent'
printf "%s\n" ' hax HAX acceleration support'
@@ -156,6 +160,8 @@ meson_options_help() {
printf "%s\n" ' usb-redir libusbredir support'
printf "%s\n" ' vde vde network backend support'
printf "%s\n" ' vdi vdi image format support'
+ printf "%s\n" ' vduse-blk-export'
+ printf "%s\n" ' VDUSE block export support'
printf "%s\n" ' vfio-user-server'
printf "%s\n" ' vfio-user server support'
printf "%s\n" ' vhost-cryptovhost-user crypto backend support'
@@ -164,8 +170,6 @@ meson_options_help() {
printf "%s\n" ' vhost-user vhost-user backend support'
printf "%s\n" ' vhost-user-blk-server'
printf "%s\n" ' build vhost-user-blk server'
- printf "%s\n" ' vduse-blk-export'
- printf "%s\n" ' VDUSE block export support'
printf "%s\n" ' vhost-vdpa vhost-vdpa kernel backend support'
printf "%s\n" ' virglrenderer virgl rendering support'
printf "%s\n" ' virtfs virtio-9p support'
@@ -283,6 +287,8 @@ _meson_option_parse() {
--disable-guest-agent-msi) printf "%s" -Dguest_agent_msi=disabled ;;
--enable-hax) printf "%s" -Dhax=enabled ;;
--disable-hax) printf "%s" -Dhax=disabled ;;
+--enable-hexagon-idef-parser) printf "%s" -Dhexagon_idef_parser=true ;;
+--disable-hexagon-idef-parser) printf "%s" -Dhexagon_idef_parser=false ;;
--enable-hvf) printf "%s" -Dhvf=enabled ;;
--disable-hvf) printf "%s" -Dhvf=disabled ;;
--iasl=*) quote_sh "-Diasl=$2" ;;
@@ -429,6 +435,8 @@ _meson_option_parse() {
--disable-vde) printf "%s" -Dvde=disabled ;;
--enable-vdi) printf "%s" -Dvdi=enabled ;;
--disable-vdi) printf "%s" -Dvdi=disabled ;;
+--enable-vduse-blk-export) printf "%s" -Dvduse_blk_export=enabled ;;
+--disable-vduse-blk-export) printf "%s" -Dvduse_blk_export=disabled ;;
--enable-vfio-user-server) printf "%s" -Dvfio_user_server=enabled ;;
--disable-vfio-user-server) printf "%s" -Dvfio_user_server=disabled ;;
--enable-vhost-crypto) printf "%s" -Dvhost_crypto=enabled ;;
@@ -441,8 +449,6 @@ _meson_option_parse() {
[PATCH] Update scripts/meson-buildoptions.sh
Note: `Makefile` relies on modification dates in the source tree to
detect changes to `meson_options.txt`. However, git does not track
those. Therefore, the following was necessary to regenerate
`meson-buildoptions.sh`:
touch meson_options.txt
cd "$BUILD_DIR"
make update-buildoptions
Signed-off-by: Alessandro Di Federico
---
scripts/meson-buildoptions.sh | 18 --
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index aa6e30ea911..0f71e92dcba 100644
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -10,6 +10,9 @@ meson_options_help() {
printf "%s\n" ' affects only QEMU, not tools like
qemu-img)'
printf "%s\n" ' --datadir=VALUE Data file directory [share]'
printf "%s\n" ' --disable-coroutine-pool coroutine freelist (better
performance)'
+ printf "%s\n" ' --disable-hexagon-idef-parser'
+ printf "%s\n" ' use idef-parser to automatically
generate TCG'
+ printf "%s\n" ' code for the Hexagon frontend'
printf "%s\n" ' --disable-install-blobs install provided firmware blobs'
printf "%s\n" ' --docdir=VALUE Base directory for documentation
installation'
printf "%s\n" ' (can be empty) [share/doc]'
@@ -40,7 +43,8 @@ meson_options_help() {
printf "%s\n" ' --enable-trace-backends=CHOICES'
printf "%s\n" ' Set available tracing backends
[log] (choices:'
printf "%s\n" '
dtrace/ftrace/log/nop/simple/syslog/ust)'
- printf "%s\n" ' --firmwarepath=VALUESsearch PATH for firmware files
[share/qemu-firmware]'
+ printf "%s\n" ' --firmwarepath=VALUESsearch PATH for firmware files
[share/qemu-'
+ printf "%s\n" ' firmware]'
printf "%s\n" ' --iasl=VALUE Path to ACPI disassembler'
printf "%s\n" ' --includedir=VALUE Header file directory [include]'
printf "%s\n" ' --interp-prefix=VALUEwhere to find shared libraries
etc., use %M for'
@@ -93,7 +97,7 @@ meson_options_help() {
printf "%s\n" ' glusterfs Glusterfs block device driver'
printf "%s\n" ' gnutls GNUTLS cryptography support'
printf "%s\n" ' gtk GTK+ user interface'
- printf "%s\n" ' gtk-clipboard clipboard support for GTK (EXPERIMENTAL,
MAY HANG)'
+ printf "%s\n" ' gtk-clipboard clipboard support for the gtk UI
(EXPERIMENTAL, MAY HANG)'
printf "%s\n" ' guest-agent Build QEMU Guest Agent'
printf "%s\n" ' guest-agent-msi Build MSI package for the QEMU Guest Agent'
printf "%s\n" ' hax HAX acceleration support'
@@ -156,6 +160,8 @@ meson_options_help() {
printf "%s\n" ' usb-redir libusbredir support'
printf "%s\n" ' vde vde network backend support'
printf "%s\n" ' vdi vdi image format support'
+ printf "%s\n" ' vduse-blk-export'
+ printf "%s\n" ' VDUSE block export support'
printf "%s\n" ' vfio-user-server'
printf "%s\n" ' vfio-user server support'
printf "%s\n" ' vhost-cryptovhost-user crypto backend support'
@@ -164,8 +170,6 @@ meson_options_help() {
printf "%s\n" ' vhost-user vhost-user backend support'
printf "%s\n" ' vhost-user-blk-server'
printf "%s\n" ' build vhost-user-blk server'
- printf "%s\n" ' vduse-blk-export'
- printf "%s\n" ' VDUSE block export support'
printf "%s\n" ' vhost-vdpa vhost-vdpa kernel backend support'
printf "%s\n" ' virglrenderer virgl rendering support'
printf "%s\n" ' virtfs virtio-9p support'
@@ -283,6 +287,8 @@ _meson_option_parse() {
--disable-guest-agent-msi) printf "%s" -Dguest_agent_msi=disabled ;;
--enable-hax) printf "%s" -Dhax=enabled ;;
--disable-hax) printf "%s" -Dhax=disabled ;;
+--enable-hexagon-idef-parser) printf "%s" -Dhexagon_idef_parser=true ;;
+--disable-hexagon-idef-parser) printf "%s" -Dhexagon_idef_parser=false ;;
--enable-hvf) printf "%s" -Dhvf=enabled ;;
--disable-hvf) printf "%s" -Dhvf=disabled ;;
--iasl=*) quote_sh "-Diasl=$2" ;;
@@ -429,6 +435,8 @@ _meson_option_parse() {
--disable-vde) printf "%s" -Dvde=disabled ;;
--enable-vdi) printf "%s" -Dvdi=enabled ;;
--disable-vdi) printf "%s" -Dvdi=disabled ;;
+--enable-vduse-blk-export) printf "%s" -Dvduse_blk_export=enabled ;;
+--disable-vduse-blk-export) printf "%s" -Dvduse_blk_export=disabled ;;
--enable-vfio-user-server) printf "%s" -Dvfio_user_server=enabled ;;
--disable-vfio-user-server) printf "%s" -Dvfio_user_server=disabled ;;
--enable-vhost-crypto) printf "%s" -Dvhost_crypto=enabled ;;
@@ -441,8 +449,6 @@ _meson_option_parse() {
--disable-vhost-user) printf "%s" -Dvhost_user=disabled ;;
--enable-vhost-user-blk-server) printf
