On 03/11/19 13:07, Eric Blake wrote:
> On 3/8/19 6:48 PM, Laszlo Ersek wrote:
>> Extract the dense logic for architecture and toolchain massaging from
>> "tests/uefi-test-tools/build.sh", to a set of small functions. We'll reuse
>> these functions for building full platform firmware images.
>>
>> Signed-off-by: Laszlo Ersek <ler...@redhat.com>
>> ---
>> roms/edk2-funcs.sh | 240 ++++++++++++++++++++
>> tests/uefi-test-tools/build.sh | 97 +-------
>> 2 files changed, 246 insertions(+), 91 deletions(-)
>>
>> diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
>> new file mode 100644
>> index 000000000000..908c7665c6ed
>> --- /dev/null
>> +++ b/roms/edk2-funcs.sh
>> @@ -0,0 +1,240 @@
>> +# Shell script that defines functions for determining some environmental
>
> No #! line, so this runs with /bin/sh, which might not be bash. However,...
>
>> +# characteristics for the edk2 "build" utility.
>> +#
>> +# This script is meant to be sourced.
>> +#
>> +# Copyright (C) 2019, Red Hat, Inc.
>
> I don't usually put a comma after the year.
>
>
>> +# Parameters:
>> +# $1: QEMU system emulation target
>> +qemu_edk2_verify_arch()
>> +{
>> + local emulation_target="$1"
>> + local program_name=$(basename -- "$0")
>
> ...local is a bashism, not present in all /bin/sh implementations. Then
> again...
>
>> +++ b/tests/uefi-test-tools/build.sh
>> @@ -38,97 +38,12 @@ if [ $ret -ne 0 ]; then
>
> build.sh is already marked as a bash script, and...
>
>> +# Fetch some option arguments, and set the cross-compilation environment (if
>> +# any), for the edk2 "build" utility.
>> +source "$edk2_dir/../edk2-funcs.sh"
>
> you are only ever sourcing the file, rather than directly executing it
> as a standalone script. I'd update the comments to mention that the file
> is meant to be sourced. (By the way, 'source' is also a bashism, the
> portable spelling is '.').
>
> So nothing jumped out at me as a potential shell script trap, although I
> did not closely review the logic.
>
Thanks! I'll drop the comma after the year.
Laszlo
