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

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

Am Fri, 8 Sep 2017 09:37:38 +0200
schrieb "[ext] 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 ...

Well this would actually be pretty dangerous. The right side of the 4
assignments needs to get quoted and we should be safe.

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

Problem without the quotes, one could put commands into the variables
and execute them with sudo:
  export foo="bla ls"
  sudo foo=$foo env | grep foo
   ls: cannot access 'env': No such file or directory

The quotes solve that problem. Quote removal always comes last in posix
shell Word Expansion.
http://pubs.opengroup.org/onlinepubs/009695399/utilities/xcu_chap02.html#tag_02_06

Henning

> 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
>