diff mbox series

[RFC] tests/vm: avoid re-building the VM images all the time

Message ID 20240226174639.438987-1-alex.bennee@linaro.org
State New
Headers show
Series [RFC] tests/vm: avoid re-building the VM images all the time | expand

Commit Message

Alex Bennée Feb. 26, 2024, 5:46 p.m. UTC 
There are two problems.

The first is a .PHONY target will always evaluate which triggers a
full re-build of the VM images. Drop the requirement knowing that this
introduces a manual step on freshly configure build dirs.

The second is a minor unrelated tweak to the Makefile also triggers an
expensive full re-build. Solve this be avoiding the dependency and
putting a comment just above the bit that matters and hope developers
notice the comment.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

---

This is hacky and sub-optimal. There surely must be a way to have our cake
and eat it?
---
 tests/vm/Makefile.include | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Alex Bennée Feb. 26, 2024, 6:04 p.m. UTC | #1 
Alex Bennée <alex.bennee@linaro.org> writes:

> There are two problems.
>
> The first is a .PHONY target will always evaluate which triggers a
> full re-build of the VM images. Drop the requirement knowing that this
> introduces a manual step on freshly configure build dirs.
>
> The second is a minor unrelated tweak to the Makefile also triggers an
> expensive full re-build. Solve this be avoiding the dependency and
> putting a comment just above the bit that matters and hope developers
> notice the comment.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> ---
>
> This is hacky and sub-optimal. There surely must be a way to have our cake
> and eat it?
> ---
>  tests/vm/Makefile.include | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index bf12e0fa3c5..a109773c588 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -88,10 +88,11 @@ vm-build-all: $(addprefix vm-build-, $(IMAGES))
>  vm-clean-all:
>  	rm -f $(IMAGE_FILES)
>  
> +# Rebuilding the VMs every time this Makefile is tweaked is very
> +# expensive for most users. If you tweak the recipe bellow you will
> +# need to manually zap $(IMAGES_DIR)/%.img to rebuild.
>  $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
> -			$(SRC_PATH)/tests/vm/basevm.py \
> -			$(SRC_PATH)/tests/vm/Makefile.include \
> -			$(VM_VENV)
> +			$(SRC_PATH)/tests/vm/basevm.py

Maybe:

 # need to manually zap $(IMAGES_DIR)/%.img to rebuild.
 $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
 			$(SRC_PATH)/tests/vm/basevm.py
+	$(if $(VM_VENV), make $(VM_VENV))
 	@mkdir -p $(IMAGES_DIR)
 	$(call quiet-command, \

?


>  	@mkdir -p $(IMAGES_DIR)
>  	$(call quiet-command, \
>  		$(VM_PYTHON) $< \
Peter Maydell Feb. 26, 2024, 6:12 p.m. UTC | #2 
On Mon, 26 Feb 2024 at 18:06, Alex Bennée <alex.bennee@linaro.org> wrote:
>
> Alex Bennée <alex.bennee@linaro.org> writes:
>
> > There are two problems.
> >
> > The first is a .PHONY target will always evaluate which triggers a
> > full re-build of the VM images. Drop the requirement knowing that this
> > introduces a manual step on freshly configure build dirs.
> >
> > The second is a minor unrelated tweak to the Makefile also triggers an
> > expensive full re-build. Solve this be avoiding the dependency and
> > putting a comment just above the bit that matters and hope developers
> > notice the comment.
> >
> > Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > ---
> >
> > This is hacky and sub-optimal. There surely must be a way to have our cake
> > and eat it?
> > ---
> >  tests/vm/Makefile.include | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> > index bf12e0fa3c5..a109773c588 100644
> > --- a/tests/vm/Makefile.include
> > +++ b/tests/vm/Makefile.include
> > @@ -88,10 +88,11 @@ vm-build-all: $(addprefix vm-build-, $(IMAGES))
> >  vm-clean-all:
> >       rm -f $(IMAGE_FILES)
> >
> > +# Rebuilding the VMs every time this Makefile is tweaked is very
> > +# expensive for most users. If you tweak the recipe bellow you will

"below".

But how many people edit tests/vm/Makefile.include ?
It had only 5 changes made to it last year. At that
frequency of changes I think I'd favour "always do the
right thing" over "require manual removal of the cached
image sometimes".

thanks
-- PMM
Daniel P. Berrangé Feb. 26, 2024, 6:23 p.m. UTC | #3 
On Mon, Feb 26, 2024 at 05:46:39PM +0000, Alex Bennée wrote:
> There are two problems.
> 
> The first is a .PHONY target will always evaluate which triggers a
> full re-build of the VM images. Drop the requirement knowing that this
> introduces a manual step on freshly configure build dirs.

For context, the background to this is:

   https://gitlab.com/qemu-project/qemu/-/issues/2118

dropping '$(VM_VENV)' is the fix for that bit, which is the real
killer bit.

> 
> The second is a minor unrelated tweak to the Makefile also triggers an
> expensive full re-build. Solve this be avoiding the dependency and
> putting a comment just above the bit that matters and hope developers
> notice the comment.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> 
> ---
> 
> This is hacky and sub-optimal. There surely must be a way to have our cake
> and eat it?
> ---
>  tests/vm/Makefile.include | 7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
> index bf12e0fa3c5..a109773c588 100644
> --- a/tests/vm/Makefile.include
> +++ b/tests/vm/Makefile.include
> @@ -88,10 +88,11 @@ vm-build-all: $(addprefix vm-build-, $(IMAGES))
>  vm-clean-all:
>  	rm -f $(IMAGE_FILES)
>  
> +# Rebuilding the VMs every time this Makefile is tweaked is very
> +# expensive for most users. If you tweak the recipe bellow you will
> +# need to manually zap $(IMAGES_DIR)/%.img to rebuild.
>  $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
> -			$(SRC_PATH)/tests/vm/basevm.py \
> -			$(SRC_PATH)/tests/vm/Makefile.include \
> -			$(VM_VENV)
> +			$(SRC_PATH)/tests/vm/basevm.py
>  	@mkdir -p $(IMAGES_DIR)
>  	$(call quiet-command, \
>  		$(VM_PYTHON) $< \
> -- 
> 2.39.2
> 
> 

With regards,
Daniel
diff mbox series

Patch

diff --git a/tests/vm/Makefile.include b/tests/vm/Makefile.include
index bf12e0fa3c5..a109773c588 100644
--- a/tests/vm/Makefile.include
+++ b/tests/vm/Makefile.include
@@ -88,10 +88,11 @@  vm-build-all: $(addprefix vm-build-, $(IMAGES))
 vm-clean-all:
 	rm -f $(IMAGE_FILES)
 
+# Rebuilding the VMs every time this Makefile is tweaked is very
+# expensive for most users. If you tweak the recipe bellow you will
+# need to manually zap $(IMAGES_DIR)/%.img to rebuild.
 $(IMAGES_DIR)/%.img:	$(SRC_PATH)/tests/vm/% \
-			$(SRC_PATH)/tests/vm/basevm.py \
-			$(SRC_PATH)/tests/vm/Makefile.include \
-			$(VM_VENV)
+			$(SRC_PATH)/tests/vm/basevm.py
 	@mkdir -p $(IMAGES_DIR)
 	$(call quiet-command, \
 		$(VM_PYTHON) $< \