Re: [PATCH 2/3] gitlab-ci.yml: Add a job to build EDK2 firmware binaries

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 1/7/20 11:12 AM, Laszlo Ersek wrote:
> On 01/06/20 19:46, Philippe Mathieu-Daudé wrote:
>> Add a GitLab job to build the EDK2 firmware binaries.
>> This job is only built when the roms/edk2/ submodule is updated,
>> when a git-ref starts with 'edk2' or when the last commit contains
>> 'EDK2'.
> 
> keyword "or"; okay.
> 
>>
>> GitLab CI generates an artifacts.zip file containing the firmware
>> binaries.
>>
>> With edk2-stable201905, the job took 40 minutes 26 seconds,
>> the artifacts.zip takes 10MiB.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
>> ---
>>   .gitlab-ci-edk2.yml | 37 +++++++++++++++++++++++++++++++++++++
>>   .gitlab-ci.yml      |  3 +++
>>   MAINTAINERS         |  3 ++-
>>   3 files changed, 42 insertions(+), 1 deletion(-)
>>   create mode 100644 .gitlab-ci-edk2.yml
>>
>> diff --git a/.gitlab-ci-edk2.yml b/.gitlab-ci-edk2.yml
>> new file mode 100644
>> index 0000000000..abfaf52874
>> --- /dev/null
>> +++ b/.gitlab-ci-edk2.yml
>> @@ -0,0 +1,37 @@
>> +build-edk2:
>> + rules: # Only run this job when ...
>> + - changes: # ... roms/edk2/ is modified (submodule updated)
>> +   - roms/edk2/*
>> +   when: always
>> + - if: '$CI_COMMIT_REF_NAME =~ /^edk2/' # ... the branch/tag starts with 'edk2'
> 
> (1) can you add "or" in the comment here?
> 
>> +   when: always
>> + - if: '$CI_COMMIT_MESSAGE =~ /edk2/i' # last commit description contains 'EDK2'
> 
> (2) ditto
> 
>> +   when: always
>> + artifacts:
>> +   paths: # 'artifacts.zip' will contains the following files:
>> +   - pc-bios/edk2*bz2
>> +   - pc-bios/edk2-licenses.txt
>> +   - edk2-stdout.log
>> +   - edk2-stderr.log
>> + image: ubuntu:16.04 # Use Ubuntu Xenial
>> + before_script: # Install packages requiered to build EDK2
>> + - apt-get update --quiet --quiet
>> + - DEBIAN_FRONTEND=noninteractive
>> +   apt-get install --assume-yes --no-install-recommends --quiet --quiet
>> +     build-essential
>> +     ca-certificates
>> +     dos2unix
>> +     gcc-aarch64-linux-gnu
>> +     gcc-arm-linux-gnueabi
>> +     git
>> +     iasl
>> +     make
>> +     nasm
>> +     python
>> +     uuid-dev
>> + script: # Clone the required submodules and build EDK2
>> + - git submodule update --init roms/edk2
> 
> yes, this is needed; qemu users are used to updating top-level
> submodules (which is why we didn't try to automate that away in the edk2
> build stuff)
> 
>> + - git -C roms/edk2 submodule update --init
> 
> (3) but this should not be necessary. See the "submodules" target in
> "roms/Makefile.edk2".

Hmm build fails without it:
https://gitlab.com/philmd/qemu/-/jobs/395644357#L436

The 'test -d edk2/.git' might be not enough?

>> + - export JOBS=$(($(getconf _NPROCESSORS_ONLN) + 1))
>> + - echo "=== Using ${JOBS} simultaneous jobs ==="
>> + - make -j${JOBS} -C roms efi 1>edk2-stdout.log 2> >(tee -a edk2-stderr.log >&2)
> 
> Process substitution is a nifty feature, but perhaps we can do without
> it, for simplicity. (I realize this is bash-only; I just like to
> minimize the use of non-portable features if there is a portable
> replacement that is also simple.)
> 
> Redirections are processed in the order they appear on the command line
> [1], *after* stdout/stdin is redirected for pipelining [2]:
> 
> [1]
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_07
> 
> "If more than one redirection operator is specified with a command, the
> order of evaluation is from beginning to end."
> 
> [2]
> https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_02
> 
> "The standard input, standard output, or both of a command shall be
> considered to be assigned by the pipeline before any redirection
> specified by redirection operators that are part of the command"
> 
> 
> (4) Therefore, the following should work:
> 
>    make -j${JOBS} -C roms efi 2>&1 1>edk2-stdout.log \
>    | tee -a edk2-stderr.log >&2
> 
> Untested, of course :)

This works like charm :>

> Looks OK otherwise.

Thanks for the review!

> 
> Thanks!
> Laszlo