[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] roms/edk2-build.sh: Allow to run edk2-build.sh
From: |
Laszlo Ersek |
Subject: |
Re: [Qemu-devel] [PATCH] roms/edk2-build.sh: Allow to run edk2-build.sh from command line |
Date: |
Fri, 14 Jun 2019 19:35:06 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 |
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é <address@hidden>
>>>> ---
>>>> 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 <address@hidden>
Thanks
Laszlo