Re: [XEN PATCH v3 04/25] tools/firmware/hvmloader: rework Makefile

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Fri, Jul 08, 2022 at 03:39:00PM +0000, Luca Fancellu wrote:
> > On 24 Jun 2022, at 17:04, Anthony PERARD <anthony.perard@xxxxxxxxxx> wrote:
[...]
> > For "roms.inc" target, use "$(SHELL)" instead of plain "sh". And use
> > full path to "mkhex" instead of a relative one. Lastly, add "-f" flag
> > to "mv", in case the target already exist.
> > 
> > Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> > ---
> > diff --git a/tools/firmware/hvmloader/Makefile 
> > b/tools/firmware/hvmloader/Makefile
> > index b754220839..fc20932110 100644
> > --- a/tools/firmware/hvmloader/Makefile
> > +++ b/tools/firmware/hvmloader/Makefile
> > @@ -87,21 +89,21 @@ roms.inc: $(ROMS)
> > 
> > ifneq ($(ROMBIOS_ROM),)
> >     echo "#ifdef ROM_INCLUDE_ROMBIOS" >> $@.new
> > -   sh ../../misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> > +   $(SHELL) $(XEN_ROOT)/tools/misc/mkhex rombios $(ROMBIOS_ROM) >> $@.new
> >     echo "#endif" >> $@.new
> > endif
> > 
> > ifneq ($(STDVGA_ROM),)
> >     echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> > -   sh ../../misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> $@.new
> > +   $(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_stdvga $(STDVGA_ROM) >> 
> > $@.new
> >     echo "#endif" >> $@.new
> > endif
> > ifneq ($(CIRRUSVGA_ROM),)
> >     echo "#ifdef ROM_INCLUDE_VGABIOS" >> $@.new
> > -   sh ../../misc/mkhex vgabios_cirrusvga $(CIRRUSVGA_ROM) >> $@.new
> > +   $(SHELL) $(XEN_ROOT)/tools/misc/mkhex vgabios_cirrusvga 
> > $(CIRRUSVGA_ROM) >> $@.new
> >     echo "#endif" >> $@.new
> > endif
> > -   mv $@.new $@
> > +   mv -f $@.new $@
> 
> Here, instead of -f, is it safer -u? What’s your opinion on that? The patch 
> looks good to me.

make want to rebuild the target, so there is no reason to keep the
existing target. We do need to overwrite the existing target if it
exist.

Thanks for the reviews!

-- 
Anthony PERARD