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

[Wei Huang]

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.

> 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.

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).

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
>