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

[Claudius Heine <claudius.heine.ext@siemens.com>]

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.

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

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