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

[Andreas Reichel <andreas.reichel.ext@siemens.com>]

On Fri, Sep 08, 2017 at 09:37:38AM +0200, Henning Schild wrote:
> 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
I have just tested this without adding those variable names and again
isar cannot connect to official debian mirrors. To me it seems obvious,
that the whole environment is cleared besides 'BASEDIR' and 'BUILDDIR'.
At least I cannot reproduce any other behavior.

If you look in bitbake/lib/bb/utils.py, line 635, BB_ENV_EXTRAWHITE
contains a list of approved variables, that remain in the environment.
In line 629, approved is set to empty list if BB_PRESERVE_ENV is not
set, which is the case.
Line 641 defines clean_environment, which filters everything out,
that is not mentioned in approved_variables(), except BB_PRESERVE_ENV
is set, which is not the case.

in bitbake's main.py, line 445, clean_environment gets called, which
starts the before-mentioned mechanism. Thus, I do not believe, it has
ever worked without extending BB_ENV_EXTRAWHITE.

In official poky, we find

BB_ENV_EXTRAWHITE_OE="MACHINE DISTRO TCMODE TCLIBC HTTP_PROXY http_proxy \
HTTPS_PROXY https_proxy FTP_PROXY ftp_proxy FTPS_PROXY ftps_proxy ALL_PROXY \
all_proxy NO_PROXY no_proxy SSH_AGENT_PID SSH_AUTH_SOCK BB_SRCREV_POLICY \
SDKMACHINE BB_NUMBER_THREADS BB_NO_NETWORK PARALLEL_MAKE GIT_PROXY_COMMAND \
SOCKS5_PASSWD SOCKS5_USER SCREENDIR STAMPS_DIR BBPATH_EXTRA BB_SETSCENE_ENFORCE"

Oh what great wonder... we have the proxy variables there...

Andreas


-- 
Andreas Reichel
Dipl.-Phys. (Univ.)
Software Consultant

Andreas.Reichel@tngtech.com, +49-174-3180074
TNG Technology Consulting GmbH, Betastr. 13a, 85774 Unterfoehring
Geschaeftsfuehrer: Henrik Klagges, Dr. Robert Dahlke, Gerhard Mueller
Sitz: Unterfoehring * Amtsgericht Muenchen * HRB 135082