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

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

Hi Tolga,

On 17/12/2018 19.11, Tolga Hoşgör wrote:
> Hi,
> 
> The second patch is kind of an unrelated clean up that does a relatively
> big change to other parts of the code (get_distro_primary_source_entry to
> be exact). I believe it is better off as a separate commit that is
> revertible on its own.

But I pointed out the changes to get_distro_have_https_source specifically.

If you feel that refactoring of your just introduced 
`get_distro_primary_source_entry` function should be handled separately, 
then OK. I sort of see where you are coming from here. Since you just 
split a function before and then change it in the next patch.

Also please avoid top-posting in mailing lists.

Cheers,
Claudius

> 
> On Mon, Dec 17, 2018, 14:55 Claudius Heine <claudius.heine.ext@siemens.com
> wrote:
> 
>> 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
>>
> 

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