Re: [PATCH v2 2/2] isar-bootstrap: simplified distro source functions

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

Hi Tolga,

On 17/12/2018 12.45, Hosgor, Tolga (CT RDA DS EU TR MTS) wrote:
> - distro sources are created with a generator
> - debian-buster.list begins with a 'deb-src' to test
> the parser works correctly
> 
> Signed-off-by: Hosgor, Tolga (CT RDA DS EU TR MTS) <tolga.hosgor@siemens.com>
> ---
>   meta-isar/conf/distro/debian-buster.list      |  1 +
>   .../isar-bootstrap/isar-bootstrap.inc         | 22 ++++++++-----------
>   2 files changed, 10 insertions(+), 13 deletions(-)
> 
> diff --git a/meta-isar/conf/distro/debian-buster.list b/meta-isar/conf/distro/debian-buster.list
> index 92d20e1..7828671 100644
> --- a/meta-isar/conf/distro/debian-buster.list
> +++ b/meta-isar/conf/distro/debian-buster.list
> @@ -1,3 +1,4 @@
> +deb-src	http://ftp.de.debian.org/debian buster	main contrib non-free
>   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
> diff --git a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> index d868bb6..6dc1888 100644
> --- a/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> +++ b/meta/recipes-core/isar-bootstrap/isar-bootstrap.inc
> @@ -117,7 +117,7 @@ def get_aptsources_list(d, is_host=False):
>           apt_sources_list = (d.getVar("DISTRO_APT_SOURCES", True) or "").split()
>       return apt_sources_list
>   
> -def get_distro_primary_source_entry(d, is_host=False):
> +def generate_distro_sources(d, is_host=False):
>       apt_sources_list = get_aptsources_list(d, is_host)
>       for entry in apt_sources_list:
>           entry_real = bb.parse.resolve_file(entry, d)
> @@ -126,21 +126,17 @@ def get_distro_primary_source_entry(d, is_host=False):
>                   parsed = parse_aptsources_list_line(line)
>                   if parsed:
>                       parsed = get_apt_source_mirror(d, parsed)
> -                    if parsed[0] == "deb":
> -                        return parsed[2:]
> +                    yield parsed
> +
> +def get_distro_primary_source_entry(d, is_host=False):
> +    apt_sources_list = get_aptsources_list(d, is_host)
> +    for source in generate_distro_sources(d, is_host):
> +        if source[0] == "deb":
> +            return source[2:]
>       return ["", "", ""]
>   
>   def get_distro_have_https_source(d, is_host=False):
> -    for entry in get_aptsources_list(d, is_host):
> -        entry_real = bb.parse.resolve_file(entry, d)
> -        with open(entry_real, "r") as in_fd:
> -            for line in in_fd:
> -                parsed = parse_aptsources_list_line(line)
> -                if parsed:
> -                    parsed = get_apt_source_mirror(d, parsed)
> -                    if parsed[2].startswith("https://"):
> -                        return True
> -    return False
> +    return any(source[2].startswith("https://") for source in generate_distro_sources(d, is_host))

Maybe you should merge those changes into the first patch?

Changing the same code in one patchset makes the review more difficult, 
also I don't really see a reason for this split here.

Cheers,
Claudius

>   
>   def get_distro_needs_https_support(d, is_host=False):
>       if get_distro_have_https_source(d, is_host):
> 

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