Re: [PATCH v2 1/2] isar-bootstrap: debootstrap https support

["Tolga Hoşgör" <tlghosgor@gmail.com>]

Hello,

On Tue, Dec 18, 2018 at 4:20 PM Claudius Heine
<claudius.heine.ext@siemens.com> wrote:
>
> Hi again,
>
> On 17/12/2018 12.45, Hosgor, Tolga (CT RDA DS EU TR MTS) wrote:
> > Building 'isar-bootstrap-target' fails when there are HTTPS URIs in
> > the distro APT sources and the first URI is not HTTPS.
> > The first URI in the APT sources is passed to 'debootstrap' and
> > the distro APT sources file is written to the isar bootstrap rootfs.
> > Then, following 'apt-get update' fails due to apt-transport-https,
> > ca-certificates being missing.
> >
> > This patch allows a distro to specify 'DISTRO_BOOTSTRAP_BASE_PACKAGES'
> > and introduces 'https-support' concept using bitbake OVERRIDES.
> > An example usage is specifying the necessary packages for http support
> > via 'DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support' in distro
> > configuration.
> >
> > Signed-off-by: Hosgor, Tolga (CT RDA DS EU TR MTS) <tolga.hosgor@siemens.com>
> > ---
> >   meta-isar/conf/distro/debian-buster.list      |  1 +
> >   meta-isar/conf/distro/debian-jessie.list      |  1 +
> >   .../conf/multiconfig/qemuamd64-buster.conf    |  2 ++
> >   .../conf/multiconfig/qemuamd64-jessie.conf    |  2 ++
> >   .../isar-bootstrap/isar-bootstrap-host.bb     |  2 ++
> >   .../isar-bootstrap/isar-bootstrap-target.bb   |  2 ++
> >   .../isar-bootstrap/isar-bootstrap.inc         | 29 +++++++++++++++++--
> >   7 files changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/meta-isar/conf/distro/debian-buster.list b/meta-isar/conf/distro/debian-buster.list
> > index 18311d8..92d20e1 100644
> > --- a/meta-isar/conf/distro/debian-buster.list
> > +++ b/meta-isar/conf/distro/debian-buster.list
> > @@ -1,3 +1,4 @@
> >   deb http://ftp.de.debian.org/debian buster  main contrib non-free
> > +deb  https://debian.inf.tu-dresden.de/debian buster  main contrib non-free
> >   deb http://ftp.de.debian.org/debian buster-updates  main contrib non-free
> >   deb http://security.debian.org      buster/updates  main contrib non-free
> > diff --git a/meta-isar/conf/distro/debian-jessie.list b/meta-isar/conf/distro/debian-jessie.list
> > index be46a57..2471402 100644
> > --- a/meta-isar/conf/distro/debian-jessie.list
> > +++ b/meta-isar/conf/distro/debian-jessie.list
> > @@ -1,3 +1,4 @@
> > +deb  https://debian.inf.tu-dresden.de/debian jessie  main contrib non-free
> >   deb http://ftp.de.debian.org/debian jessie  main contrib non-free
> >   deb http://ftp.de.debian.org/debian jessie-updates  main contrib non-free
> >   deb http://security.debian.org      jessie/updates  main contrib non-free
> > diff --git a/meta-isar/conf/multiconfig/qemuamd64-buster.conf b/meta-isar/conf/multiconfig/qemuamd64-buster.conf
> > index 059ea00..394d76b 100644
> > --- a/meta-isar/conf/multiconfig/qemuamd64-buster.conf
> > +++ b/meta-isar/conf/multiconfig/qemuamd64-buster.conf
> > @@ -15,3 +15,5 @@ QEMU_ARCH ?= "x86_64"
> >   QEMU_MACHINE ?= "q35"
> >   QEMU_CPU ?= ""
> >   QEMU_DISK_ARGS ?= "-hda ##ROOTFS_IMAGE## -bios /usr/local/share/ovmf/OVMF.fd"
> > +
> > +DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = " apt-transport-https ca-certificates"
> > diff --git a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
> > index ca00e15..d1335ff 100644
> > --- a/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
> > +++ b/meta-isar/conf/multiconfig/qemuamd64-jessie.conf
> > @@ -14,3 +14,5 @@ QEMU_ARCH ?= "x86_64"
> >   QEMU_MACHINE ?= "pc"
> >   QEMU_CPU ?= ""
> >   QEMU_DISK_ARGS ?= "-hda ##ROOTFS_IMAGE##"
> > +
> > +DISTRO_BOOTSTRAP_BASE_PACKAGES_append_https-support = " apt-transport-https ca-certificates"
> > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap-host.bb b/meta/recipes-core/isar-bootstrap/isar-bootstrap-host.bb
> > index 55696ea..47cff63 100644
> > --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap-host.bb
> > +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap-host.bb
> > @@ -40,6 +40,8 @@ python do_apt_config_prepare() {
> >   }
> >   addtask apt_config_prepare before do_bootstrap after do_unpack
> >
> > +OVERRIDES_append = ":${@get_distro_needs_https_support(d, True)}"
> > +
> >   do_bootstrap[stamp-extra-info] = "${HOST_DISTRO}-${HOST_ARCH}"
> >   do_bootstrap[vardeps] += "HOST_DISTRO_APT_SOURCES"
> >   do_bootstrap() {
> > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap-target.bb b/meta/recipes-core/isar-bootstrap/isar-bootstrap-target.bb
> > index 5752b14..57b607f 100644
> > --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap-target.bb
> > +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap-target.bb
> > @@ -39,6 +39,8 @@ python do_apt_config_prepare() {
> >   }
> >   addtask apt_config_prepare before do_bootstrap after do_unpack
> >
> > +OVERRIDES_append = ":${@get_distro_needs_https_support(d, False)}"
> > +
> >   do_bootstrap[stamp-extra-info] = "${DISTRO}-${DISTRO_ARCH}"
> >   do_bootstrap[vardeps] += "DISTRO_APT_SOURCES"
> >   do_bootstrap() {
> > diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > index cfad136..d868bb6 100644
> > --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> > @@ -23,6 +23,7 @@ APTKEYFILES = ""
> >   APTKEYRING = "${WORKDIR}/apt-keyring.gpg"
> >   DEBOOTSTRAP_KEYRING = ""
> >   DEPLOY_ISAR_BOOTSTRAP ?= ""
> > +DISTRO_BOOTSTRAP_BASE_PACKAGES = "locales"
> >
> >   python () {
> >       from urllib.parse import urlparse
> > @@ -109,11 +110,15 @@ def aggregate_aptsources_list(d, file_list, file_out):
> >                       out_fd.write("\n".encode())
> >               out_fd.write("\n".encode())
> >
> > -def get_distro_primary_source_entry(d, is_host=False):
> > +def get_aptsources_list(d, is_host=False):
> >       if is_host:
> >           apt_sources_list = (d.getVar("HOST_DISTRO_APT_SOURCES", True) or "").split()
> >       else:
> >           apt_sources_list = (d.getVar("DISTRO_APT_SOURCES", True) or "").split()
> > +    return apt_sources_list
>
> After some thought really don't like this `is_host` parameter...
>
> For me that looks like some kind of backwards approach. First you have
> the difference between target and host, when you use in the different
> files `isar-bootstrap-target.bb` and `isar-bootstrap-host.bb` and then,
> because you use common functionality from `isar-bootstrap.inc` you need
> to put this difference into a boolean and pass that to all of those
> functions.

Yeah it is really counter-intuitive. I would expect a variable to be
filled with the distro
source decision at some point and go on with it. This also applies to
the rest of the
code (the two "debootstrap" calls with True/False parameters to functions).

>
> IMO the right solutions would have been, to do it like this:
>
> isar-bootstrap.inc:
>      OVERRIDES_append = ":${@get_distro_needs_https_support(d)}"
>      def get_aptsources_list(d):
>          return (d.getVar("BOOTSTRAP_DISTRO_APT_SOURCES", True) or
> "").split()
>
> isar-bootstrap-target.bb:
>      BOOTSTRAP_DISTRO_APT_SOURCE = "${DISTRO_APT_SOURCES}"
>
> isar-bootstrap-host.bb:
>      BOOTSTRAP_DISTRO_APT_SOURCE = "${HOST_DISTRO_APT_SOURCES}"

Exactly.

>
> Of course that is not in scope of your patch, but your patch
> demonstrates that weakness nicely by splitting up that function.
>
> Claudius
>
> --
> DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-54 Fax: (+49)-8142-66989-80 Email: ch@denx.de