diff mbox series

edk2 build scripts: work around TianoCore#1607 without forcing Python 2

Message ID 20190918171141.15957-1-lersek@redhat.com
State New
Headers show
Series edk2 build scripts: work around TianoCore#1607 without forcing Python 2 | expand

Commit Message

Laszlo Ersek Sept. 18, 2019, 5:11 p.m. UTC
It turns out that forcing python2 for running the edk2 "build" utility is
neither necessary nor sufficient.

Forcing python2 is not sufficient for two reasons:

- QEMU is moving away from python2, with python2 nearing EOL,

- according to my most recent testing, the lacking dependency information
  in the makefiles that are generated by edk2's "build" utility can cause
  parallel build failures even when "build" is executed by python2.

And forcing python2 is not necessary because we can still return to the
original idea of filtering out jobserver-related options from MAKEFLAGS.
So do that.

With this patch, the guest UEFI binaries that are used as part of the BIOS
tables test, and the OVMF and ArmVirtQemu platform firmwares, will be
built strictly in a single job, regardless of an outermost "-jN" make
option. Alas, there appears to be no reliable way to build edk2 in an
(outer make, inner make) environment, with a jobserver enabled.

Cc: Eduardo Habkost <ehabkost@redhat.com>
Cc: John Snow <jsnow@redhat.com>
Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Laszlo Ersek <lersek@redhat.com>
---

Notes:
    - Tested on RHEL7 (where the outer "make" sets the old-style
      "--jobserver-fds" flag) and on Fedora 29 (where the outer "make" sets
      the new-style "--jobserver-auth" flag).
    
    - I've rebuilt all the edk2 binaries with this patch applied. Everything
      works fine. However, if you test this patch, you might notice that git
      reports all the build products as modified. That's because when using
      the python3 code in edk2 BaseTools, the generated makefiles differ
      greatly from the ones generated when running in python2 mode (e.g. due
      to different random seeds in python hashes / dictionaries). As a
      result, parts of the firmware volumes / firmware filesystems could
      appear in a different order than before. This is harmless, and doesn't
      necessitate checking in the rebuilt binaries.

 roms/edk2-build.sh             |  4 +---
 roms/edk2-funcs.sh             | 17 +++++++++++++++++
 tests/uefi-test-tools/build.sh |  6 +++---
 3 files changed, 21 insertions(+), 6 deletions(-)

Comments

John Snow Sept. 18, 2019, 10:29 p.m. UTC | #1
On 9/18/19 1:11 PM, Laszlo Ersek wrote:
> It turns out that forcing python2 for running the edk2 "build" utility is
> neither necessary nor sufficient.
> 
> Forcing python2 is not sufficient for two reasons:
> 
> - QEMU is moving away from python2, with python2 nearing EOL,
> 

Thank you :)

> - according to my most recent testing, the lacking dependency information
>    in the makefiles that are generated by edk2's "build" utility can cause
>    parallel build failures even when "build" is executed by python2.
> 
> And forcing python2 is not necessary because we can still return to the
> original idea of filtering out jobserver-related options from MAKEFLAGS.
> So do that.
> 
> With this patch, the guest UEFI binaries that are used as part of the BIOS
> tables test, and the OVMF and ArmVirtQemu platform firmwares, will be
> built strictly in a single job, regardless of an outermost "-jN" make
> option. Alas, there appears to be no reliable way to build edk2 in an
> (outer make, inner make) environment, with a jobserver enabled.
> 
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>

Looks good to me, given your explanation of the situation so far.

Reviewed-by: John Snow <jsnow@redhat.com>

> ---
> 
> Notes:
>      - Tested on RHEL7 (where the outer "make" sets the old-style
>        "--jobserver-fds" flag) and on Fedora 29 (where the outer "make" sets
>        the new-style "--jobserver-auth" flag).
>      
>      - I've rebuilt all the edk2 binaries with this patch applied. Everything
>        works fine. However, if you test this patch, you might notice that git
>        reports all the build products as modified. That's because when using
>        the python3 code in edk2 BaseTools, the generated makefiles differ
>        greatly from the ones generated when running in python2 mode (e.g. due
>        to different random seeds in python hashes / dictionaries). As a
>        result, parts of the firmware volumes / firmware filesystems could
>        appear in a different order than before. This is harmless, and doesn't
>        necessitate checking in the rebuilt binaries.
> 
>   roms/edk2-build.sh             |  4 +---
>   roms/edk2-funcs.sh             | 17 +++++++++++++++++
>   tests/uefi-test-tools/build.sh |  6 +++---
>   3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
> index 4f46f8a6a217..8161c55ef507 100755
> --- a/roms/edk2-build.sh
> +++ b/roms/edk2-build.sh
> @@ -27,9 +27,6 @@ shift $num_args
>   
>   cd edk2
>   
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>   # Source "edksetup.sh" carefully.
>   set +e +u +C
>   source ./edksetup.sh
> @@ -43,6 +40,7 @@ fi
>   # any), for the edk2 "build" utility.
>   source ../edk2-funcs.sh
>   edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
>   edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>   qemu_edk2_set_cross_env "$emulation_target"
>   
> diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
> index a9fae7ee891b..3f4485b201f1 100644
> --- a/roms/edk2-funcs.sh
> +++ b/roms/edk2-funcs.sh
> @@ -251,3 +251,20 @@ qemu_edk2_get_thread_count()
>       printf '1\n'
>     fi
>   }
> +
> +
> +# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607> by
> +# filtering jobserver-related flags out of MAKEFLAGS. Print the result to the
> +# standard output.
> +#
> +# Parameters:
> +#   $1: the value of the MAKEFLAGS variable
> +qemu_edk2_quirk_tianocore_1607()
> +{
> +  local makeflags="$1"
> +
> +  printf %s "$makeflags" \
> +  | LC_ALL=C sed --regexp-extended \
> +      --expression='s/--jobserver-(auth|fds)=[0-9]+,[0-9]+//' \
> +      --expression='s/-j([0-9]+)?//'
> +}
> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
> index 8aa7935c43bb..eba7964a163b 100755
> --- a/tests/uefi-test-tools/build.sh
> +++ b/tests/uefi-test-tools/build.sh
> @@ -29,9 +29,6 @@ export PACKAGES_PATH=$(realpath -- "$edk2_dir")
>   export WORKSPACE=$PWD
>   mkdir -p Conf
>   
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>   # Source "edksetup.sh" carefully.
>   set +e +u +C
>   source "$PACKAGES_PATH/edksetup.sh"
> @@ -46,12 +43,15 @@ fi
>   source "$edk2_dir/../edk2-funcs.sh"
>   edk2_arch=$(qemu_edk2_get_arch "$emulation_target")
>   edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
> +edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>   qemu_edk2_set_cross_env "$emulation_target"
>   
>   # Build the UEFI binary
>   mkdir -p log
>   build \
>     --arch="$edk2_arch" \
> +  -n "$edk2_thread_count" \
>     --buildtarget=DEBUG \
>     --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
>     --tagname="$edk2_toolchain" \
>
Philippe Mathieu-Daudé Sept. 19, 2019, 1:41 p.m. UTC | #2
Hi Laszlo,

On 9/18/19 7:11 PM, Laszlo Ersek wrote:
> It turns out that forcing python2 for running the edk2 "build" utility is
> neither necessary nor sufficient.
> 
> Forcing python2 is not sufficient for two reasons:
> 
> - QEMU is moving away from python2, with python2 nearing EOL,
> 
> - according to my most recent testing, the lacking dependency information
>   in the makefiles that are generated by edk2's "build" utility can cause
>   parallel build failures even when "build" is executed by python2.
> 
> And forcing python2 is not necessary because we can still return to the
> original idea of filtering out jobserver-related options from MAKEFLAGS.
> So do that.
> 
> With this patch, the guest UEFI binaries that are used as part of the BIOS
> tables test, and the OVMF and ArmVirtQemu platform firmwares, will be
> built strictly in a single job, regardless of an outermost "-jN" make
> option. Alas, there appears to be no reliable way to build edk2 in an
> (outer make, inner make) environment, with a jobserver enabled.
> 
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     - Tested on RHEL7 (where the outer "make" sets the old-style
>       "--jobserver-fds" flag) and on Fedora 29 (where the outer "make" sets
>       the new-style "--jobserver-auth" flag).
>     
>     - I've rebuilt all the edk2 binaries with this patch applied. Everything
>       works fine. However, if you test this patch, you might notice that git
>       reports all the build products as modified. That's because when using
>       the python3 code in edk2 BaseTools, the generated makefiles differ
>       greatly from the ones generated when running in python2 mode (e.g. due
>       to different random seeds in python hashes / dictionaries). As a
>       result, parts of the firmware volumes / firmware filesystems could
>       appear in a different order than before. This is harmless, and doesn't
>       necessitate checking in the rebuilt binaries.
> 
>  roms/edk2-build.sh             |  4 +---
>  roms/edk2-funcs.sh             | 17 +++++++++++++++++
>  tests/uefi-test-tools/build.sh |  6 +++---
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
> index 4f46f8a6a217..8161c55ef507 100755
> --- a/roms/edk2-build.sh
> +++ b/roms/edk2-build.sh
> @@ -27,9 +27,6 @@ shift $num_args
>  
>  cd edk2
>  
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>  # Source "edksetup.sh" carefully.
>  set +e +u +C
>  source ./edksetup.sh
> @@ -43,6 +40,7 @@ fi
>  # any), for the edk2 "build" utility.
>  source ../edk2-funcs.sh
>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
>  edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>  qemu_edk2_set_cross_env "$emulation_target"
>  
> diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
> index a9fae7ee891b..3f4485b201f1 100644
> --- a/roms/edk2-funcs.sh
> +++ b/roms/edk2-funcs.sh
> @@ -251,3 +251,20 @@ qemu_edk2_get_thread_count()
>      printf '1\n'
>    fi
>  }
> +
> +
> +# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607> by
> +# filtering jobserver-related flags out of MAKEFLAGS. Print the result to the
> +# standard output.
> +#
> +# Parameters:
> +#   $1: the value of the MAKEFLAGS variable
> +qemu_edk2_quirk_tianocore_1607()
> +{
> +  local makeflags="$1"
> +
> +  printf %s "$makeflags" \
> +  | LC_ALL=C sed --regexp-extended \
> +      --expression='s/--jobserver-(auth|fds)=[0-9]+,[0-9]+//' \
> +      --expression='s/-j([0-9]+)?//'
> +}
> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
> index 8aa7935c43bb..eba7964a163b 100755
> --- a/tests/uefi-test-tools/build.sh
> +++ b/tests/uefi-test-tools/build.sh
> @@ -29,9 +29,6 @@ export PACKAGES_PATH=$(realpath -- "$edk2_dir")
>  export WORKSPACE=$PWD
>  mkdir -p Conf
>  
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>  # Source "edksetup.sh" carefully.
>  set +e +u +C
>  source "$PACKAGES_PATH/edksetup.sh"
> @@ -46,12 +43,15 @@ fi
>  source "$edk2_dir/../edk2-funcs.sh"
>  edk2_arch=$(qemu_edk2_get_arch "$emulation_target")
>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
> +edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>  qemu_edk2_set_cross_env "$emulation_target"
>  
>  # Build the UEFI binary
>  mkdir -p log
>  build \
>    --arch="$edk2_arch" \
> +  -n "$edk2_thread_count" \
>    --buildtarget=DEBUG \
>    --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
>    --tagname="$edk2_toolchain" \
> 

Very clear explanation, thanks.
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Hmm this failed on Ubuntu Xenial which is the image we use for Travis-CI:

$ lsb_release -a
Distributor ID:	Ubuntu
Description:	Ubuntu 16.04.6 LTS
Release:	16.04
Codename:	xenial

make -f Makefile.edk2
make[1]: Entering directory '/home/phil/qemu/roms'
if test -d edk2/.git; then \
	cd edk2 && git submodule update --init --force; \
fi
./edk2-build.sh \
	aarch64 \
	--arch=AARCH64 \
	--platform=ArmVirtPkg/ArmVirtQemu.dsc \
	-D NETWORK_IP6_ENABLE \
	-D NETWORK_HTTP_BOOT_ENABLE
WORKSPACE: /home/phil/qemu/roms/edk2
EDK_TOOLS_PATH: /home/phil/qemu/roms/edk2/BaseTools
CONF_PATH: /home/phil/qemu/roms/edk2/Conf
Copying $EDK_TOOLS_PATH/Conf/build_rule.template
     to /home/phil/qemu/roms/edk2/Conf/build_rule.txt
Copying $EDK_TOOLS_PATH/Conf/tools_def.template
     to /home/phil/qemu/roms/edk2/Conf/tools_def.txt
Copying $EDK_TOOLS_PATH/Conf/target.template
     to /home/phil/qemu/roms/edk2/Conf/target.txt
pyenv: python3.7: command not found
The `python3.7' command exists in these Python versions:
  3.7
  3.7.1

Makefile.edk2:62: recipe for target '../pc-bios/edk2-aarch64-code.fd' failed
make[1]: *** [../pc-bios/edk2-aarch64-code.fd] Error 127
make[1]: Leaving directory '/home/phil/qemu/roms'
Makefile:168: recipe for target 'efi' failed
make: *** [efi] Error 2
make: Leaving directory '/home/phil/qemu/roms'
The command "make -C roms efi -j2" exited with 2.

The local Python3 version is:

$ apt-cache show python3-minimal
Package: python3-minimal
Version: 3.5.1-3

Any idea which script is choosing python3.7?
Philippe Mathieu-Daudé Sept. 19, 2019, 1:56 p.m. UTC | #3
Hi Bruce,

On 9/18/19 7:11 PM, Laszlo Ersek wrote:
> It turns out that forcing python2 for running the edk2 "build" utility is
> neither necessary nor sufficient.
> 
> Forcing python2 is not sufficient for two reasons:
> 
> - QEMU is moving away from python2, with python2 nearing EOL,
> 
> - according to my most recent testing, the lacking dependency information
>   in the makefiles that are generated by edk2's "build" utility can cause
>   parallel build failures even when "build" is executed by python2.
> 
> And forcing python2 is not necessary because we can still return to the
> original idea of filtering out jobserver-related options from MAKEFLAGS.
> So do that.
> 
> With this patch, the guest UEFI binaries that are used as part of the BIOS
> tables test, and the OVMF and ArmVirtQemu platform firmwares, will be
> built strictly in a single job, regardless of an outermost "-jN" make
> option. Alas, there appears to be no reliable way to build edk2 in an
> (outer make, inner make) environment, with a jobserver enabled.
> 
> Cc: Eduardo Habkost <ehabkost@redhat.com>
> Cc: John Snow <jsnow@redhat.com>
> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
> ---
> 
> Notes:
>     - Tested on RHEL7 (where the outer "make" sets the old-style
>       "--jobserver-fds" flag) and on Fedora 29 (where the outer "make" sets
>       the new-style "--jobserver-auth" flag).

Since I plan to queue this patch, do you mind testing this patch on your
distribution? I don't want to break your workflow.

Beware it creates ~3.5GiB of temporary data in roms/edk2/Build.

Thanks!

Meanwhile:
Tested-by: Philippe Mathieu-Daudé <philmd@redhat.com>

>     - I've rebuilt all the edk2 binaries with this patch applied. Everything
>       works fine. However, if you test this patch, you might notice that git
>       reports all the build products as modified. That's because when using
>       the python3 code in edk2 BaseTools, the generated makefiles differ
>       greatly from the ones generated when running in python2 mode (e.g. due
>       to different random seeds in python hashes / dictionaries). As a
>       result, parts of the firmware volumes / firmware filesystems could
>       appear in a different order than before. This is harmless, and doesn't
>       necessitate checking in the rebuilt binaries.
> 
>  roms/edk2-build.sh             |  4 +---
>  roms/edk2-funcs.sh             | 17 +++++++++++++++++
>  tests/uefi-test-tools/build.sh |  6 +++---
>  3 files changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
> index 4f46f8a6a217..8161c55ef507 100755
> --- a/roms/edk2-build.sh
> +++ b/roms/edk2-build.sh
> @@ -27,9 +27,6 @@ shift $num_args
>  
>  cd edk2
>  
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>  # Source "edksetup.sh" carefully.
>  set +e +u +C
>  source ./edksetup.sh
> @@ -43,6 +40,7 @@ fi
>  # any), for the edk2 "build" utility.
>  source ../edk2-funcs.sh
>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
>  edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>  qemu_edk2_set_cross_env "$emulation_target"
>  
> diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
> index a9fae7ee891b..3f4485b201f1 100644
> --- a/roms/edk2-funcs.sh
> +++ b/roms/edk2-funcs.sh
> @@ -251,3 +251,20 @@ qemu_edk2_get_thread_count()
>      printf '1\n'
>    fi
>  }
> +
> +
> +# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607> by
> +# filtering jobserver-related flags out of MAKEFLAGS. Print the result to the
> +# standard output.
> +#
> +# Parameters:
> +#   $1: the value of the MAKEFLAGS variable
> +qemu_edk2_quirk_tianocore_1607()
> +{
> +  local makeflags="$1"
> +
> +  printf %s "$makeflags" \
> +  | LC_ALL=C sed --regexp-extended \
> +      --expression='s/--jobserver-(auth|fds)=[0-9]+,[0-9]+//' \
> +      --expression='s/-j([0-9]+)?//'
> +}
> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
> index 8aa7935c43bb..eba7964a163b 100755
> --- a/tests/uefi-test-tools/build.sh
> +++ b/tests/uefi-test-tools/build.sh
> @@ -29,9 +29,6 @@ export PACKAGES_PATH=$(realpath -- "$edk2_dir")
>  export WORKSPACE=$PWD
>  mkdir -p Conf
>  
> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
> -export PYTHON_COMMAND=python2
> -
>  # Source "edksetup.sh" carefully.
>  set +e +u +C
>  source "$PACKAGES_PATH/edksetup.sh"
> @@ -46,12 +43,15 @@ fi
>  source "$edk2_dir/../edk2-funcs.sh"
>  edk2_arch=$(qemu_edk2_get_arch "$emulation_target")
>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
> +edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>  qemu_edk2_set_cross_env "$emulation_target"
>  
>  # Build the UEFI binary
>  mkdir -p log
>  build \
>    --arch="$edk2_arch" \
> +  -n "$edk2_thread_count" \
>    --buildtarget=DEBUG \
>    --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
>    --tagname="$edk2_toolchain" \
>
Philippe Mathieu-Daudé Sept. 19, 2019, 4:39 p.m. UTC | #4
On 9/18/19 7:11 PM, Laszlo Ersek wrote:
> It turns out that forcing python2 for running the edk2 "build" utility is
> neither necessary nor sufficient.
> 
> Forcing python2 is not sufficient for two reasons:
> 
> - QEMU is moving away from python2, with python2 nearing EOL,
> 
> - according to my most recent testing, the lacking dependency information
>   in the makefiles that are generated by edk2's "build" utility can cause
>   parallel build failures even when "build" is executed by python2.
> 
> And forcing python2 is not necessary because we can still return to the
> original idea of filtering out jobserver-related options from MAKEFLAGS.
> So do that.

FYI I tried uninstalling python2 on Fedora 29,

$ make -C roms efi -j8
make: Entering directory '/home/phil/source/qemu/roms'
make -C edk2/BaseTools \
        EXTRA_OPTFLAGS='' \
        EXTRA_LDFLAGS=''
make[1]: Entering directory '/home/phil/source/qemu/roms/edk2/BaseTools'
[...]
make -C Tests
make[2]: Entering directory
'/home/phil/source/qemu/roms/edk2/BaseTools/Tests'
/bin/sh: python: command not found
make[2]: *** [GNUmakefile:11: test] Error 127

'python' seems to be provided by python-unversioned-command which is
wired to Python2:

$ dnf info python-unversioned-command
Last metadata expiration check: 0:03:08 ago on Thu 19 Sep 2019 04:21:21
PM UTC.
Available Packages
Name         : python-unversioned-command
Version      : 2.7.16
Release      : 2.fc29
Arch         : noarch
Size         : 13 k
Source       : python2-2.7.16-2.fc29.src.rpm
Repo         : updates
Summary      : The "python" command that runs Python 2
URL          : https://www.python.org/
License      : Python
Description  : This package contains /usr/bin/python - the "python"
command that runs Python 2.

I had to manually run update-alternatives to continue:

$ sudo update-alternatives --install /usr/bin/python python
/usr/bin/python3 69

Not sure this is the expected behavior, it is confusing.
Laszlo Ersek Sept. 19, 2019, 6:54 p.m. UTC | #5
On 09/19/19 15:41, Philippe Mathieu-Daudé wrote:
> Hi Laszlo,
> 
> On 9/18/19 7:11 PM, Laszlo Ersek wrote:
>> It turns out that forcing python2 for running the edk2 "build" utility is
>> neither necessary nor sufficient.
>>
>> Forcing python2 is not sufficient for two reasons:
>>
>> - QEMU is moving away from python2, with python2 nearing EOL,
>>
>> - according to my most recent testing, the lacking dependency information
>>   in the makefiles that are generated by edk2's "build" utility can cause
>>   parallel build failures even when "build" is executed by python2.
>>
>> And forcing python2 is not necessary because we can still return to the
>> original idea of filtering out jobserver-related options from MAKEFLAGS.
>> So do that.
>>
>> With this patch, the guest UEFI binaries that are used as part of the BIOS
>> tables test, and the OVMF and ArmVirtQemu platform firmwares, will be
>> built strictly in a single job, regardless of an outermost "-jN" make
>> option. Alas, there appears to be no reliable way to build edk2 in an
>> (outer make, inner make) environment, with a jobserver enabled.
>>
>> Cc: Eduardo Habkost <ehabkost@redhat.com>
>> Cc: John Snow <jsnow@redhat.com>
>> Cc: Philippe Mathieu-Daudé <philmd@redhat.com>
>> Reported-by: John Snow <jsnow@redhat.com>
>> Signed-off-by: Laszlo Ersek <lersek@redhat.com>
>> ---
>>
>> Notes:
>>     - Tested on RHEL7 (where the outer "make" sets the old-style
>>       "--jobserver-fds" flag) and on Fedora 29 (where the outer "make" sets
>>       the new-style "--jobserver-auth" flag).
>>     
>>     - I've rebuilt all the edk2 binaries with this patch applied. Everything
>>       works fine. However, if you test this patch, you might notice that git
>>       reports all the build products as modified. That's because when using
>>       the python3 code in edk2 BaseTools, the generated makefiles differ
>>       greatly from the ones generated when running in python2 mode (e.g. due
>>       to different random seeds in python hashes / dictionaries). As a
>>       result, parts of the firmware volumes / firmware filesystems could
>>       appear in a different order than before. This is harmless, and doesn't
>>       necessitate checking in the rebuilt binaries.
>>
>>  roms/edk2-build.sh             |  4 +---
>>  roms/edk2-funcs.sh             | 17 +++++++++++++++++
>>  tests/uefi-test-tools/build.sh |  6 +++---
>>  3 files changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
>> index 4f46f8a6a217..8161c55ef507 100755
>> --- a/roms/edk2-build.sh
>> +++ b/roms/edk2-build.sh
>> @@ -27,9 +27,6 @@ shift $num_args
>>  
>>  cd edk2
>>  
>> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
>> -export PYTHON_COMMAND=python2
>> -
>>  # Source "edksetup.sh" carefully.
>>  set +e +u +C
>>  source ./edksetup.sh
>> @@ -43,6 +40,7 @@ fi
>>  # any), for the edk2 "build" utility.
>>  source ../edk2-funcs.sh
>>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
>> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
>>  edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>>  qemu_edk2_set_cross_env "$emulation_target"
>>  
>> diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
>> index a9fae7ee891b..3f4485b201f1 100644
>> --- a/roms/edk2-funcs.sh
>> +++ b/roms/edk2-funcs.sh
>> @@ -251,3 +251,20 @@ qemu_edk2_get_thread_count()
>>      printf '1\n'
>>    fi
>>  }
>> +
>> +
>> +# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607> by
>> +# filtering jobserver-related flags out of MAKEFLAGS. Print the result to the
>> +# standard output.
>> +#
>> +# Parameters:
>> +#   $1: the value of the MAKEFLAGS variable
>> +qemu_edk2_quirk_tianocore_1607()
>> +{
>> +  local makeflags="$1"
>> +
>> +  printf %s "$makeflags" \
>> +  | LC_ALL=C sed --regexp-extended \
>> +      --expression='s/--jobserver-(auth|fds)=[0-9]+,[0-9]+//' \
>> +      --expression='s/-j([0-9]+)?//'
>> +}
>> diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
>> index 8aa7935c43bb..eba7964a163b 100755
>> --- a/tests/uefi-test-tools/build.sh
>> +++ b/tests/uefi-test-tools/build.sh
>> @@ -29,9 +29,6 @@ export PACKAGES_PATH=$(realpath -- "$edk2_dir")
>>  export WORKSPACE=$PWD
>>  mkdir -p Conf
>>  
>> -# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
>> -export PYTHON_COMMAND=python2
>> -
>>  # Source "edksetup.sh" carefully.
>>  set +e +u +C
>>  source "$PACKAGES_PATH/edksetup.sh"
>> @@ -46,12 +43,15 @@ fi
>>  source "$edk2_dir/../edk2-funcs.sh"
>>  edk2_arch=$(qemu_edk2_get_arch "$emulation_target")
>>  edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
>> +MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
>> +edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
>>  qemu_edk2_set_cross_env "$emulation_target"
>>  
>>  # Build the UEFI binary
>>  mkdir -p log
>>  build \
>>    --arch="$edk2_arch" \
>> +  -n "$edk2_thread_count" \
>>    --buildtarget=DEBUG \
>>    --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
>>    --tagname="$edk2_toolchain" \
>>
> 
> Very clear explanation, thanks.
> Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Thanks!

> 
> Hmm this failed on Ubuntu Xenial which is the image we use for Travis-CI:
> 
> $ lsb_release -a
> Distributor ID:	Ubuntu
> Description:	Ubuntu 16.04.6 LTS
> Release:	16.04
> Codename:	xenial
> 
> make -f Makefile.edk2
> make[1]: Entering directory '/home/phil/qemu/roms'
> if test -d edk2/.git; then \
> 	cd edk2 && git submodule update --init --force; \
> fi
> ./edk2-build.sh \
> 	aarch64 \
> 	--arch=AARCH64 \
> 	--platform=ArmVirtPkg/ArmVirtQemu.dsc \
> 	-D NETWORK_IP6_ENABLE \
> 	-D NETWORK_HTTP_BOOT_ENABLE
> WORKSPACE: /home/phil/qemu/roms/edk2
> EDK_TOOLS_PATH: /home/phil/qemu/roms/edk2/BaseTools
> CONF_PATH: /home/phil/qemu/roms/edk2/Conf
> Copying $EDK_TOOLS_PATH/Conf/build_rule.template
>      to /home/phil/qemu/roms/edk2/Conf/build_rule.txt
> Copying $EDK_TOOLS_PATH/Conf/tools_def.template
>      to /home/phil/qemu/roms/edk2/Conf/tools_def.txt
> Copying $EDK_TOOLS_PATH/Conf/target.template
>      to /home/phil/qemu/roms/edk2/Conf/target.txt
> pyenv: python3.7: command not found
> The `python3.7' command exists in these Python versions:
>   3.7
>   3.7.1

... I don't have the slightest idea what this error message means. Edk2
contains no reference to "pyenv".

> 
> Makefile.edk2:62: recipe for target '../pc-bios/edk2-aarch64-code.fd' failed
> make[1]: *** [../pc-bios/edk2-aarch64-code.fd] Error 127
> make[1]: Leaving directory '/home/phil/qemu/roms'
> Makefile:168: recipe for target 'efi' failed
> make: *** [efi] Error 2
> make: Leaving directory '/home/phil/qemu/roms'
> The command "make -C roms efi -j2" exited with 2.
> 
> The local Python3 version is:
> 
> $ apt-cache show python3-minimal
> Package: python3-minimal
> Version: 3.5.1-3
> 
> Any idea which script is choosing python3.7?
> 

It's the SetupPython() function in "edksetup.sh".

If there is a universal pathname (or just filename) that refers to
python3 on all build hosts where "make efi" is expected to run, I can
assign that to PYTHON_COMMAND. Otherwise, I'm out of ideas.

Thanks
Laszlo
Laszlo Ersek Sept. 19, 2019, 7:08 p.m. UTC | #6
On 09/19/19 18:39, Philippe Mathieu-Daudé wrote:
> On 9/18/19 7:11 PM, Laszlo Ersek wrote:
>> It turns out that forcing python2 for running the edk2 "build" utility is
>> neither necessary nor sufficient.
>>
>> Forcing python2 is not sufficient for two reasons:
>>
>> - QEMU is moving away from python2, with python2 nearing EOL,
>>
>> - according to my most recent testing, the lacking dependency information
>>   in the makefiles that are generated by edk2's "build" utility can cause
>>   parallel build failures even when "build" is executed by python2.
>>
>> And forcing python2 is not necessary because we can still return to the
>> original idea of filtering out jobserver-related options from MAKEFLAGS.
>> So do that.
> 
> FYI I tried uninstalling python2 on Fedora 29,
> 
> $ make -C roms efi -j8
> make: Entering directory '/home/phil/source/qemu/roms'
> make -C edk2/BaseTools \
>         EXTRA_OPTFLAGS='' \
>         EXTRA_LDFLAGS=''
> make[1]: Entering directory '/home/phil/source/qemu/roms/edk2/BaseTools'
> [...]
> make -C Tests
> make[2]: Entering directory
> '/home/phil/source/qemu/roms/edk2/BaseTools/Tests'
> /bin/sh: python: command not found
> make[2]: *** [GNUmakefile:11: test] Error 127
> 
> 'python' seems to be provided by python-unversioned-command which is
> wired to Python2:
> 
> $ dnf info python-unversioned-command
> Last metadata expiration check: 0:03:08 ago on Thu 19 Sep 2019 04:21:21
> PM UTC.
> Available Packages
> Name         : python-unversioned-command
> Version      : 2.7.16
> Release      : 2.fc29
> Arch         : noarch
> Size         : 13 k
> Source       : python2-2.7.16-2.fc29.src.rpm
> Repo         : updates
> Summary      : The "python" command that runs Python 2
> URL          : https://www.python.org/
> License      : Python
> Description  : This package contains /usr/bin/python - the "python"
> command that runs Python 2.
> 
> I had to manually run update-alternatives to continue:
> 
> $ sudo update-alternatives --install /usr/bin/python python
> /usr/bin/python3 69
> 
> Not sure this is the expected behavior, it is confusing.
> 

The python detection is not fool-proof in edk2. A description is given at:

https://github.com/tianocore/tianocore.github.io/wiki/BaseTools-Support-Python2-Python3

To summarize that, it works like this, on Linux:

- if you set PYTHON_COMMAND, then the binary pointed to by
PYTHON_COMMAND will be used. The edk2 build infrastructure will
determine whether the pointed-to binary is python 2 or python 3, and
branch to the corresponding implementation of the build tools.

- Otherwise, *minor* version auto-detection is attempted. With
PYTHON3_ENABLE unset, or set to "TRUE", this minor version autodetection
will aim at minor versions of python3.

- Otherwise (= PYTHON3_ENABLE set to a string different from "TRUE"),
the minor version auto-detection will focus on python2.

With this patch applied, the middle case is active. Apparently it fails,
because the edk2 build tools developers could not foresee the situations
that you've exposed the auto-detection to, on Ubuntu and Fedora. Back
when I tested the python3 enablement in edk2, I checked the patches in
the following environments:

- on RHEL-7 with the system python 2,
- on RHEL-7 with python3.4 from EPEL-7,
- on RHEL-8 with python3.6,
- on RHEL-8 with platform-python.

Everything worked fine for me. I have no clue what's going on in Ubuntu
and in Fedora.

Can we require all build host installations -- where we expect to run
"make efi" -- to provide a Python 3 binary on $PATH that is plainly
called "python3"?

Then I think this patch should work. (If necessary, I could set
"PYTHON_COMMAND=python3", too.)

Thanks!
Laszlo
Philippe Mathieu-Daudé Sept. 19, 2019, 7:56 p.m. UTC | #7
On 9/19/19 9:08 PM, Laszlo Ersek wrote:
> On 09/19/19 18:39, Philippe Mathieu-Daudé wrote:
>> On 9/18/19 7:11 PM, Laszlo Ersek wrote:
>>> It turns out that forcing python2 for running the edk2 "build" utility is
>>> neither necessary nor sufficient.
>>>
>>> Forcing python2 is not sufficient for two reasons:
>>>
>>> - QEMU is moving away from python2, with python2 nearing EOL,
>>>
>>> - according to my most recent testing, the lacking dependency information
>>>   in the makefiles that are generated by edk2's "build" utility can cause
>>>   parallel build failures even when "build" is executed by python2.
>>>
>>> And forcing python2 is not necessary because we can still return to the
>>> original idea of filtering out jobserver-related options from MAKEFLAGS.
>>> So do that.
>>
>> FYI I tried uninstalling python2 on Fedora 29,
>>
>> $ make -C roms efi -j8
>> make: Entering directory '/home/phil/source/qemu/roms'
>> make -C edk2/BaseTools \
>>         EXTRA_OPTFLAGS='' \
>>         EXTRA_LDFLAGS=''

^ this is the 'edk2-basetools' target from roms/Makefile.

>> make[1]: Entering directory '/home/phil/source/qemu/roms/edk2/BaseTools'
>> [...]
>> make -C Tests
>> make[2]: Entering directory
>> '/home/phil/source/qemu/roms/edk2/BaseTools/Tests'
>> /bin/sh: python: command not found
>> make[2]: *** [GNUmakefile:11: test] Error 127
>>
>> 'python' seems to be provided by python-unversioned-command which is
>> wired to Python2:
>>
>> $ dnf info python-unversioned-command
>> Last metadata expiration check: 0:03:08 ago on Thu 19 Sep 2019 04:21:21
>> PM UTC.
>> Available Packages
>> Name         : python-unversioned-command
>> Version      : 2.7.16
>> Release      : 2.fc29
>> Arch         : noarch
>> Size         : 13 k
>> Source       : python2-2.7.16-2.fc29.src.rpm
>> Repo         : updates
>> Summary      : The "python" command that runs Python 2
>> URL          : https://www.python.org/
>> License      : Python
>> Description  : This package contains /usr/bin/python - the "python"
>> command that runs Python 2.
>>
>> I had to manually run update-alternatives to continue:
>>
>> $ sudo update-alternatives --install /usr/bin/python python
>> /usr/bin/python3 69
>>
>> Not sure this is the expected behavior, it is confusing.
>>
> 
> The python detection is not fool-proof in edk2. A description is given at:
> 
> https://github.com/tianocore/tianocore.github.io/wiki/BaseTools-Support-Python2-Python3
> 
> To summarize that, it works like this, on Linux:
> 
> - if you set PYTHON_COMMAND, then the binary pointed to by
> PYTHON_COMMAND will be used. The edk2 build infrastructure will
> determine whether the pointed-to binary is python 2 or python 3, and
> branch to the corresponding implementation of the build tools.
> 
> - Otherwise, *minor* version auto-detection is attempted. With
> PYTHON3_ENABLE unset, or set to "TRUE", this minor version autodetection
> will aim at minor versions of python3.
> 
> - Otherwise (= PYTHON3_ENABLE set to a string different from "TRUE"),
> the minor version auto-detection will focus on python2.

What you document regarding PYTHON3_ENABLE is valid once we sourced
edksetup.sh which is done in Makefile.edk2, one step after the previous
call:

efi: edk2-basetools               # call 1 (python failing)
	$(MAKE) -f Makefile.edk2  # call 2 sourcing edksetup.sh

> With this patch applied, the middle case is active. Apparently it fails,
> because the edk2 build tools developers could not foresee the situations
> that you've exposed the auto-detection to, on Ubuntu and Fedora. Back
> when I tested the python3 enablement in edk2, I checked the patches in
> the following environments:
> 
> - on RHEL-7 with the system python 2,
> - on RHEL-7 with python3.4 from EPEL-7,
> - on RHEL-8 with python3.6,
> - on RHEL-8 with platform-python.
> 
> Everything worked fine for me. I have no clue what's going on in Ubuntu
> and in Fedora.
> 
> Can we require all build host installations -- where we expect to run
> "make efi" -- to provide a Python 3 binary on $PATH that is plainly
> called "python3"?

Kevin recently suggested a similar patch (in another area):
https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04377.html

> Then I think this patch should work. (If necessary, I could set
> "PYTHON_COMMAND=python3", too.)

Yes, I confirm forcing "PYTHON_COMMAND=python3 make -C roms efi" works.

Not sure what is the cleaner way to fix this although...

Regards,

Phil.
Laszlo Ersek Sept. 19, 2019, 9:40 p.m. UTC | #8
On 09/19/19 21:56, Philippe Mathieu-Daudé wrote:
> On 9/19/19 9:08 PM, Laszlo Ersek wrote:
>> On 09/19/19 18:39, Philippe Mathieu-Daudé wrote:
>>> On 9/18/19 7:11 PM, Laszlo Ersek wrote:
>>>> It turns out that forcing python2 for running the edk2 "build" utility is
>>>> neither necessary nor sufficient.
>>>>
>>>> Forcing python2 is not sufficient for two reasons:
>>>>
>>>> - QEMU is moving away from python2, with python2 nearing EOL,
>>>>
>>>> - according to my most recent testing, the lacking dependency information
>>>>   in the makefiles that are generated by edk2's "build" utility can cause
>>>>   parallel build failures even when "build" is executed by python2.
>>>>
>>>> And forcing python2 is not necessary because we can still return to the
>>>> original idea of filtering out jobserver-related options from MAKEFLAGS.
>>>> So do that.
>>>
>>> FYI I tried uninstalling python2 on Fedora 29,
>>>
>>> $ make -C roms efi -j8
>>> make: Entering directory '/home/phil/source/qemu/roms'
>>> make -C edk2/BaseTools \
>>>         EXTRA_OPTFLAGS='' \
>>>         EXTRA_LDFLAGS=''
> 
> ^ this is the 'edk2-basetools' target from roms/Makefile.
> 
>>> make[1]: Entering directory '/home/phil/source/qemu/roms/edk2/BaseTools'
>>> [...]
>>> make -C Tests
>>> make[2]: Entering directory
>>> '/home/phil/source/qemu/roms/edk2/BaseTools/Tests'
>>> /bin/sh: python: command not found
>>> make[2]: *** [GNUmakefile:11: test] Error 127
>>>
>>> 'python' seems to be provided by python-unversioned-command which is
>>> wired to Python2:
>>>
>>> $ dnf info python-unversioned-command
>>> Last metadata expiration check: 0:03:08 ago on Thu 19 Sep 2019 04:21:21
>>> PM UTC.
>>> Available Packages
>>> Name         : python-unversioned-command
>>> Version      : 2.7.16
>>> Release      : 2.fc29
>>> Arch         : noarch
>>> Size         : 13 k
>>> Source       : python2-2.7.16-2.fc29.src.rpm
>>> Repo         : updates
>>> Summary      : The "python" command that runs Python 2
>>> URL          : https://www.python.org/
>>> License      : Python
>>> Description  : This package contains /usr/bin/python - the "python"
>>> command that runs Python 2.
>>>
>>> I had to manually run update-alternatives to continue:
>>>
>>> $ sudo update-alternatives --install /usr/bin/python python
>>> /usr/bin/python3 69
>>>
>>> Not sure this is the expected behavior, it is confusing.
>>>
>>
>> The python detection is not fool-proof in edk2. A description is given at:
>>
>> https://github.com/tianocore/tianocore.github.io/wiki/BaseTools-Support-Python2-Python3
>>
>> To summarize that, it works like this, on Linux:
>>
>> - if you set PYTHON_COMMAND, then the binary pointed to by
>> PYTHON_COMMAND will be used. The edk2 build infrastructure will
>> determine whether the pointed-to binary is python 2 or python 3, and
>> branch to the corresponding implementation of the build tools.
>>
>> - Otherwise, *minor* version auto-detection is attempted. With
>> PYTHON3_ENABLE unset, or set to "TRUE", this minor version autodetection
>> will aim at minor versions of python3.
>>
>> - Otherwise (= PYTHON3_ENABLE set to a string different from "TRUE"),
>> the minor version auto-detection will focus on python2.
> 
> What you document regarding PYTHON3_ENABLE is valid once we sourced
> edksetup.sh which is done in Makefile.edk2, one step after the previous
> call:
> 
> efi: edk2-basetools               # call 1 (python failing)
> 	$(MAKE) -f Makefile.edk2  # call 2 sourcing edksetup.sh
> 
>> With this patch applied, the middle case is active. Apparently it fails,
>> because the edk2 build tools developers could not foresee the situations
>> that you've exposed the auto-detection to, on Ubuntu and Fedora. Back
>> when I tested the python3 enablement in edk2, I checked the patches in
>> the following environments:
>>
>> - on RHEL-7 with the system python 2,
>> - on RHEL-7 with python3.4 from EPEL-7,
>> - on RHEL-8 with python3.6,
>> - on RHEL-8 with platform-python.
>>
>> Everything worked fine for me. I have no clue what's going on in Ubuntu
>> and in Fedora.
>>
>> Can we require all build host installations -- where we expect to run
>> "make efi" -- to provide a Python 3 binary on $PATH that is plainly
>> called "python3"?
> 
> Kevin recently suggested a similar patch (in another area):
> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04377.html
> 
>> Then I think this patch should work. (If necessary, I could set
>> "PYTHON_COMMAND=python3", too.)
> 
> Yes, I confirm forcing "PYTHON_COMMAND=python3 make -C roms efi" works.
> 
> Not sure what is the cleaner way to fix this although...

Thanks for the analysis!

I understand the issue now.

- "edk2/BaseTools/GNUmakefile" runs $(MAKE) in three subdirs:
  - Source/C,
  - Source/Python,
  - Tests (which depends on the former two)

- "edk2/BaseTools/Source/C/GNUmakefile" builds fine
- "edk2/BaseTools/Source/Python/GNUmakefile" does nothing
- "edk2/BaseTools/Tests/GNUmakefile" depends on PYTHON_COMMAND -- which
  should either come from the user, or from sourcing "edksetup.sh"

Therefore the issue is:

- the "edk2-basetools" target in "roms/Makefile" does not
  run (more precisely, does not "source") edksetup.sh

- the "build-edk2-tools" target in "tests/uefi-test-tools/Makefile"
  does not run (more precisely, does not "source") edksetup.sh

I don't think I will reorganize the dependencies in those makefiles. Nor
will I source edksetup.sh in the makefile recipes. Instead, I'll
directly set PYTHON_COMMAND=python3 in the "tools" recipes, and in the
build wrapper shell scripts.

I'll try to post v2 soon.

Thanks!
Laszlo
Philippe Mathieu-Daudé Sept. 20, 2019, 7:58 a.m. UTC | #9
On 9/19/19 11:40 PM, Laszlo Ersek wrote:
> On 09/19/19 21:56, Philippe Mathieu-Daudé wrote:
>> On 9/19/19 9:08 PM, Laszlo Ersek wrote:
>>> On 09/19/19 18:39, Philippe Mathieu-Daudé wrote:
>>>> On 9/18/19 7:11 PM, Laszlo Ersek wrote:
>>>>> It turns out that forcing python2 for running the edk2 "build" utility is
>>>>> neither necessary nor sufficient.
>>>>>
>>>>> Forcing python2 is not sufficient for two reasons:
>>>>>
>>>>> - QEMU is moving away from python2, with python2 nearing EOL,
>>>>>
>>>>> - according to my most recent testing, the lacking dependency information
>>>>>   in the makefiles that are generated by edk2's "build" utility can cause
>>>>>   parallel build failures even when "build" is executed by python2.
>>>>>
>>>>> And forcing python2 is not necessary because we can still return to the
>>>>> original idea of filtering out jobserver-related options from MAKEFLAGS.
>>>>> So do that.
>>>>
>>>> FYI I tried uninstalling python2 on Fedora 29,
>>>>
>>>> $ make -C roms efi -j8
>>>> make: Entering directory '/home/phil/source/qemu/roms'
>>>> make -C edk2/BaseTools \
>>>>         EXTRA_OPTFLAGS='' \
>>>>         EXTRA_LDFLAGS=''
>>
>> ^ this is the 'edk2-basetools' target from roms/Makefile.
>>
>>>> make[1]: Entering directory '/home/phil/source/qemu/roms/edk2/BaseTools'
>>>> [...]
>>>> make -C Tests
>>>> make[2]: Entering directory
>>>> '/home/phil/source/qemu/roms/edk2/BaseTools/Tests'
>>>> /bin/sh: python: command not found
>>>> make[2]: *** [GNUmakefile:11: test] Error 127
>>>>
>>>> 'python' seems to be provided by python-unversioned-command which is
>>>> wired to Python2:
>>>>
>>>> $ dnf info python-unversioned-command
>>>> Last metadata expiration check: 0:03:08 ago on Thu 19 Sep 2019 04:21:21
>>>> PM UTC.
>>>> Available Packages
>>>> Name         : python-unversioned-command
>>>> Version      : 2.7.16
>>>> Release      : 2.fc29
>>>> Arch         : noarch
>>>> Size         : 13 k
>>>> Source       : python2-2.7.16-2.fc29.src.rpm
>>>> Repo         : updates
>>>> Summary      : The "python" command that runs Python 2
>>>> URL          : https://www.python.org/
>>>> License      : Python
>>>> Description  : This package contains /usr/bin/python - the "python"
>>>> command that runs Python 2.
>>>>
>>>> I had to manually run update-alternatives to continue:
>>>>
>>>> $ sudo update-alternatives --install /usr/bin/python python
>>>> /usr/bin/python3 69
>>>>
>>>> Not sure this is the expected behavior, it is confusing.
>>>>
>>>
>>> The python detection is not fool-proof in edk2. A description is given at:
>>>
>>> https://github.com/tianocore/tianocore.github.io/wiki/BaseTools-Support-Python2-Python3
>>>
>>> To summarize that, it works like this, on Linux:
>>>
>>> - if you set PYTHON_COMMAND, then the binary pointed to by
>>> PYTHON_COMMAND will be used. The edk2 build infrastructure will
>>> determine whether the pointed-to binary is python 2 or python 3, and
>>> branch to the corresponding implementation of the build tools.
>>>
>>> - Otherwise, *minor* version auto-detection is attempted. With
>>> PYTHON3_ENABLE unset, or set to "TRUE", this minor version autodetection
>>> will aim at minor versions of python3.
>>>
>>> - Otherwise (= PYTHON3_ENABLE set to a string different from "TRUE"),
>>> the minor version auto-detection will focus on python2.
>>
>> What you document regarding PYTHON3_ENABLE is valid once we sourced
>> edksetup.sh which is done in Makefile.edk2, one step after the previous
>> call:
>>
>> efi: edk2-basetools               # call 1 (python failing)
>> 	$(MAKE) -f Makefile.edk2  # call 2 sourcing edksetup.sh
>>
>>> With this patch applied, the middle case is active. Apparently it fails,
>>> because the edk2 build tools developers could not foresee the situations
>>> that you've exposed the auto-detection to, on Ubuntu and Fedora. Back
>>> when I tested the python3 enablement in edk2, I checked the patches in
>>> the following environments:
>>>
>>> - on RHEL-7 with the system python 2,
>>> - on RHEL-7 with python3.4 from EPEL-7,
>>> - on RHEL-8 with python3.6,
>>> - on RHEL-8 with platform-python.
>>>
>>> Everything worked fine for me. I have no clue what's going on in Ubuntu
>>> and in Fedora.
>>>
>>> Can we require all build host installations -- where we expect to run
>>> "make efi" -- to provide a Python 3 binary on $PATH that is plainly
>>> called "python3"?
>>
>> Kevin recently suggested a similar patch (in another area):
>> https://lists.gnu.org/archive/html/qemu-devel/2019-09/msg04377.html
>>
>>> Then I think this patch should work. (If necessary, I could set
>>> "PYTHON_COMMAND=python3", too.)
>>
>> Yes, I confirm forcing "PYTHON_COMMAND=python3 make -C roms efi" works.
>>
>> Not sure what is the cleaner way to fix this although...
> 
> Thanks for the analysis!
> 
> I understand the issue now.
> 
> - "edk2/BaseTools/GNUmakefile" runs $(MAKE) in three subdirs:
>   - Source/C,
>   - Source/Python,
>   - Tests (which depends on the former two)
> 
> - "edk2/BaseTools/Source/C/GNUmakefile" builds fine
> - "edk2/BaseTools/Source/Python/GNUmakefile" does nothing
> - "edk2/BaseTools/Tests/GNUmakefile" depends on PYTHON_COMMAND -- which
>   should either come from the user, or from sourcing "edksetup.sh"
> 
> Therefore the issue is:
> 
> - the "edk2-basetools" target in "roms/Makefile" does not
>   run (more precisely, does not "source") edksetup.sh
> 
> - the "build-edk2-tools" target in "tests/uefi-test-tools/Makefile"
>   does not run (more precisely, does not "source") edksetup.sh
> 
> I don't think I will reorganize the dependencies in those makefiles. Nor

Exactly, nor do I.

> will I source edksetup.sh in the makefile recipes. Instead, I'll
> directly set PYTHON_COMMAND=python3 in the "tools" recipes, and in the
> build wrapper shell scripts.

I notice this:

 $ git grep -i python configure
 ...
 configure:1832:python="$python -B"
 ...
 configure:7287:echo "PYTHON=$python" >> $config_host_mak
 ...
 configure:7863:echo "export PYTHON='$python'" >> "$iotests_common_env"

While config-host.mak is too QEMU specific, the iotest common.env is
very simple:

 $ cat tests/qemu-iotests/common.env
 # Automatically generated by configure - do not modify
 export PYTHON='python3 -B'

I'm not sure we need to run ./configure to run any make target in the
roms/ directory (since we use 'make -C roms ...'), this script doesn't
even clone the required submodules, we need to call 'make
git-submodule-update' first.

Note that when there is no 'python3' in $PATH, the ./configure script
checks for python2, but does not use generic 'python':

configure:901:for binary in "${PYTHON-python3}" python python2
configure:905:        python="$binary"

> I'll try to post v2 soon.

While forcing PYTHON_COMMAND in roms/Makefile would help, I think part
of the proper way to fix this is generic to QEMU.

Not sure what is the cleaner way yet :/

Regards,

Phil.
diff mbox series

Patch

diff --git a/roms/edk2-build.sh b/roms/edk2-build.sh
index 4f46f8a6a217..8161c55ef507 100755
--- a/roms/edk2-build.sh
+++ b/roms/edk2-build.sh
@@ -27,9 +27,6 @@  shift $num_args
 
 cd edk2
 
-# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
-export PYTHON_COMMAND=python2
-
 # Source "edksetup.sh" carefully.
 set +e +u +C
 source ./edksetup.sh
@@ -43,6 +40,7 @@  fi
 # any), for the edk2 "build" utility.
 source ../edk2-funcs.sh
 edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
+MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
 edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
 qemu_edk2_set_cross_env "$emulation_target"
 
diff --git a/roms/edk2-funcs.sh b/roms/edk2-funcs.sh
index a9fae7ee891b..3f4485b201f1 100644
--- a/roms/edk2-funcs.sh
+++ b/roms/edk2-funcs.sh
@@ -251,3 +251,20 @@  qemu_edk2_get_thread_count()
     printf '1\n'
   fi
 }
+
+
+# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607> by
+# filtering jobserver-related flags out of MAKEFLAGS. Print the result to the
+# standard output.
+#
+# Parameters:
+#   $1: the value of the MAKEFLAGS variable
+qemu_edk2_quirk_tianocore_1607()
+{
+  local makeflags="$1"
+
+  printf %s "$makeflags" \
+  | LC_ALL=C sed --regexp-extended \
+      --expression='s/--jobserver-(auth|fds)=[0-9]+,[0-9]+//' \
+      --expression='s/-j([0-9]+)?//'
+}
diff --git a/tests/uefi-test-tools/build.sh b/tests/uefi-test-tools/build.sh
index 8aa7935c43bb..eba7964a163b 100755
--- a/tests/uefi-test-tools/build.sh
+++ b/tests/uefi-test-tools/build.sh
@@ -29,9 +29,6 @@  export PACKAGES_PATH=$(realpath -- "$edk2_dir")
 export WORKSPACE=$PWD
 mkdir -p Conf
 
-# Work around <https://bugzilla.tianocore.org/show_bug.cgi?id=1607>.
-export PYTHON_COMMAND=python2
-
 # Source "edksetup.sh" carefully.
 set +e +u +C
 source "$PACKAGES_PATH/edksetup.sh"
@@ -46,12 +43,15 @@  fi
 source "$edk2_dir/../edk2-funcs.sh"
 edk2_arch=$(qemu_edk2_get_arch "$emulation_target")
 edk2_toolchain=$(qemu_edk2_get_toolchain "$emulation_target")
+MAKEFLAGS=$(qemu_edk2_quirk_tianocore_1607 "$MAKEFLAGS")
+edk2_thread_count=$(qemu_edk2_get_thread_count "$MAKEFLAGS")
 qemu_edk2_set_cross_env "$emulation_target"
 
 # Build the UEFI binary
 mkdir -p log
 build \
   --arch="$edk2_arch" \
+  -n "$edk2_thread_count" \
   --buildtarget=DEBUG \
   --platform=UefiTestToolsPkg/UefiTestToolsPkg.dsc \
   --tagname="$edk2_toolchain" \