Re: [Qemu-devel] [PATCH v2 4/5] tests/uefi-test-tools: add build scripts

[Laszlo Ersek]

On 01/31/19 19:55, Laszlo Ersek wrote:
> On 01/31/19 18:07, Philippe Mathieu-Daudé wrote:
>> On 1/24/19 9:39 PM, Laszlo Ersek wrote:

>>> +# "build.sh" invokes the "build" utility of edk2 BaseTools. In any given 
>>> edk2
>>> +# workspace, at most one "build" instance may be operating at a time. 
>>> Therefore
>>> +# we must serialize the rebuilding of targets in this Makefile.
>>> +.NOTPARALLEL:
>>
>> Well this doesn't seem to improve my case :|
>>
>> You can test with:
>>
>> $ alias make='make -j8 -l7.5'
>>
>> So you get:
>>
>> $ make print-MAKEFLAGS
>> MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4
>
> I get something different (with make-3.82-23.el7.x86_64):
>
> MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j
>
> Notably, the argument 8 of "-j" is dropped.
>
> Anyway, I think this is a side topic.
>
>
>> and this command fails:
>>
>> $ make -C tests/uefi-test-tools Build/bios-tables-test.x86_64.efi
>
> (1) How *exactly* does it fail for you?
>
> (2) What operating system and "make" are you using?
>
> Because, the command completes perfectly fine for me. (Just for this
> test, I rebased the v2 branch on top of current master, i.e. commit
> aefcd2836620, and re-tested there.)
>
> (3) Also, did you update all submodules first? (git submodule update
> --init --force)
>
> (4) IMPORTANT: did you make sure you didn't have an earlier
> *mis-built* BaseTools directory in the submodule? For that, run "make
> clean" in the QEMU project root. Note that "git clean -ffdx" will
> *not* clean submodules, when issued from the QEMU project root.

I've found the issue myself, through using an up to date Fedora 29 guest
for the build.

(a) On RHEL7, I got "make-3.82-23.el7.x86_64". On Fedora 29, it's
"make-4.2.1-10.fc29.x86_64". Note in particular "4.2", on Fedora.

(b) In the earlier discussion on help-make
<http://lists.gnu.org/archive/html/help-make/2019-01/msg00004.html>,
Paul Smith responded to me,

On 01/23/19 14:45, Paul Smith wrote:
> On Wed, 2019-01-23 at 11:24 +0100, Laszlo Ersek wrote:
>> In particular, "--jobserver-fds" is not documented in end-user
>> documentation, apparently, so I get a feeling this option could
>> change at any time, as an implementation detail of distributing jobs.
>
> In fact it DID change, in GNU make 4.2, to be --jobserver-auth.  At
> the same time this new option was published in the documentation and
> made an official part of the GNU make interface.

(c) That is, the version bump from RHEL7's make to Fedora29's make
triggered, in our testing, this change. This explains why

  make print-MAKEFLAGS

printed

  MAKEFLAGS=rR -j8 -l7.5 --jobserver-auth=3,4

for you, and returned

  MAKEFLAGS=Rrl 7.5 - --jobserver-fds=3,4 -j

for me.

(d) The build *indeed* fails on Fedora29, with the following obscure
error:

> make[1]: *** read jobs pipe: Bad file descriptor.  Stop.
> make[1]: *** Waiting for unfinished jobs....

(e) The documentation that Paul referenced, after I found it, provided
the critical hint.

https://www.gnu.org/software/make/manual/html_node/POSIX-Jobserver.html

> If your tool determines that the --jobserver-auth option is available
> in MAKEFLAGS but that the file descriptors specified are closed, this
> means that the calling make process did not think that your tool was a
> recursive make invocation (e.g., the command line was not prefixed
> with a + character). [...]

This means that the inner (= edk2's) make process inherits the
"--jobserver-auth=3,4" option from the outer (= qemu's) make process,
through the MAKEFLAGS variable. However, it does not inherit the file
descriptors themselves! And the reason is that the outer make does not
believe that the recipe that invokes "build.sh" is in fact an (indirect)
recursive make invocation. Therefore the outer make never bothers
passing down the file descriptors for the jobserver pipe.

The documentation writes,

> [...] only command lines that make understands to be recursive
> invocations of make [...] will have access to the jobserver. When
> writing makefiles you must be sure to mark the command as recursive
> (most commonly by prefixing the command line with the + indicator
> [...]

(Also:

> Using the MAKE variable has the same effect as using a '+' character
> at the beginning of the recipe line.

This is why an explicit "+" is needed only infrequently; most recursive
invocations are performed via $(MAKE), and the outer make recognizes
that automatically).

(f) So, the solution is to prefix the "./build.sh" recipe with a "+"
sign, to mark it as "recursive":

> diff --git a/tests/uefi-test-tools/Makefile b/tests/uefi-test-tools/Makefile
> index 61d263861e..449b81d8ba 100644
> --- a/tests/uefi-test-tools/Makefile
> +++ b/tests/uefi-test-tools/Makefile
> @@ -87,7 +87,7 @@ Build/%.fat: Build/%.efi
>  .NOTPARALLEL:
>
>  Build/bios-tables-test.%.efi: build-edk2-tools
> -     ./build.sh $(edk2_dir) BiosTablesTest $* $@
> +     +./build.sh $(edk2_dir) BiosTablesTest $* $@
>
>  build-edk2-tools:
>       $(MAKE) -C $(edk2_dir)/BaseTools

This fixes the issue for me on Fedora 29, without breaking the behavior
on RHEL7.

I'll submit v3 later. Thank you for catching this error.

Laszlo