| Message ID | 20190613175435.6575-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | roms/edk2-build.sh: Allow to run edk2-build.sh from command line | expand |
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. >
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.
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. >
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
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.
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(-)