Re: [Qemu-devel] [PATCH V5 2/4] tests/migration: Convert the boot block compilation script into Makefile

[Dr. David Alan Gilbert]

* Wei Huang (address@hidden) wrote:
> 
> 
> On 02/27/2018 05:38 AM, Dr. David Alan Gilbert wrote:
> > * Wei Huang (address@hidden) wrote:
> >>
> >>
> >> On 02/26/2018 12:01 PM, Dr. David Alan Gilbert wrote:
> >>> * Wei Huang (address@hidden) wrote:
> >>>> The x86 boot block header currently is generated with a shell script.
> >>>> To better support other CPUs (e.g. aarch64), we convert the script
> >>>> into Makefile. This allows us to 1) support cross-compilation easily,
> >>>> and 2) avoid creating a script file for every architecture.
> >>>>
> >>>> Signed-off-by: Wei Huang <address@hidden>
> >>>
> >>> But what happens if you're on a system that doesn't have the cross
> >>> compilers?  I found it's too easy to get stuck with it trying
> >>> to rebuild the header when there's no need.
> >>
> >> I think it is fine. If no cross compiler is present, NNN_cross_prefix
> >> won't find anything. So it ends up with using the gcc tools on host
> >> systems. Obviously the compilation will fail. This should become the
> >> problem to solve for whoever wants to do cross-compile. For us, we have
> >> done our part because the .h files have been provided.
> > 
> > But isn't it a problem for people just trying to make check?
> 
> "make check" doesn't invoke the Makefile in tests/migration/. This
> Makefile is only invoked when people manually run "make" or "make
> x86-a-b-bootblock.h" inside the tests/migration/ directory.

Ah! That's what I hadn't spotted; I'd not noticed you'd created a
separate non-included Makefile.

In that case, yes:


Reviewed-by: Dr. David Alan Gilbert <address@hidden>

> > One of the things that convinced me to move it out of the makefile and
> > into a separate script was that it was too easy to get into a situation
> > where Make wanted to rebuild the headerilfe on a system.
> 
> This could be potentially one concern. But the real reason behind this
> concern is the cross-compilation doesn't always work (it depends on the
> availability of cross-compiler tools). My argument is this problem also
> exists in the original script approach. As long as QEMU doesn't include
> tests/migration/Makefile in "make" or "make check", I think this rebuild
> headerfile situation won't happen.

Yes.

> Also, using Makefile, we don't need to write a script for each single
> architecture. And it is easier to support cross-compilation detection
> (than script).

That doesn't make too much difference to me; but as long as it's a
separate Makefile that's OK.

Dave

> Thanks,
> -Wei
> 
> > 
> > Dave
> > 
> >>>
> >>> Dave
> >>>
> >>>> ---
> >>>>  tests/migration/Makefile                 | 36 
> >>>> ++++++++++++++++++++++++++++++++
> >>>>  tests/migration/rebuild-x86-bootblock.sh | 33 
> >>>> -----------------------------
> >>>>  tests/migration/x86-a-b-bootblock.h      |  2 +-
> >>>>  tests/migration/x86-a-b-bootblock.s      |  5 ++---
> >>>>  4 files changed, 39 insertions(+), 37 deletions(-)
> >>>>  create mode 100644 tests/migration/Makefile
> >>>>  delete mode 100755 tests/migration/rebuild-x86-bootblock.sh
> >>>>
> >>>> diff --git a/tests/migration/Makefile b/tests/migration/Makefile
> >>>> new file mode 100644
> >>>> index 0000000000..8fbedaa8b8
> >>>> --- /dev/null
> >>>> +++ b/tests/migration/Makefile
> >>>> @@ -0,0 +1,36 @@
> >>>> +#
> >>>> +# Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
> >>>> +#
> >>>> +# Authors:
> >>>> +#   Dave Gilbert <address@hidden>
> >>>> +#
> >>>> +# This work is licensed under the terms of the GNU GPL, version 2 or 
> >>>> later.
> >>>> +# See the COPYING file in the top-level directory.
> >>>> +#
> >>>> +export __note
> >>>> +override define __note
> >>>> +/* This file is automatically generated from
> >>>> + * tests/migration/$<, edit that and then run
> >>>> + * "make $@" inside tests/migration to update,
> >>>> + * and then remember to send both in your patch submission.
> >>>> + */
> >>>> +endef
> >>>> +
> >>>> +all: x86-a-b-bootblock.h
> >>>> +# Dummy command so that make thinks it has done something
> >>>> +        @true
> >>>> +
> >>>> +SRC_PATH=../..
> >>>> +include $(SRC_PATH)/rules.mak
> >>>> +
> >>>> +x86_64_cross_prefix := $(call find-cross-prefix,x86_64)
> >>>> +
> >>>> +x86-a-b-bootblock.h: x86-a-b-bootblock.s
> >>>> +        $(x86_64_cross_prefix)as --32 -march=i486 $< -o x86.o
> >>>> +        $(x86_64_cross_prefix)objcopy -O binary x86.o x86.boot
> >>>> +        dd if=x86.boot of=x86.bootsect bs=256 count=2 skip=124
> >>>> +        echo "$$__note" > $@
> >>>> +        xxd -i x86.bootsect | sed -e 's/.*int.*//' >> $@
> >>>> +
> >>>> +clean:
> >>>> +        rm -f *.bootsect *.boot *.o
> >>>> diff --git a/tests/migration/rebuild-x86-bootblock.sh 
> >>>> b/tests/migration/rebuild-x86-bootblock.sh
> >>>> deleted file mode 100755
> >>>> index 86cec5d284..0000000000
> >>>> --- a/tests/migration/rebuild-x86-bootblock.sh
> >>>> +++ /dev/null
> >>>> @@ -1,33 +0,0 @@
> >>>> -#!/bin/sh
> >>>> -# Copyright (c) 2016-2018 Red Hat, Inc. and/or its affiliates
> >>>> -# This work is licensed under the terms of the GNU GPL, version 2 or 
> >>>> later.
> >>>> -# See the COPYING file in the top-level directory.
> >>>> -#
> >>>> -# Author: address@hidden
> >>>> -
> >>>> -ASMFILE=$PWD/tests/migration/x86-a-b-bootblock.s
> >>>> -HEADER=$PWD/tests/migration/x86-a-b-bootblock.h
> >>>> -
> >>>> -if [ ! -e "$ASMFILE" ]
> >>>> -then
> >>>> -  echo "Couldn't find $ASMFILE" >&2
> >>>> -  exit 1
> >>>> -fi
> >>>> -
> >>>> -ASM_WORK_DIR=$(mktemp -d --tmpdir X86BB.XXXXXX)
> >>>> -cd "$ASM_WORK_DIR" &&
> >>>> -as --32 -march=i486 "$ASMFILE" -o x86.o &&
> >>>> -objcopy -O binary x86.o x86.boot &&
> >>>> -dd if=x86.boot of=x86.bootsect bs=256 count=2 skip=124 &&
> >>>> -xxd -i x86.bootsect |
> >>>> -sed -e 's/.*int.*//' > x86.hex &&
> >>>> -cat - x86.hex <<HERE > "$HEADER"
> >>>> -/* This file is automatically generated from
> >>>> - * tests/migration/x86-a-b-bootblock.s, edit that and then run
> >>>> - * tests/migration/rebuild-x86-bootblock.sh to update,
> >>>> - * and then remember to send both in your patch submission.
> >>>> - */
> >>>> -HERE
> >>>> -
> >>>> -rm x86.hex x86.bootsect x86.boot x86.o
> >>>> -cd .. && rmdir "$ASM_WORK_DIR"
> >>>> diff --git a/tests/migration/x86-a-b-bootblock.h 
> >>>> b/tests/migration/x86-a-b-bootblock.h
> >>>> index 78a151fe2a..9e8e2e028b 100644
> >>>> --- a/tests/migration/x86-a-b-bootblock.h
> >>>> +++ b/tests/migration/x86-a-b-bootblock.h
> >>>> @@ -1,6 +1,6 @@
> >>>>  /* This file is automatically generated from
> >>>>   * tests/migration/x86-a-b-bootblock.s, edit that and then run
> >>>> - * tests/migration/rebuild-x86-bootblock.sh to update,
> >>>> + * "make x86-a-b-bootblock.h" inside tests/migration to update,
> >>>>   * and then remember to send both in your patch submission.
> >>>>   */
> >>>>  unsigned char x86_bootsect[] = {
> >>>> diff --git a/tests/migration/x86-a-b-bootblock.s 
> >>>> b/tests/migration/x86-a-b-bootblock.s
> >>>> index b1642641a7..98dbfab084 100644
> >>>> --- a/tests/migration/x86-a-b-bootblock.s
> >>>> +++ b/tests/migration/x86-a-b-bootblock.s
> >>>> @@ -3,9 +3,8 @@
> >>>>  #  range.
> >>>>  #  Outputs an initial 'A' on serial followed by repeated 'B's
> >>>>  #
> >>>> -# run   tests/migration/rebuild-x86-bootblock.sh
> >>>> -#   to regenerate the hex, and remember to include both the .h and .s
> >>>> -#   in any patches.
> >>>> +#  In tests/migration dir, run 'make x86-a-b-bootblock.h' to regenerate
> >>>> +#  the hex, and remember to include both the .h and .s in any patches.
> >>>>  #
> >>>>  # Copyright (c) 2016 Red Hat, Inc. and/or its affiliates
> >>>>  # This work is licensed under the terms of the GNU GPL, version 2 or 
> >>>> later.
> >>>> -- 
> >>>> 2.14.3
> >>>>
> >>> --
> >>> Dr. David Alan Gilbert / address@hidden / Manchester, UK
> >>>
> > --
> > Dr. David Alan Gilbert / address@hidden / Manchester, UK
> > 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK