Re: [PATCH 1/1] Add proxy support to isar-image-*.bb and buildchroot.bb

[Henning Schild <henning.schild@siemens.com>]

Thanks for looking into this and finally finding a solution. More
comments inline.

Am Thu, 7 Sep 2017 17:03:35 +0200
schrieb "Andreas J. Reichel" <andreas.reichel.ext@siemens.com>:

> From: Andreas Reichel <andreas.reichel.ext@siemens.com>
> 
> * BB_ENV_EXTRAWHITE provides a list for variables that are kept in the
> environment by bitbake. However, isar init script clears any
> additional settings. Thus, add proxy variables to BB_ENV_EXTRAWHITE in
> isar-buildenv-internal.
> 
> * Bitbake clears environment variables for each task within a recipe.
> However, bb.utils.export_proxies function can be used with an
> inline-python call to reexport the proxy settings.
> 
> * Sudo loses environment variables again, thus call multistrap with
> sudo with the -E option to preserve (the already cleaned) environment
> for the task's multistrap command.
> 
> Note:
> Downloads are normally done by the fetcher task, which calls a python
> function that in turn uses bb.util.export_proxies. However we have a
> non-fetcher task, which needs download capabilities as well.
> 
> Signed-off-by: Andreas Reichel <andreas.reichel.ext@siemens.com>
> ---
>  meta-isar/recipes-core/images/isar-image-base.bb | 8 +++++++-
>  meta/recipes-devtools/buildchroot/buildchroot.bb | 9 +++++++--
>  scripts/isar-buildenv-internal                   | 2 +-
>  3 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/meta-isar/recipes-core/images/isar-image-base.bb
> b/meta-isar/recipes-core/images/isar-image-base.bb index
> b679d97..a826b88 100644 ---
> a/meta-isar/recipes-core/images/isar-image-base.bb +++
> b/meta-isar/recipes-core/images/isar-image-base.bb @@ -24,6 +24,11 @@
> IMAGE_ROOTFS = "${S}" do_rootfs[stamp-extra-info] =
> "${MACHINE}-${DISTRO}" 
>  do_rootfs() {
> +    # Bitbake clears environment for all task functions, but we need
> the proxy
> +    # settings in this task so do an inline python call which
> exports them
> +    # again to the environment
> +    E="${@ bb.utils.export_proxies(d)}"
> +

I think the commit message is already verbose enough and the
function-name tells people that it is about proxies. IMHO no need for a
comment.

>      install -d -m 755 ${WORKDIR}/hooks_multistrap
>  
>      # Copy config file
> @@ -46,7 +51,8 @@ do_rootfs() {
>      cd ${TOPDIR}
>  
>      # Create root filesystem
> -    sudo multistrap -a ${DISTRO_ARCH} -d "${S}" -f
> "${WORKDIR}/multistrap.conf" || true
> +    # We must use sudo -E here to preserve the environment because
> of proxy settings
> +    sudo -E multistrap -a ${DISTRO_ARCH} -d "${S}" -f
> "${WORKDIR}/multistrap.conf" || true 

I know that the env was already cleared and that it should be safe to
use "sudo -E". What about the following?

sudo http_proxy=$http_proxy ... no_proxy=$no_proxy multistrap ...

It makes truly clear which variables should be set. There is not risk
to keep anything in addition and the comment can go away. Note, if the
variables end up empty because they where not set in the env everything
should work as if they where not set in the first place. I just tested
that with wget.

>      # Configure root filesystem
>      sudo chroot ${S} /configscript.sh ${MACHINE_SERIAL}
> ${BAUDRATE_TTY} \ diff --git
> a/meta/recipes-devtools/buildchroot/buildchroot.bb
> b/meta/recipes-devtools/buildchroot/buildchroot.bb index
> ccba683..7627015 100644 ---
> a/meta/recipes-devtools/buildchroot/buildchroot.bb +++
> b/meta/recipes-devtools/buildchroot/buildchroot.bb @@ -26,6 +26,11 @@
> WORKDIR = "${TMPDIR}/work/${PF}/${DISTRO}" do_build[stamp-extra-info]
> = "${DISTRO}-${DISTRO_ARCH}" do_build() {
> +    # Bitbake clears environment for all task functions, but we need
> the proxy
> +    # settings in this task so do an inline python call which
> exports them
> +    # again to the environment
> +    E="${@ bb.utils.export_proxies(d)}"
> +

Again, comment can probably go.

>      install -d -m 755 ${WORKDIR}/hooks_multistrap
>  
>      # Copy config files
> @@ -48,11 +53,11 @@ do_build() {
>      cd ${TOPDIR}
>  
>      # Create root filesystem
> -    sudo multistrap -a ${DISTRO_ARCH} -d "${BUILDCHROOT_DIR}" -f
> "${WORKDIR}/multistrap.conf" || true
> +    sudo -E multistrap -a ${DISTRO_ARCH} -d "${BUILDCHROOT_DIR}" -f
> "${WORKDIR}/multistrap.conf" || true 
>      # Install package builder script
>      sudo install -m 755 ${THISDIR}/files/build.sh ${BUILDCHROOT_DIR}
>  
>      # Configure root filesystem
> -    sudo chroot ${BUILDCHROOT_DIR} /configscript.sh
> +    sudo -E chroot ${BUILDCHROOT_DIR} /configscript.sh

Consider the explicit export of the vars for those two sudos. Whatever
you decide it should be consistent between the 3 sudos.

With the package hooks in place the configscript will probably shrink
or disappear. So if it does not access the internet today this step
should not gain "permission" to do so. Please consider dropping the
"-E" from the third sudo.

>  }
> diff --git a/scripts/isar-buildenv-internal
> b/scripts/isar-buildenv-internal index f14d1ff..94d7eb1 100755
> --- a/scripts/isar-buildenv-internal
> +++ b/scripts/isar-buildenv-internal
> @@ -66,5 +66,5 @@ export PATH
>  BBPATH="${BUILDDIR}"
>  export BBPATH
>  
> -BB_ENV_EXTRAWHITE="BASEDIR BUILDDIR"
> +BB_ENV_EXTRAWHITE="BASEDIR BUILDDIR http_proxy https_proxy ftp_proxy
> no_proxy" export BB_ENV_EXTRAWHITE

I do not fully understand that change. As far as i understood the
problem, the fetcher was so far always able to deal with proxies
and _all_ the magic would be in bb.utils.export_proxies. Why is
export_proxies not enough for the other tasks?

Henning