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

[Laszlo Ersek]

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

Thanks!
Laszlo