On 3/12/19 4:15 PM, Laszlo Ersek wrote: > On 03/11/19 13:09, Eric Blake wrote: >> On 3/10/19 10:10 AM, Philippe Mathieu-Daudé wrote: >>> Hi Laszlo, >>> >>> On 3/9/19 5:48 PM, Philippe Mathieu-Daudé wrote: >>>> On 3/9/19 1:48 AM, Laszlo Ersek wrote: >>>>> Add the "efi" target to "Makefile". >>>>> >> >>>>> + >>>>> +toolchain = $(shell source ./edk2-funcs.sh && qemu_edk2_get_toolchain >>>>> $(1)) >>> >>> Well I finally figured out why building on Ubuntu fails. It default >>> shell is dash, and 'source' is a bash builtin command. The portable >>> equivalent is '.' (dot). > > Yes, I'm aware: > > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_18 > > also, > > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01 > > If the command name matches the name of a utility listed in the > following table, the results are unspecified [...] /source/ > >>> The fix is: >>> >>> -- >8 -- >>> -toolchain = $(shell source ./edk2-funcs.sh && qemu_edk2_get_toolchain $(1)) >>> +toolchain = $(shell . ./edk2-funcs.sh && qemu_edk2_get_toolchain $(1)) >> >> Ouch - this changes my analysis in 1/10, where I argued that since the >> file was only ever sourced by a bash script, its use of 'local' was >> okay. Now that you are also sourcing it from /bin/sh via Makefile, you >> HAVE to make edk2-funcs.sh portable to POSIX shell, by eliminating use >> of 'local'. >> > > I'm acutely aware of portable vs. non-portable shell scripts. While > working on the code, I specifically checked that "local" was not > specified in SUSv4. The point is, I didn't care, because I didn't target > a POSIX system, but a GNU system. > > We already require GNU Make, to my knowledge. Perhaps not by decree, but > through the feature set that we use. > > ( > > For example, the extended description of the portable "Makefile" > language, at > > > http://pubs.opengroup.org/onlinepubs/9699919799/utilities/make.html#tag_20_76_13 > > doesn't even mention $(shell), or other parametrizable macros. > > ) > > I really want to keep "local", as it keeps the shell variable namespace > clean. Regarding the functions that I did put in the "global namespace", > I was careful to prefix them suitably. > > IMO, it's perfectly fine to require the shell to be bash here, given > that this feature is meant for a subset of maintainers (and not for > end-users), and that building edk2 already requires a quite sizeable set > of packages installed (such as nasm, iasl, ...) So this feature is not > meant for any random POSIX system. > > What I did miss in fact was that "GNU Make" didn't imply "/bin/bash": > > https://www.gnu.org/software/make/manual/make.html#Choosing-the-Shell > > The program used as the shell is taken from the variable SHELL. If > this variable is not set in your makefile, the program /bin/sh is > used as the shell. > > Thus, the real fix here is to be explicit about the bash requirement. I > should add > > SHELL=/bin/bash > > near the top of "Makefile.edk2". > > (The GNU make documentation also provides examples with > SHELL=/usr/bin/perl, so I think SHELL=/bin/bash should be safe, > generally speaking.) > > > Phil: can you please test whether SHELL=/bin/bash works for you on > Ubuntu? (After you install "bash", of course.)
It indeed worked using: -- >8 -- @@ -11,6 +11,8 @@ # THE PROGRAM IS DISTRIBUTED UNDER THE BSD LICENSE ON AN "AS IS" BASIS, WITHOUT # WARRANTIES OR REPRESENTATIONS OF ANY KIND, EITHER EXPRESS OR IMPLIED. +SHELL = /bin/bash + toolchain = $(shell source ./edk2-funcs.sh && qemu_edk2_get_toolchain $(1)) --- With this fixup: Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com> Tested-by: Philippe Mathieu-Daudé <phi...@redhat.com> Regards, Phil.
