diff mbox series

[v2,4/5] tests/uefi-test-tools: add build scripts

Message ID 20190124203959.30875-5-lersek@redhat.com (mailing list archive)
State New, archived
Headers show
Series add the BiosTablesTest UEFI app, build it with the new roms/edk2 submodule | expand

Commit Message

Laszlo Ersek Jan. 24, 2019, 8:39 p.m. UTC
Introduce the following build scripts under "tests/uefi-test-tools":

* "build.sh" builds a single module (a UEFI application) from
  UefiTestToolsPkg, for a single QEMU emulation target.

  "build.sh" relies on cross-compilers when the emulation target and the
  build host architecture don't match. The cross-compiler prefix is
  computed according to a fixed, Linux-specific pattern. No attempt is
  made to copy or reimplement the GNU Make magic from "qemu/roms/Makefile"
  for cross-compiler prefix determination. The reason is that the build
  host OSes that are officially supported by edk2, and those that are
  supported by QEMU, intersect only in Linux. (Note that the UNIXGCC
  toolchain is being removed from edk2,
  <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>.)

* "Makefile" currently builds the "UefiTestToolsPkg/BiosTablesTest"
  application, for arm, aarch64, i386, and x86_64, with the help of
  "build.sh".

  "Makefile" turns each resultant UEFI executable into a UEFI-bootable,
  qcow2-compressed ISO image. The ISO images are output as
  "tests/data/uefi-boot-images/bios-tables-test.<TARGET>.iso.qcow2".

  Each ISO image should be passed to QEMU as follows:

    -drive id=boot-cd,if=none,readonly,format=qcow2,file=$ISO \
    -device virtio-scsi-pci,id=scsi0 \
    -device scsi-cd,drive=boot-cd,bus=scsi0.0,bootindex=0 \

  "Makefile" assumes that "mkdosfs", "mtools", and "genisoimage" are
  present.

Cc: "Michael S. Tsirkin" <mst@redhat.com>
Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Igor Mammedov <imammedo@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    v2:
    - add the .NOTPARALLEL target [Phil, help-make, edk2-devel]

 tests/uefi-test-tools/Makefile   |  97 +++++++++++++
 tests/uefi-test-tools/.gitignore |   3 +
 tests/uefi-test-tools/build.sh   | 145 ++++++++++++++++++++
 3 files changed, 245 insertions(+)

Comments

Philippe Mathieu-Daudé Jan. 31, 2019, 5:07 p.m. UTC | #1
Hi Laszlo,

On 1/24/19 9:39 PM, Laszlo Ersek wrote:
> Introduce the following build scripts under "tests/uefi-test-tools":
> 
> * "build.sh" builds a single module (a UEFI application) from
>   UefiTestToolsPkg, for a single QEMU emulation target.
> 
>   "build.sh" relies on cross-compilers when the emulation target and the
>   build host architecture don't match. The cross-compiler prefix is
>   computed according to a fixed, Linux-specific pattern. No attempt is
>   made to copy or reimplement the GNU Make magic from "qemu/roms/Makefile"
>   for cross-compiler prefix determination. The reason is that the build
>   host OSes that are officially supported by edk2, and those that are
>   supported by QEMU, intersect only in Linux. (Note that the UNIXGCC
>   toolchain is being removed from edk2,
>   <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>.)
> 
> * "Makefile" currently builds the "UefiTestToolsPkg/BiosTablesTest"
>   application, for arm, aarch64, i386, and x86_64, with the help of
>   "build.sh".
> 
>   "Makefile" turns each resultant UEFI executable into a UEFI-bootable,
>   qcow2-compressed ISO image. The ISO images are output as
>   "tests/data/uefi-boot-images/bios-tables-test.<TARGET>.iso.qcow2".
> 
>   Each ISO image should be passed to QEMU as follows:
> 
>     -drive id=boot-cd,if=none,readonly,format=qcow2,file=$ISO \
>     -device virtio-scsi-pci,id=scsi0 \
>     -device scsi-cd,drive=boot-cd,bus=scsi0.0,bootindex=0 \
> 
>   "Makefile" assumes that "mkdosfs", "mtools", and "genisoimage" are
>   present.
> 
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Igor Mammedov <imammedo@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     v2:
>     - add the .NOTPARALLEL target [Phil, help-make, edk2-devel]
> 
>  tests/uefi-test-tools/Makefile   |  97 +++++++++++++
>  tests/uefi-test-tools/.gitignore |   3 +
>  tests/uefi-test-tools/build.sh   | 145 ++++++++++++++++++++
>  3 files changed, 245 insertions(+)
> 
> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
> new file mode 100644
> index 000000000000..61d263861e01
> --- /dev/null
> +++ b/tests/uefi-test-tools/Makefile
> @@ -0,0 +1,97 @@
> +# Makefile for the test helper UEFI applications that run in guests.
> +#
> +# Copyright (C) 2019, Red Hat, Inc.
> +#
> +# This program and the accompanying materials are licensed and made available
> +# under the terms and conditions of the BSD License that accompanies this
> +# distribution. The full text of the license may be found at
> +# <http://opensource.org/licenses/bsd-license.php>.
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +edk2_dir              := ../../roms/edk2
> +images_dir            := ../data/uefi-boot-images
> +emulation_targets     := arm aarch64 i386 x86_64
> +uefi_binaries         := bios-tables-test
> +intermediate_suffixes := .efi .fat .iso.raw
> +
> +images: $(foreach binary,$(uefi_binaries), \
> +		$(foreach target,$(emulation_targets), \
> +			$(images_dir)/$(binary).$(target).iso.qcow2))
> +
> +# Preserve all intermediate targets if the build succeeds.
> +# - Intermediate targets help with development & debugging.
> +# - Preserving intermediate targets also keeps spurious changes out of the
> +#   final build products, in case the user re-runs "make" without any changes
> +#   to the UEFI source code. Normally, the intermediate files would have been
> +#   removed by the last "make" invocation, hence the re-run would rebuild them
> +#   from the unchanged UEFI sources. Unfortunately, the "mkdosfs" and
> +#   "genisoimage" utilities embed timestamp-based information in their outputs,
> +#   which causes git to report differences for the tracked qcow2 ISO images.
> +.SECONDARY: $(foreach binary,$(uefi_binaries), \
> +		$(foreach target,$(emulation_targets), \
> +			$(foreach suffix,$(intermediate_suffixes), \
> +				Build/$(binary).$(target)$(suffix))))
> +
> +# In the pattern rules below, the stem (%, $*) stands for
> +# "$(binary).$(target)".
> +
> +# Convert the raw ISO image to a qcow2 one, enabling compression, and using a
> +# small cluster size. This allows for small binary files under git control,
> +# hence for small binary patches.
> +$(images_dir)/%.iso.qcow2: Build/%.iso.raw
> +	mkdir -p -- $(images_dir)
> +	$${QTEST_QEMU_IMG:-qemu-img} convert -f raw -O qcow2 -c \
> +		-o cluster_size=512 -- $< $@
> +
> +# Embed the "UEFI system partition" into an ISO9660 file system as an ElTorito
> +# boot image.
> +Build/%.iso.raw: Build/%.fat
> +	genisoimage -input-charset ASCII -efi-boot $(notdir $<) -no-emul-boot \
> +		-quiet -o $@ -- $<
> +
> +# Define chained macros in order to map QEMU system emulation targets to
> +# *short* UEFI architecture identifiers. Periods are allowed in, and ultimately
> +# stripped from, the argument.
> +map_arm_to_uefi     = $(subst arm,ARM,$(1))
> +map_aarch64_to_uefi = $(subst aarch64,AA64,$(call map_arm_to_uefi,$(1)))
> +map_i386_to_uefi    = $(subst i386,IA32,$(call map_aarch64_to_uefi,$(1)))
> +map_x86_64_to_uefi  = $(subst x86_64,X64,$(call map_i386_to_uefi,$(1)))
> +map_to_uefi         = $(subst .,,$(call map_x86_64_to_uefi,$(1)))
> +
> +# Format a "UEFI system partition", using the UEFI binary as the default boot
> +# loader. Add 10% size for filesystem metadata, round up to the next KB, and
> +# make sure the size is large enough for a FAT filesystem. Name the filesystem
> +# after the UEFI binary. (Excess characters are automatically dropped from the
> +# filesystem label.)
> +Build/%.fat: Build/%.efi
> +	rm -f -- $@
> +	uefi_bin_b=$$(stat --format=%s -- $<) && \
> +		uefi_fat_kb=$$(( (uefi_bin_b * 11 / 10 + 1023) / 1024 )) && \
> +		uefi_fat_kb=$$(( uefi_fat_kb >= 64 ? uefi_fat_kb : 64 )) && \
> +		mkdosfs -C $@ -n $(basename $(@F)) -- $$uefi_fat_kb
> +	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI
> +	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI/BOOT
> +	MTOOLS_SKIP_CHECK=1 mcopy -i $@ -- $< \
> +		::EFI/BOOT/BOOT$(call map_to_uefi,$(suffix $*)).EFI
> +
> +# In the pattern rules below, the stem (%, $*) stands for "$(target)" only. The
> +# association between the UEFI binary (such as "bios-tables-test") and the
> +# component name from the edk2 platform DSC file (such as "BiosTablesTest") is
> +# explicit in each rule.
> +
> +# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given edk2
> +# workspace, at most one "build" instance may be operating at a time. Therefore
> +# we must serialize the rebuilding of targets in this Makefile.
> +.NOTPARALLEL:

Well this doesn't seem to improve my case :|

You can test with:

$ alias make='make -j8 -l7.5'

So you get:

$ make print-MAKEFLAGS
MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4

and this command fails:

$ make -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi

Now, using -j1 we have:

$ make print-MAKEFLAGS -j1
MAKEFLAGS=rR -j1 -l7.5

And the builds work:

$ make -j1 -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi

Reading 'info make' section "5.7.3 Communicating Options to a
Sub-'make'" I figured we can overwrite MAKEFLAGS, and this snippet works
like charm:

-- >8 --
@@ -84,8 +84,7 @@ Build/%.fat: Build/%.efi
 # "build.sh" invokes the "build" utility of edk2 BaseTools. In any
given edk2
 # workspace, at most one "build" instance may be operating at a time.
Therefore
 # we must serialize the rebuilding of targets in this Makefile.
-.NOTPARALLEL:
-
+Build/bios-tables-test.%.efi: MAKEFLAGS=
 Build/bios-tables-test.%.efi: build-edk2-tools
        ./build.sh $(edk2_dir) BiosTablesTest $* $@

---

I tested with:

$ rm -rf tests/uefi-test-tools/Build
$ make -C tests/uefi-test-tools \
  Build/bios-tables-test.{arm,aarch64,i386,x86_64}.efi images -j42

What do you think about using this snippet? I don't know if this is the
best way to solve this, but it works...
If you test/agree, Michael could eventually apply the snippet directly.

> +
> +Build/bios-tables-test.%.efi: build-edk2-tools
> +	./build.sh $(edk2_dir) BiosTablesTest $* $@
> +
> +build-edk2-tools:
> +	$(MAKE) -C $(edk2_dir)/BaseTools
> +
> +clean:
> +	rm -rf Build Conf log
> +	$(MAKE) -C $(edk2_dir)/BaseTools clean
> diff --git a/tests/uefi-test-tools/.gitignore b/tests/uefi-test-tools/.gitignore
> new file mode 100644
> index 000000000000..9f246701dea1
> --- /dev/null
> +++ b/tests/uefi-test-tools/.gitignore
> @@ -0,0 +1,3 @@
> +Build
> +Conf
> +log
> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
> new file mode 100755
> index 000000000000..155cb75c4ddb
> --- /dev/null
> +++ b/tests/uefi-test-tools/build.sh
> @@ -0,0 +1,145 @@
> +#!/bin/bash
> +
> +# Build script that determines the edk2 toolchain to use, invokes the edk2
> +# "build" utility, and copies the built UEFI binary to the requested location.
> +#
> +# Copyright (C) 2019, Red Hat, Inc.
> +#
> +# This program and the accompanying materials are licensed and made available
> +# under the terms and conditions of the BSD License that accompanies this
> +# distribution. The full text of the license may be found at
> +# <http://opensource.org/licenses/bsd-license.php>.
> +#
> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
> +
> +set -e -u -C
> +
> +# Save the command line arguments. We need to reset $# to 0 before sourcing
> +# "edksetup.sh", as it will inherit $@.
> +program_name=$(basename -- "$0")
> +edk2_dir=$1
> +dsc_component=$2
> +emulation_target=$3
> +uefi_binary=$4
> +shift 4
> +
> +# Set up the environment for edk2 building.
> +export PACKAGES_PATH=$(realpath -- "$edk2_dir")
> +export WORKSPACE=$PWD
> +mkdir -p Conf
> +
> +# Source "edksetup.sh" carefully.
> +set +e +u +C
> +source "$PACKAGES_PATH/edksetup.sh"
> +ret=$?
> +set -e -u -C
> +if [ $ret -ne 0 ]; then
> +  exit $ret
> +fi
> +
> +# Map the QEMU system emulation target to the following types of architecture
> +# identifiers:
> +# - edk2,
> +# - gcc cross-compilation.
> +# Cover only those targets that are supported by the UEFI spec and edk2.
> +case "$emulation_target" in
> +  (arm)
> +    edk2_arch=ARM
> +    gcc_arch=arm
> +    ;;
> +  (aarch64)
> +    edk2_arch=AARCH64
> +    gcc_arch=aarch64
> +    ;;
> +  (i386)
> +    edk2_arch=IA32
> +    gcc_arch=i686
> +    ;;
> +  (x86_64)
> +    edk2_arch=X64
> +    gcc_arch=x86_64
> +    ;;
> +  (*)
> +    printf '%s: unknown/unsupported QEMU system emulation target "%s"\n' \
> +      "$program_name" "$emulation_target" >&2
> +    exit 1
> +    ;;
> +esac
> +
> +# Check if cross-compilation is needed.
> +host_arch=$(uname -m)
> +if [ "$gcc_arch" == "$host_arch" ] ||
> +   ( [ "$gcc_arch" == i686 ] && [ "$host_arch" == x86_64 ] ); then
> +  cross_prefix=
> +else
> +  cross_prefix=${gcc_arch}-linux-gnu-
> +fi
> +
> +# Expose cross_prefix (which is possibly empty) to the edk2 tools. While at it,
> +# determine the suitable edk2 toolchain as well.
> +# - For ARM and AARCH64, edk2 only offers the GCC5 toolchain tag, which covers
> +#   the gcc-5+ releases.
> +# - For IA32 and X64, edk2 offers the GCC44 through GCC49 toolchain tags, in
> +#   addition to GCC5. Unfortunately, the mapping between the toolchain tags and
> +#   the actual gcc releases isn't entirely trivial. Run "git-blame" on
> +#   "OvmfPkg/build.sh" in edk2 for more information.
> +# And, because the above is too simple, we have to assign cross_prefix to an
> +# edk2 build variable that is specific to both the toolchain tag and the target
> +# architecture.
> +case "$edk2_arch" in
> +  (ARM)
> +    edk2_toolchain=GCC5
> +    export GCC5_ARM_PREFIX=$cross_prefix
> +    ;;
> +  (AARCH64)
> +    edk2_toolchain=GCC5
> +    export GCC5_AARCH64_PREFIX=$cross_prefix
> +    ;;
> +  (IA32|X64)
> +    gcc_version=$("${cross_prefix}gcc" -v 2>&1 | tail -1 | awk '{print $3}')
> +    case "$gcc_version" in
> +      ([1-3].*|4.[0-3].*)
> +        printf '%s: unsupported gcc version "%s"\n' \
> +          "$program_name" "$gcc_version" >&2
> +        exit 1
> +        ;;
> +      (4.4.*)
> +        edk2_toolchain=GCC44
> +        ;;
> +      (4.5.*)
> +        edk2_toolchain=GCC45
> +        ;;
> +      (4.6.*)
> +        edk2_toolchain=GCC46
> +        ;;
> +      (4.7.*)
> +        edk2_toolchain=GCC47
> +        ;;
> +      (4.8.*)
> +        edk2_toolchain=GCC48
> +        ;;
> +      (4.9.*|6.[0-2].*)
> +        edk2_toolchain=GCC49
> +        ;;
> +      (*)
> +        edk2_toolchain=GCC5
> +        ;;
> +    esac
> +    eval "export ${edk2_toolchain}_BIN=\$cross_prefix"
> +    ;;
> +esac
> +
> +# Build the UEFI binary
> +mkdir -p log
> +build \
> +  --arch="$edk2_arch" \
> +  --buildtarget=DEBUG \
> +  --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
> +  --tagname="$edk2_toolchain" \
> +  --module="UefiTestToolsPkg/$dsc_component/$dsc_component.inf" \
> +  --log="log/$dsc_component.$edk2_arch.log" \
> +  --report-file="log/$dsc_component.$edk2_arch.report"
> +cp -a -- \
> +  "Build/UefiTestTools/DEBUG_${edk2_toolchain}/$edk2_arch/$dsc_component.efi" \
> +  "$uefi_binary"
>
Laszlo Ersek Jan. 31, 2019, 6:55 p.m. UTC | #2
On 01/31/19 18:07, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 1/24/19 9:39 PM, Laszlo Ersek wrote:
>> Introduce the following build scripts under "tests/uefi-test-tools":
>>
>> * "build.sh" builds a single module (a UEFI application) from
>>   UefiTestToolsPkg, for a single QEMU emulation target.
>>
>>   "build.sh" relies on cross-compilers when the emulation target and the
>>   build host architecture don't match. The cross-compiler prefix is
>>   computed according to a fixed, Linux-specific pattern. No attempt is
>>   made to copy or reimplement the GNU Make magic from "qemu/roms/Makefile"
>>   for cross-compiler prefix determination. The reason is that the build
>>   host OSes that are officially supported by edk2, and those that are
>>   supported by QEMU, intersect only in Linux. (Note that the UNIXGCC
>>   toolchain is being removed from edk2,
>>   <https://bugzilla.tianocore.org/show_bug.cgi?id=1377>.)
>>
>> * "Makefile" currently builds the "UefiTestToolsPkg/BiosTablesTest"
>>   application, for arm, aarch64, i386, and x86_64, with the help of
>>   "build.sh".
>>
>>   "Makefile" turns each resultant UEFI executable into a UEFI-bootable,
>>   qcow2-compressed ISO image. The ISO images are output as
>>   "tests/data/uefi-boot-images/bios-tables-test.<TARGET>.iso.qcow2".
>>
>>   Each ISO image should be passed to QEMU as follows:
>>
>>     -drive id=boot-cd,if=none,readonly,format=qcow2,file=$ISO \
>>     -device virtio-scsi-pci,id=scsi0 \
>>     -device scsi-cd,drive=boot-cd,bus=scsi0.0,bootindex=0 \
>>
>>   "Makefile" assumes that "mkdosfs", "mtools", and "genisoimage" are
>>   present.
>>
>> Cc: "Michael S. Tsirkin" <mst@redhat.com>
>> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>> Cc: Igor Mammedov <imammedo@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Cc: Shannon Zhao <shannon.zhaosl@gmail.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     v2:
>>     - add the .NOTPARALLEL target [Phil, help-make, edk2-devel]
>>
>>  tests/uefi-test-tools/Makefile   |  97 +++++++++++++
>>  tests/uefi-test-tools/.gitignore |   3 +
>>  tests/uefi-test-tools/build.sh   | 145 ++++++++++++++++++++
>>  3 files changed, 245 insertions(+)
>>
>> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
>> new file mode 100644
>> index 000000000000..61d263861e01
>> --- /dev/null
>> +++ b/tests/uefi-test-tools/Makefile
>> @@ -0,0 +1,97 @@
>> +# Makefile for the test helper UEFI applications that run in guests.
>> +#
>> +# Copyright (C) 2019, Red Hat, Inc.
>> +#
>> +# This program and the accompanying materials are licensed and made available
>> +# under the terms and conditions of the BSD License that accompanies this
>> +# distribution. The full text of the license may be found at
>> +# <http://opensource.org/licenses/bsd-license.php>.
>> +#
>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +edk2_dir              := ../../roms/edk2
>> +images_dir            := ../data/uefi-boot-images
>> +emulation_targets     := arm aarch64 i386 x86_64
>> +uefi_binaries         := bios-tables-test
>> +intermediate_suffixes := .efi .fat .iso.raw
>> +
>> +images: $(foreach binary,$(uefi_binaries), \
>> +		$(foreach target,$(emulation_targets), \
>> +			$(images_dir)/$(binary).$(target).iso.qcow2))
>> +
>> +# Preserve all intermediate targets if the build succeeds.
>> +# - Intermediate targets help with development & debugging.
>> +# - Preserving intermediate targets also keeps spurious changes out of the
>> +#   final build products, in case the user re-runs "make" without any changes
>> +#   to the UEFI source code. Normally, the intermediate files would have been
>> +#   removed by the last "make" invocation, hence the re-run would rebuild them
>> +#   from the unchanged UEFI sources. Unfortunately, the "mkdosfs" and
>> +#   "genisoimage" utilities embed timestamp-based information in their outputs,
>> +#   which causes git to report differences for the tracked qcow2 ISO images.
>> +.SECONDARY: $(foreach binary,$(uefi_binaries), \
>> +		$(foreach target,$(emulation_targets), \
>> +			$(foreach suffix,$(intermediate_suffixes), \
>> +				Build/$(binary).$(target)$(suffix))))
>> +
>> +# In the pattern rules below, the stem (%, $*) stands for
>> +# "$(binary).$(target)".
>> +
>> +# Convert the raw ISO image to a qcow2 one, enabling compression, and using a
>> +# small cluster size. This allows for small binary files under git control,
>> +# hence for small binary patches.
>> +$(images_dir)/%.iso.qcow2: Build/%.iso.raw
>> +	mkdir -p -- $(images_dir)
>> +	$${QTEST_QEMU_IMG:-qemu-img} convert -f raw -O qcow2 -c \
>> +		-o cluster_size=512 -- $< $@
>> +
>> +# Embed the "UEFI system partition" into an ISO9660 file system as an ElTorito
>> +# boot image.
>> +Build/%.iso.raw: Build/%.fat
>> +	genisoimage -input-charset ASCII -efi-boot $(notdir $<) -no-emul-boot \
>> +		-quiet -o $@ -- $<
>> +
>> +# Define chained macros in order to map QEMU system emulation targets to
>> +# *short* UEFI architecture identifiers. Periods are allowed in, and ultimately
>> +# stripped from, the argument.
>> +map_arm_to_uefi     = $(subst arm,ARM,$(1))
>> +map_aarch64_to_uefi = $(subst aarch64,AA64,$(call map_arm_to_uefi,$(1)))
>> +map_i386_to_uefi    = $(subst i386,IA32,$(call map_aarch64_to_uefi,$(1)))
>> +map_x86_64_to_uefi  = $(subst x86_64,X64,$(call map_i386_to_uefi,$(1)))
>> +map_to_uefi         = $(subst .,,$(call map_x86_64_to_uefi,$(1)))
>> +
>> +# Format a "UEFI system partition", using the UEFI binary as the default boot
>> +# loader. Add 10% size for filesystem metadata, round up to the next KB, and
>> +# make sure the size is large enough for a FAT filesystem. Name the filesystem
>> +# after the UEFI binary. (Excess characters are automatically dropped from the
>> +# filesystem label.)
>> +Build/%.fat: Build/%.efi
>> +	rm -f -- $@
>> +	uefi_bin_b=$$(stat --format=%s -- $<) && \
>> +		uefi_fat_kb=$$(( (uefi_bin_b * 11 / 10 + 1023) / 1024 )) && \
>> +		uefi_fat_kb=$$(( uefi_fat_kb >= 64 ? uefi_fat_kb : 64 )) && \
>> +		mkdosfs -C $@ -n $(basename $(@F)) -- $$uefi_fat_kb
>> +	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI
>> +	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI/BOOT
>> +	MTOOLS_SKIP_CHECK=1 mcopy -i $@ -- $< \
>> +		::EFI/BOOT/BOOT$(call map_to_uefi,$(suffix $*)).EFI
>> +
>> +# In the pattern rules below, the stem (%, $*) stands for "$(target)" only. The
>> +# association between the UEFI binary (such as "bios-tables-test") and the
>> +# component name from the edk2 platform DSC file (such as "BiosTablesTest") is
>> +# explicit in each rule.
>> +
>> +# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given edk2
>> +# workspace, at most one "build" instance may be operating at a time. Therefore
>> +# we must serialize the rebuilding of targets in this Makefile.
>> +.NOTPARALLEL:
> 
> Well this doesn't seem to improve my case :|
> 
> You can test with:
> 
> $ alias make='make -j8 -l7.5'
> 
> So you get:
> 
> $ make print-MAKEFLAGS
> MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4

I get something different (with make-3.82-23.el7.x86_64):

MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j

Notably, the argument 8 of "-j" is dropped.

Anyway, I think this is a side topic.


> and this command fails:
> 
> $ make -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi

(1) How *exactly* does it fail for you?

(2) What operating system and "make" are you using?

Because, the command completes perfectly fine for me. (Just for this
test, I rebased the v2 branch on top of current master, i.e. commit
aefcd2836620, and re-tested there.)

(3) Also, did you update all submodules first? (git submodule update
--init --force)

(4) IMPORTANT: did you make sure you didn't have an earlier *mis-built*
BaseTools directory in the submodule? For that, run "make clean" in the
QEMU project root. Note that "git clean -ffdx" will *not* clean
submodules, when issued from the QEMU project root.


> Now, using -j1 we have:
> 
> $ make print-MAKEFLAGS -j1
> MAKEFLAGS=rR -j1 -l7.5
> 
> And the builds work:
> 
> $ make -j1 -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi
> 
> Reading 'info make' section "5.7.3 Communicating Options to a
> Sub-'make'" I figured we can overwrite MAKEFLAGS, and this snippet works
> like charm:
> 
> -- >8 --
> @@ -84,8 +84,7 @@ Build/%.fat: Build/%.efi
>  # "build.sh" invokes the "build" utility of edk2 BaseTools. In any
> given edk2
>  # workspace, at most one "build" instance may be operating at a time.
> Therefore
>  # we must serialize the rebuilding of targets in this Makefile.
> -.NOTPARALLEL:
> -
> +Build/bios-tables-test.%.efi: MAKEFLAGS=
>  Build/bios-tables-test.%.efi: build-edk2-tools
>         ./build.sh $(edk2_dir) BiosTablesTest $* $@
> 
> ---

No, this is wrong. First, it removes other make flags that are not
related to parallelized building. Second, I was advised on the help-make
mailing list that if we really want to manipulate make options (as
opposed to setting .NOTPARALLEL), then we should explicitly append a
"-j1" to the invocation of the inner make. Please see this thread:
<http://lists.gnu.org/archive/html/help-make/2019-01/msg00004.html>.

That would mean modifying edk2 build code (because that's where the
inner make is invoked from), and I started researching that, but it
wasn't necessary in the end.


> I tested with:
> 
> $ rm -rf tests/uefi-test-tools/Build
> $ make -C tests/uefi-test-tools \
>   Build/bios-tables-test.{arm,aarch64,i386,x86_64}.efi images -j42

This command works for me flawlessly already, with my tree as-is.


> What do you think about using this snippet? I don't know if this is the
> best way to solve this, but it works...
> If you test/agree, Michael could eventually apply the snippet directly.

Again, I disagree with wiping MAKEFLAGS (or messing with it in any way);
*if* we need to modify the inner make's command line, that should be
done differently. However, my main point is that we need not modify the
inner make's invocation.

Can you please answer questions (1) through (4)? If it still fails for
you afterwards, I could attempt reproducing the issue in an environment
that's similar to yours.

Thanks,
Laszlo

> 
>> +
>> +Build/bios-tables-test.%.efi: build-edk2-tools
>> +	./build.sh $(edk2_dir) BiosTablesTest $* $@
>> +
>> +build-edk2-tools:
>> +	$(MAKE) -C $(edk2_dir)/BaseTools
>> +
>> +clean:
>> +	rm -rf Build Conf log
>> +	$(MAKE) -C $(edk2_dir)/BaseTools clean
>> diff --git a/tests/uefi-test-tools/.gitignore b/tests/uefi-test-tools/.gitignore
>> new file mode 100644
>> index 000000000000..9f246701dea1
>> --- /dev/null
>> +++ b/tests/uefi-test-tools/.gitignore
>> @@ -0,0 +1,3 @@
>> +Build
>> +Conf
>> +log
>> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
>> new file mode 100755
>> index 000000000000..155cb75c4ddb
>> --- /dev/null
>> +++ b/tests/uefi-test-tools/build.sh
>> @@ -0,0 +1,145 @@
>> +#!/bin/bash
>> +
>> +# Build script that determines the edk2 toolchain to use, invokes the edk2
>> +# "build" utility, and copies the built UEFI binary to the requested location.
>> +#
>> +# Copyright (C) 2019, Red Hat, Inc.
>> +#
>> +# This program and the accompanying materials are licensed and made available
>> +# under the terms and conditions of the BSD License that accompanies this
>> +# distribution. The full text of the license may be found at
>> +# <http://opensource.org/licenses/bsd-license.php>.
>> +#
>> +# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
>> +# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
>> +
>> +set -e -u -C
>> +
>> +# Save the command line arguments. We need to reset $# to 0 before sourcing
>> +# "edksetup.sh", as it will inherit $@.
>> +program_name=$(basename -- "$0")
>> +edk2_dir=$1
>> +dsc_component=$2
>> +emulation_target=$3
>> +uefi_binary=$4
>> +shift 4
>> +
>> +# Set up the environment for edk2 building.
>> +export PACKAGES_PATH=$(realpath -- "$edk2_dir")
>> +export WORKSPACE=$PWD
>> +mkdir -p Conf
>> +
>> +# Source "edksetup.sh" carefully.
>> +set +e +u +C
>> +source "$PACKAGES_PATH/edksetup.sh"
>> +ret=$?
>> +set -e -u -C
>> +if [ $ret -ne 0 ]; then
>> +  exit $ret
>> +fi
>> +
>> +# Map the QEMU system emulation target to the following types of architecture
>> +# identifiers:
>> +# - edk2,
>> +# - gcc cross-compilation.
>> +# Cover only those targets that are supported by the UEFI spec and edk2.
>> +case "$emulation_target" in
>> +  (arm)
>> +    edk2_arch=ARM
>> +    gcc_arch=arm
>> +    ;;
>> +  (aarch64)
>> +    edk2_arch=AARCH64
>> +    gcc_arch=aarch64
>> +    ;;
>> +  (i386)
>> +    edk2_arch=IA32
>> +    gcc_arch=i686
>> +    ;;
>> +  (x86_64)
>> +    edk2_arch=X64
>> +    gcc_arch=x86_64
>> +    ;;
>> +  (*)
>> +    printf '%s: unknown/unsupported QEMU system emulation target "%s"\n' \
>> +      "$program_name" "$emulation_target" >&2
>> +    exit 1
>> +    ;;
>> +esac
>> +
>> +# Check if cross-compilation is needed.
>> +host_arch=$(uname -m)
>> +if [ "$gcc_arch" == "$host_arch" ] ||
>> +   ( [ "$gcc_arch" == i686 ] && [ "$host_arch" == x86_64 ] ); then
>> +  cross_prefix=
>> +else
>> +  cross_prefix=${gcc_arch}-linux-gnu-
>> +fi
>> +
>> +# Expose cross_prefix (which is possibly empty) to the edk2 tools. While at it,
>> +# determine the suitable edk2 toolchain as well.
>> +# - For ARM and AARCH64, edk2 only offers the GCC5 toolchain tag, which covers
>> +#   the gcc-5+ releases.
>> +# - For IA32 and X64, edk2 offers the GCC44 through GCC49 toolchain tags, in
>> +#   addition to GCC5. Unfortunately, the mapping between the toolchain tags and
>> +#   the actual gcc releases isn't entirely trivial. Run "git-blame" on
>> +#   "OvmfPkg/build.sh" in edk2 for more information.
>> +# And, because the above is too simple, we have to assign cross_prefix to an
>> +# edk2 build variable that is specific to both the toolchain tag and the target
>> +# architecture.
>> +case "$edk2_arch" in
>> +  (ARM)
>> +    edk2_toolchain=GCC5
>> +    export GCC5_ARM_PREFIX=$cross_prefix
>> +    ;;
>> +  (AARCH64)
>> +    edk2_toolchain=GCC5
>> +    export GCC5_AARCH64_PREFIX=$cross_prefix
>> +    ;;
>> +  (IA32|X64)
>> +    gcc_version=$("${cross_prefix}gcc" -v 2>&1 | tail -1 | awk '{print $3}')
>> +    case "$gcc_version" in
>> +      ([1-3].*|4.[0-3].*)
>> +        printf '%s: unsupported gcc version "%s"\n' \
>> +          "$program_name" "$gcc_version" >&2
>> +        exit 1
>> +        ;;
>> +      (4.4.*)
>> +        edk2_toolchain=GCC44
>> +        ;;
>> +      (4.5.*)
>> +        edk2_toolchain=GCC45
>> +        ;;
>> +      (4.6.*)
>> +        edk2_toolchain=GCC46
>> +        ;;
>> +      (4.7.*)
>> +        edk2_toolchain=GCC47
>> +        ;;
>> +      (4.8.*)
>> +        edk2_toolchain=GCC48
>> +        ;;
>> +      (4.9.*|6.[0-2].*)
>> +        edk2_toolchain=GCC49
>> +        ;;
>> +      (*)
>> +        edk2_toolchain=GCC5
>> +        ;;
>> +    esac
>> +    eval "export ${edk2_toolchain}_BIN=\$cross_prefix"
>> +    ;;
>> +esac
>> +
>> +# Build the UEFI binary
>> +mkdir -p log
>> +build \
>> +  --arch="$edk2_arch" \
>> +  --buildtarget=DEBUG \
>> +  --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
>> +  --tagname="$edk2_toolchain" \
>> +  --module="UefiTestToolsPkg/$dsc_component/$dsc_component.inf" \
>> +  --log="log/$dsc_component.$edk2_arch.log" \
>> +  --report-file="log/$dsc_component.$edk2_arch.report"
>> +cp -a -- \
>> +  "Build/UefiTestTools/DEBUG_${edk2_toolchain}/$edk2_arch/$dsc_component.efi" \
>> +  "$uefi_binary"
>>
Laszlo Ersek Jan. 31, 2019, 7:58 p.m. UTC | #3
On 01/31/19 19:55, Laszlo Ersek wrote:

> (4) IMPORTANT: did you make sure you didn't have an earlier *mis-built*
> BaseTools directory in the submodule? For that, run "make clean" in the
> QEMU project root.

Sorry, thinko above; the command to run is

  make -C tests/uefi-test-tools clean

> Note that "git clean -ffdx" will *not* clean
> submodules, when issued from the QEMU project root.

Thanks
Laszlo
Laszlo Ersek Feb. 1, 2019, 10:35 p.m. UTC | #4
On 01/31/19 19:55, Laszlo Ersek wrote:
> On 01/31/19 18:07, Philippe Mathieu-Daudé wrote:
>> On 1/24/19 9:39 PM, Laszlo Ersek wrote:

>>> +# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given edk2
>>> +# workspace, at most one "build" instance may be operating at a time. Therefore
>>> +# we must serialize the rebuilding of targets in this Makefile.
>>> +.NOTPARALLEL:
>>
>> Well this doesn't seem to improve my case :|
>>
>> You can test with:
>>
>> $ alias make='make -j8 -l7.5'
>>
>> So you get:
>>
>> $ make print-MAKEFLAGS
>> MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4
>
> I get something different (with make-3.82-23.el7.x86_64):
>
> MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j
>
> Notably, the argument 8 of "-j" is dropped.
>
> Anyway, I think this is a side topic.
>
>
>> and this command fails:
>>
>> $ make -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi
>
> (1) How *exactly* does it fail for you?
>
> (2) What operating system and "make" are you using?
>
> Because, the command completes perfectly fine for me. (Just for this
> test, I rebased the v2 branch on top of current master, i.e. commit
> aefcd2836620, and re-tested there.)
>
> (3) Also, did you update all submodules first? (git submodule update
> --init --force)
>
> (4) IMPORTANT: did you make sure you didn't have an earlier
> *mis-built* BaseTools directory in the submodule? For that, run "make
> clean" in the QEMU project root. Note that "git clean -ffdx" will
> *not* clean submodules, when issued from the QEMU project root.

I've found the issue myself, through using an up to date Fedora 29 guest
for the build.

(a) On RHEL7, I got "make-3.82-23.el7.x86_64". On Fedora 29, it's
"make-4.2.1-10.fc29.x86_64". Note in particular "4.2", on Fedora.

(b) In the earlier discussion on help-make
<http://lists.gnu.org/archive/html/help-make/2019-01/msg00004.html>,
Paul Smith responded to me,

On 01/23/19 14:45, Paul Smith wrote:
> On Wed, 2019-01-23 at 11:24 +0100, Laszlo Ersek wrote:
>> In particular, "--jobserver-fds" is not documented in end-user
>> documentation, apparently, so I get a feeling this option could
>> change at any time, as an implementation detail of distributing jobs.
>
> In fact it DID change, in GNU make 4.2, to be --jobserver-auth.  At
> the same time this new option was published in the documentation and
> made an official part of the GNU make interface.

(c) That is, the version bump from RHEL7's make to Fedora29's make
triggered, in our testing, this change. This explains why

  make print-MAKEFLAGS

printed

  MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4

for you, and returned

  MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j

for me.

(d) The build *indeed* fails on Fedora29, with the following obscure
error:

> make[1]: *** read jobs pipe: Bad file descriptor.  Stop.
> make[1]: *** Waiting for unfinished jobs....

(e) The documentation that Paul referenced, after I found it, provided
the critical hint.

https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html

> If your tool determines that the --jobserver-auth option is available
> in MAKEFLAGS but that the file descriptors specified are closed, this
> means that the calling make process did not think that your tool was a
> recursive make invocation (e.g., the command line was not prefixed
> with a + character). [...]

This means that the inner (= edk2's) make process inherits the
"--jobserver-auth=3,4" option from the outer (= qemu's) make process,
through the MAKEFLAGS variable. However, it does not inherit the file
descriptors themselves! And the reason is that the outer make does not
believe that the recipe that invokes "build.sh" is in fact an (indirect)
recursive make invocation. Therefore the outer make never bothers
passing down the file descriptors for the jobserver pipe.

The documentation writes,

> [...] only command lines that make understands to be recursive
> invocations of make [...] will have access to the jobserver. When
> writing makefiles you must be sure to mark the command as recursive
> (most commonly by prefixing the command line with the + indicator
> [...]

(Also:

> Using the MAKE variable has the same effect as using a '+' character
> at the beginning of the recipe line.

This is why an explicit "+" is needed only infrequently; most recursive
invocations are performed via $(MAKE), and the outer make recognizes
that automatically).

(f) So, the solution is to prefix the "./build.sh" recipe with a "+"
sign, to mark it as "recursive":

> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
> index 61d263861e..449b81d8ba 100644
> --- a/tests/uefi-test-tools/Makefile
> +++ b/tests/uefi-test-tools/Makefile
> @@ -87,7 +87,7 @@ Build/%.fat: Build/%.efi
>  .NOTPARALLEL:
>
>  Build/bios-tables-test.%.efi: build-edk2-tools
> -	./build.sh $(edk2_dir) BiosTablesTest $* $@
> +	+./build.sh $(edk2_dir) BiosTablesTest $* $@
>
>  build-edk2-tools:
>  	$(MAKE) -C $(edk2_dir)/BaseTools

This fixes the issue for me on Fedora 29, without breaking the behavior
on RHEL7.

I'll submit v3 later. Thank you for catching this error.

Laszlo
Philippe Mathieu-Daudé Feb. 1, 2019, 11:03 p.m. UTC | #5
On 2/1/19 11:35 PM, Laszlo Ersek wrote:
> On 01/31/19 19:55, Laszlo Ersek wrote:
>> On 01/31/19 18:07, Philippe Mathieu-Daudé wrote:
>>> On 1/24/19 9:39 PM, Laszlo Ersek wrote:
> 
>>>> +# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given edk2
>>>> +# workspace, at most one "build" instance may be operating at a time. Therefore
>>>> +# we must serialize the rebuilding of targets in this Makefile.
>>>> +.NOTPARALLEL:
>>>
>>> Well this doesn't seem to improve my case :|
>>>
>>> You can test with:
>>>
>>> $ alias make='make -j8 -l7.5'
>>>
>>> So you get:
>>>
>>> $ make print-MAKEFLAGS
>>> MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4
>>
>> I get something different (with make-3.82-23.el7.x86_64):
>>
>> MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j
>>
>> Notably, the argument 8 of "-j" is dropped.
>>
>> Anyway, I think this is a side topic.
>>
>>
>>> and this command fails:
>>>
>>> $ make -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi
>>
>> (1) How *exactly* does it fail for you?

In my mailbox the mail appears with Message-ID:
cf693646-58c8-8810-58a1-a6e503636d39@redhat.com in response to
https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06393.html but
I can't see the mail on the public archives :/

>>
>> (2) What operating system and "make" are you using?
>>
>> Because, the command completes perfectly fine for me. (Just for this
>> test, I rebased the v2 branch on top of current master, i.e. commit
>> aefcd2836620, and re-tested there.)
>>
>> (3) Also, did you update all submodules first? (git submodule update
>> --init --force)
>>
>> (4) IMPORTANT: did you make sure you didn't have an earlier
>> *mis-built* BaseTools directory in the submodule? For that, run "make
>> clean" in the QEMU project root. Note that "git clean -ffdx" will
>> *not* clean submodules, when issued from the QEMU project root.
> 
> I've found the issue myself, through using an up to date Fedora 29 guest
> for the build.
> 
> (a) On RHEL7, I got "make-3.82-23.el7.x86_64". On Fedora 29, it's
> "make-4.2.1-10.fc29.x86_64". Note in particular "4.2", on Fedora.
> 
> (b) In the earlier discussion on help-make
> <http://lists.gnu.org/archive/html/help-make/2019-01/msg00004.html>,
> Paul Smith responded to me,
> 
> On 01/23/19 14:45, Paul Smith wrote:
>> On Wed, 2019-01-23 at 11:24 +0100, Laszlo Ersek wrote:
>>> In particular, "--jobserver-fds" is not documented in end-user
>>> documentation, apparently, so I get a feeling this option could
>>> change at any time, as an implementation detail of distributing jobs.
>>
>> In fact it DID change, in GNU make 4.2, to be --jobserver-auth.  At
>> the same time this new option was published in the documentation and
>> made an official part of the GNU make interface.
> 
> (c) That is, the version bump from RHEL7's make to Fedora29's make
> triggered, in our testing, this change. This explains why
> 
>   make print-MAKEFLAGS
> 
> printed
> 
>   MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4
> 
> for you, and returned
> 
>   MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j
> 
> for me.
> 
> (d) The build *indeed* fails on Fedora29, with the following obscure
> error:
> 
>> make[1]: *** read jobs pipe: Bad file descriptor.  Stop.
>> make[1]: *** Waiting for unfinished jobs....

Yes, this is the problem.

> 
> (e) The documentation that Paul referenced, after I found it, provided
> the critical hint.
> 
> https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html
> 
>> If your tool determines that the --jobserver-auth option is available
>> in MAKEFLAGS but that the file descriptors specified are closed, this
>> means that the calling make process did not think that your tool was a
>> recursive make invocation (e.g., the command line was not prefixed
>> with a + character). [...]

o_O
Cc'ing Paolo and Eric who seems to like that kind of makefile black magic.

> This means that the inner (= edk2's) make process inherits the
> "--jobserver-auth=3,4" option from the outer (= qemu's) make process,
> through the MAKEFLAGS variable. However, it does not inherit the file
> descriptors themselves! And the reason is that the outer make does not
> believe that the recipe that invokes "build.sh" is in fact an (indirect)
> recursive make invocation. Therefore the outer make never bothers
> passing down the file descriptors for the jobserver pipe.

Yes!

> 
> The documentation writes,
> 
>> [...] only command lines that make understands to be recursive
>> invocations of make [...] will have access to the jobserver. When
>> writing makefiles you must be sure to mark the command as recursive
>> (most commonly by prefixing the command line with the + indicator
>> [...]
> 
> (Also:
> 
>> Using the MAKE variable has the same effect as using a '+' character
>> at the beginning of the recipe line.
> 
> This is why an explicit "+" is needed only infrequently; most recursive
> invocations are performed via $(MAKE), and the outer make recognizes
> that automatically).
> 
> (f) So, the solution is to prefix the "./build.sh" recipe with a "+"
> sign, to mark it as "recursive":
> 
>> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
>> index 61d263861e..449b81d8ba 100644
>> --- a/tests/uefi-test-tools/Makefile
>> +++ b/tests/uefi-test-tools/Makefile
>> @@ -87,7 +87,7 @@ Build/%.fat: Build/%.efi
>>  .NOTPARALLEL:
>>
>>  Build/bios-tables-test.%.efi: build-edk2-tools
>> -	./build.sh $(edk2_dir) BiosTablesTest $* $@
>> +	+./build.sh $(edk2_dir) BiosTablesTest $* $@

Yes :) This fixed it!

>>
>>  build-edk2-tools:
>>  	$(MAKE) -C $(edk2_dir)/BaseTools
> 
> This fixes the issue for me on Fedora 29, without breaking the behavior
> on RHEL7.

Excellent! I triggered Travis builds (Ubuntu/Debian).

> 
> I'll submit v3 later. Thank you for catching this error.

Now that we are happy, maybe Michael can do this change when applying,
but I guess you'd prefer first to write a line about this '+' in the
commit message or the Makefile.

With the +:
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks a lot for figuring this out alone and fixing it!

Phil.
Laszlo Ersek Feb. 1, 2019, 11:27 p.m. UTC | #6
On 02/02/19 00:03, Philippe Mathieu-Daudé wrote:
> On 2/1/19 11:35 PM, Laszlo Ersek wrote:
>> On 01/31/19 19:55, Laszlo Ersek wrote:

>>> (1) How *exactly* does it fail for you?
> 
> In my mailbox the mail appears with Message-ID:
> cf693646-58c8-8810-58a1-a6e503636d39@redhat.com in response to
> https://lists.gnu.org/archive/html/qemu-devel/2019-01/msg06393.html but
> I can't see the mail on the public archives :/

Ouch. I've now searched my mailbox for the Message-ID above, and there's
no match. Too bad your email got lost.

>> (f) So, the solution is to prefix the "./build.sh" recipe with a "+"
>> sign, to mark it as "recursive":
>>
>>> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
>>> index 61d263861e..449b81d8ba 100644
>>> --- a/tests/uefi-test-tools/Makefile
>>> +++ b/tests/uefi-test-tools/Makefile
>>> @@ -87,7 +87,7 @@ Build/%.fat: Build/%.efi
>>>  .NOTPARALLEL:
>>>
>>>  Build/bios-tables-test.%.efi: build-edk2-tools
>>> -	./build.sh $(edk2_dir) BiosTablesTest $* $@
>>> +	+./build.sh $(edk2_dir) BiosTablesTest $* $@
> 
> Yes :) This fixed it!
> 
>>>
>>>  build-edk2-tools:
>>>  	$(MAKE) -C $(edk2_dir)/BaseTools
>>
>> This fixes the issue for me on Fedora 29, without breaking the behavior
>> on RHEL7.
> 
> Excellent! I triggered Travis builds (Ubuntu/Debian).
> 
>>
>> I'll submit v3 later. Thank you for catching this error.
> 
> Now that we are happy, maybe Michael can do this change when applying,
> but I guess you'd prefer first to write a line about this '+' in the
> commit message or the Makefile.

That's right, I'd like to extend the comment that we already have in the
Makefile about .NOTPARALLEL, with a note on the "+" indicator.

> 
> With the +:
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Awesome, thank you! :)

> 
> Thanks a lot for figuring this out alone and fixing it!

It definitely helped that you mentioned your use of Fedora 29 earlier :)

Also, we're very lucky that GNU Make's documentation is so nice.

Thanks,
Laszlo
Philippe Mathieu-Daudé Feb. 3, 2019, 6:40 p.m. UTC | #7
On 2/2/19 12:27 AM, Laszlo Ersek wrote:
> On 02/02/19 00:03, Philippe Mathieu-Daudé wrote:
>> On 2/1/19 11:35 PM, Laszlo Ersek wrote:
>>> On 01/31/19 19:55, Laszlo Ersek wrote:
[...]
>>> (f) So, the solution is to prefix the "./build.sh" recipe with a "+"
>>> sign, to mark it as "recursive":
>>>
>>>> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
>>>> index 61d263861e..449b81d8ba 100644
>>>> --- a/tests/uefi-test-tools/Makefile
>>>> +++ b/tests/uefi-test-tools/Makefile
>>>> @@ -87,7 +87,7 @@ Build/%.fat: Build/%.efi
>>>>  .NOTPARALLEL:
>>>>
>>>>  Build/bios-tables-test.%.efi: build-edk2-tools
>>>> -	./build.sh $(edk2_dir) BiosTablesTest $* $@
>>>> +	+./build.sh $(edk2_dir) BiosTablesTest $* $@
>>
>> Yes :) This fixed it!
>>
>>>>
>>>>  build-edk2-tools:
>>>>  	$(MAKE) -C $(edk2_dir)/BaseTools
>>>
>>> This fixes the issue for me on Fedora 29, without breaking the behavior
>>> on RHEL7.
>>
>> Excellent! I triggered Travis builds (Ubuntu/Debian).

FYI, everthing build fine there.

>>>
>>> I'll submit v3 later. Thank you for catching this error.
[...]
diff mbox series

Patch

diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
new file mode 100644
index 000000000000..61d263861e01
--- /dev/null
+++ b/tests/uefi-test-tools/Makefile
@@ -0,0 +1,97 @@ 
+# Makefile for the test helper UEFI applications that run in guests.
+#
+# Copyright (C) 2019, Red Hat, Inc.
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License that accompanies this
+# distribution. The full text of the license may be found at
+# <http://opensource.org/licenses/bsd-license.php>.
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+edk2_dir              := ../../roms/edk2
+images_dir            := ../data/uefi-boot-images
+emulation_targets     := arm aarch64 i386 x86_64
+uefi_binaries         := bios-tables-test
+intermediate_suffixes := .efi .fat .iso.raw
+
+images: $(foreach binary,$(uefi_binaries), \
+		$(foreach target,$(emulation_targets), \
+			$(images_dir)/$(binary).$(target).iso.qcow2))
+
+# Preserve all intermediate targets if the build succeeds.
+# - Intermediate targets help with development & debugging.
+# - Preserving intermediate targets also keeps spurious changes out of the
+#   final build products, in case the user re-runs "make" without any changes
+#   to the UEFI source code. Normally, the intermediate files would have been
+#   removed by the last "make" invocation, hence the re-run would rebuild them
+#   from the unchanged UEFI sources. Unfortunately, the "mkdosfs" and
+#   "genisoimage" utilities embed timestamp-based information in their outputs,
+#   which causes git to report differences for the tracked qcow2 ISO images.
+.SECONDARY: $(foreach binary,$(uefi_binaries), \
+		$(foreach target,$(emulation_targets), \
+			$(foreach suffix,$(intermediate_suffixes), \
+				Build/$(binary).$(target)$(suffix))))
+
+# In the pattern rules below, the stem (%, $*) stands for
+# "$(binary).$(target)".
+
+# Convert the raw ISO image to a qcow2 one, enabling compression, and using a
+# small cluster size. This allows for small binary files under git control,
+# hence for small binary patches.
+$(images_dir)/%.iso.qcow2: Build/%.iso.raw
+	mkdir -p -- $(images_dir)
+	$${QTEST_QEMU_IMG:-qemu-img} convert -f raw -O qcow2 -c \
+		-o cluster_size=512 -- $< $@
+
+# Embed the "UEFI system partition" into an ISO9660 file system as an ElTorito
+# boot image.
+Build/%.iso.raw: Build/%.fat
+	genisoimage -input-charset ASCII -efi-boot $(notdir $<) -no-emul-boot \
+		-quiet -o $@ -- $<
+
+# Define chained macros in order to map QEMU system emulation targets to
+# *short* UEFI architecture identifiers. Periods are allowed in, and ultimately
+# stripped from, the argument.
+map_arm_to_uefi     = $(subst arm,ARM,$(1))
+map_aarch64_to_uefi = $(subst aarch64,AA64,$(call map_arm_to_uefi,$(1)))
+map_i386_to_uefi    = $(subst i386,IA32,$(call map_aarch64_to_uefi,$(1)))
+map_x86_64_to_uefi  = $(subst x86_64,X64,$(call map_i386_to_uefi,$(1)))
+map_to_uefi         = $(subst .,,$(call map_x86_64_to_uefi,$(1)))
+
+# Format a "UEFI system partition", using the UEFI binary as the default boot
+# loader. Add 10% size for filesystem metadata, round up to the next KB, and
+# make sure the size is large enough for a FAT filesystem. Name the filesystem
+# after the UEFI binary. (Excess characters are automatically dropped from the
+# filesystem label.)
+Build/%.fat: Build/%.efi
+	rm -f -- $@
+	uefi_bin_b=$$(stat --format=%s -- $<) && \
+		uefi_fat_kb=$$(( (uefi_bin_b * 11 / 10 + 1023) / 1024 )) && \
+		uefi_fat_kb=$$(( uefi_fat_kb >= 64 ? uefi_fat_kb : 64 )) && \
+		mkdosfs -C $@ -n $(basename $(@F)) -- $$uefi_fat_kb
+	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI
+	MTOOLS_SKIP_CHECK=1 mmd -i $@ ::EFI/BOOT
+	MTOOLS_SKIP_CHECK=1 mcopy -i $@ -- $< \
+		::EFI/BOOT/BOOT$(call map_to_uefi,$(suffix $*)).EFI
+
+# In the pattern rules below, the stem (%, $*) stands for "$(target)" only. The
+# association between the UEFI binary (such as "bios-tables-test") and the
+# component name from the edk2 platform DSC file (such as "BiosTablesTest") is
+# explicit in each rule.
+
+# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given edk2
+# workspace, at most one "build" instance may be operating at a time. Therefore
+# we must serialize the rebuilding of targets in this Makefile.
+.NOTPARALLEL:
+
+Build/bios-tables-test.%.efi: build-edk2-tools
+	./build.sh $(edk2_dir) BiosTablesTest $* $@
+
+build-edk2-tools:
+	$(MAKE) -C $(edk2_dir)/BaseTools
+
+clean:
+	rm -rf Build Conf log
+	$(MAKE) -C $(edk2_dir)/BaseTools clean
diff --git a/tests/uefi-test-tools/.gitignore b/tests/uefi-test-tools/.gitignore
new file mode 100644
index 000000000000..9f246701dea1
--- /dev/null
+++ b/tests/uefi-test-tools/.gitignore
@@ -0,0 +1,3 @@ 
+Build
+Conf
+log
diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
new file mode 100755
index 000000000000..155cb75c4ddb
--- /dev/null
+++ b/tests/uefi-test-tools/build.sh
@@ -0,0 +1,145 @@ 
+#!/bin/bash
+
+# Build script that determines the edk2 toolchain to use, invokes the edk2
+# "build" utility, and copies the built UEFI binary to the requested location.
+#
+# Copyright (C) 2019, Red Hat, Inc.
+#
+# This program and the accompanying materials are licensed and made available
+# under the terms and conditions of the BSD License that accompanies this
+# distribution. The full text of the license may be found at
+# <http://opensource.org/licenses/bsd-license.php>.
+#
+# THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT
+# WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED.
+
+set -e -u -C
+
+# Save the command line arguments. We need to reset $# to 0 before sourcing
+# "edksetup.sh", as it will inherit $@.
+program_name=$(basename -- "$0")
+edk2_dir=$1
+dsc_component=$2
+emulation_target=$3
+uefi_binary=$4
+shift 4
+
+# Set up the environment for edk2 building.
+export PACKAGES_PATH=$(realpath -- "$edk2_dir")
+export WORKSPACE=$PWD
+mkdir -p Conf
+
+# Source "edksetup.sh" carefully.
+set +e +u +C
+source "$PACKAGES_PATH/edksetup.sh"
+ret=$?
+set -e -u -C
+if [ $ret -ne 0 ]; then
+  exit $ret
+fi
+
+# Map the QEMU system emulation target to the following types of architecture
+# identifiers:
+# - edk2,
+# - gcc cross-compilation.
+# Cover only those targets that are supported by the UEFI spec and edk2.
+case "$emulation_target" in
+  (arm)
+    edk2_arch=ARM
+    gcc_arch=arm
+    ;;
+  (aarch64)
+    edk2_arch=AARCH64
+    gcc_arch=aarch64
+    ;;
+  (i386)
+    edk2_arch=IA32
+    gcc_arch=i686
+    ;;
+  (x86_64)
+    edk2_arch=X64
+    gcc_arch=x86_64
+    ;;
+  (*)
+    printf '%s: unknown/unsupported QEMU system emulation target "%s"\n' \
+      "$program_name" "$emulation_target" >&2
+    exit 1
+    ;;
+esac
+
+# Check if cross-compilation is needed.
+host_arch=$(uname -m)
+if [ "$gcc_arch" == "$host_arch" ] ||
+   ( [ "$gcc_arch" == i686 ] && [ "$host_arch" == x86_64 ] ); then
+  cross_prefix=
+else
+  cross_prefix=${gcc_arch}-linux-gnu-
+fi
+
+# Expose cross_prefix (which is possibly empty) to the edk2 tools. While at it,
+# determine the suitable edk2 toolchain as well.
+# - For ARM and AARCH64, edk2 only offers the GCC5 toolchain tag, which covers
+#   the gcc-5+ releases.
+# - For IA32 and X64, edk2 offers the GCC44 through GCC49 toolchain tags, in
+#   addition to GCC5. Unfortunately, the mapping between the toolchain tags and
+#   the actual gcc releases isn't entirely trivial. Run "git-blame" on
+#   "OvmfPkg/build.sh" in edk2 for more information.
+# And, because the above is too simple, we have to assign cross_prefix to an
+# edk2 build variable that is specific to both the toolchain tag and the target
+# architecture.
+case "$edk2_arch" in
+  (ARM)
+    edk2_toolchain=GCC5
+    export GCC5_ARM_PREFIX=$cross_prefix
+    ;;
+  (AARCH64)
+    edk2_toolchain=GCC5
+    export GCC5_AARCH64_PREFIX=$cross_prefix
+    ;;
+  (IA32|X64)
+    gcc_version=$("${cross_prefix}gcc" -v 2>&1 | tail -1 | awk '{print $3}')
+    case "$gcc_version" in
+      ([1-3].*|4.[0-3].*)
+        printf '%s: unsupported gcc version "%s"\n' \
+          "$program_name" "$gcc_version" >&2
+        exit 1
+        ;;
+      (4.4.*)
+        edk2_toolchain=GCC44
+        ;;
+      (4.5.*)
+        edk2_toolchain=GCC45
+        ;;
+      (4.6.*)
+        edk2_toolchain=GCC46
+        ;;
+      (4.7.*)
+        edk2_toolchain=GCC47
+        ;;
+      (4.8.*)
+        edk2_toolchain=GCC48
+        ;;
+      (4.9.*|6.[0-2].*)
+        edk2_toolchain=GCC49
+        ;;
+      (*)
+        edk2_toolchain=GCC5
+        ;;
+    esac
+    eval "export ${edk2_toolchain}_BIN=\$cross_prefix"
+    ;;
+esac
+
+# Build the UEFI binary
+mkdir -p log
+build \
+  --arch="$edk2_arch" \
+  --buildtarget=DEBUG \
+  --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
+  --tagname="$edk2_toolchain" \
+  --module="UefiTestToolsPkg/$dsc_component/$dsc_component.inf" \
+  --log="log/$dsc_component.$edk2_arch.log" \
+  --report-file="log/$dsc_component.$edk2_arch.report"
+cp -a -- \
+  "Build/UefiTestTools/DEBUG_${edk2_toolchain}/$edk2_arch/$dsc_component.efi" \
+  "$uefi_binary"