Re: [Qemu-devel] [PATCH] roms/edk2-build.sh: Allow to run edk2-build.sh from command line

[Laszlo Ersek <lersek@redhat.com>]

On 06/14/19 15:55, Philippe Mathieu-Daudé wrote:
> On 6/14/19 3:29 PM, Eric Blake wrote:
>> On 6/14/19 5:16 AM, Philippe Mathieu-Daudé wrote:
>>> Cc'ing Eric :)
>>>
>>
>>>> When running this script out of 'make', we get:
>>>>
>>>>   $ cd roms
>>>>   $ ./edk2-build.sh aarch64 --arch=AARCH64 --platform=ArmVirtPkg/ArmVirtQemu.dsc > /dev/null
>>>>   ./edk2-build.sh: line 46: MAKEFLAGS: unbound variable
>>>>
>>>> Fix this by checking the variable is defined before using it,
>>>> else use a default value.
>>>>
>>>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>>>> ---
>>>>  roms/edk2-build.sh | 8 +++++++-
>>>>  1 file changed, 7 insertions(+), 1 deletion(-)

(1) "allow to run" is not correct English, to my understanding. This
form of "allow" requires an object. You could reformulate the subject
line as "allow edk2-build.sh to be invoked from the command line".


>>>> diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
>>>> index 4f46f8a6a2..5390228b4e 100755
>>>> --- a/roms/edk2-build.sh
>>>> +++ b/roms/edk2-build.sh
>>
>> This is running under /bin/bash (hmm - not '/bin/env bash' like other
>> scripts in qemu?), so...
>>
>>>> @@ -43,7 +43,13 @@ fi
>>>>  # any), for the edk2 "build" utility.
>>>>  source ../edk2-funcs.sh
>>>>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
>>>> -edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>>>> +if [ -v MAKEFLAGS ]; then
>>
>> the non-portable bashism '[ -v' works. However, it's just as easy to
> 
> Ah, OK.
> 
>> work around this problem portably for all POSIX shells without needing 'if':
>>
>>>> +  edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>>>> +else
>>>> +  # We are not running within 'make', let the edk2 "build" utility to fetch
>>>> +  # the logical CPU count with Python's multiprocessing.cpu_count() method.
>>>> +  edk2_thread_count=0
>>>> +fi

(2) "let" doesn't take the preposition "to". I'd suggest:

  Let the edk2 "build" utility [] fetch ...


>>
>> edk2_thread_count=$(qemu_edk2_get_thread_count "${MAKEFLAGS:-0}")

(3) The expression

  edk2_thread_count=$(qemu_edk2_get_thread_count "${MAKEFLAGS:-0}")

would pass the string "0" as $1 to the qemu_edk2_get_thread_count()
function. That doesn't seem useful (please see the docs on said shell
function).

We could write

  edk2_thread_count=$(qemu_edk2_get_thread_count "${MAKEFLAGS:-}")

instead, to pass "". But that would cause qemu_edk2_get_thread_count()
to print "1", which is not what we want here, AIUI.


I think I prefer the approach with "[ -v". While it's nonportable,
"edk2-build.sh" is already -- consciously -- so: it uses an array
variable, for example.


(4) Phil, did you regression-test this change with plain "make" (i.e.,
no "-j" option)? The behavior shouldn't change for that case (i.e.
qemu_edk2_get_thread_count() should be invoked, and it should print "1").


With (1) and (2) fixed, and (4) confirmed:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Thanks
Laszlo