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

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

[Philippe Mathieu-Daudé]

The edk2-build.sh script set the 'nounset' option:

  BASH(1)

  set [arg ...]

      -u   Treat unset variables and parameters other than the
           special parameters "@" and "*" as an error when
           performing parameter expansion.  If expansion is
           attempted on an unset variable or parameter, the shell
           prints an error message, and, if not interactive,
           exits with a non-zero status.

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

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
@@ -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
+  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
 qemu_edk2_set_cross_env "$emulation_target"
 
 # Build the platform firmware.
-- 
2.20.1

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

[Philippe Mathieu-Daudé]

Cc'ing Eric :)

On 6/13/19 7:54 PM, Philippe Mathieu-Daudé wrote:
> The edk2-build.sh script set the 'nounset' option:
> 
>   BASH(1)
> 
>   set [arg ...]
> 
>       -u   Treat unset variables and parameters other than the
>            special parameters "@" and "*" as an error when
>            performing parameter expansion.  If expansion is
>            attempted on an unset variable or parameter, the shell
>            prints an error message, and, if not interactive,
>            exits with a non-zero status.
> 
> 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(-)
> 
> 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
> @@ -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
> +  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
>  qemu_edk2_set_cross_env "$emulation_target"
>  
>  # Build the platform firmware.
> 

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

[Eric Blake]

[-- Attachment #1.1: Type: text/plain, Size: 1785 bytes --]

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

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

at which point the really long comment needs a bit of a tweak.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Philippe Mathieu-Daudé]

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(-)
>>>
>>> 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
> 
> edk2_thread_count=$(qemu_edk2_get_thread_count "${MAKEFLAGS:-0}")

Argh I'm confuse, this is what I wanted to do first but I couldn't get
it working, maybe I forget the '-'.

Thanks a lot for your help, the result is way more elegant :)

> 
> at which point the really long comment needs a bit of a tweak.
> 

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

[Laszlo Ersek]

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