Re: [Qemu-devel] [PATCH 07/10] roms: build edk2 firmware binaries and variable store templates

[Philippe Mathieu-Daudé]

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é <address@hidden>
Tested-by: Philippe Mathieu-Daudé <address@hidden>

Regards,

Phil.