[PATCH v2 0/2] Drop Python 3.5 support, re-add user-controlled config ordering

[Jan Kiszka]

Changes in v2:
- drop python 3.5 in order to get ordered dicts for free

Jan

Jan Kiszka (2):
Drop support for Python 3.5
Store BBLAYERS and local.conf fragments in specification order again

.github/workflows/next.yml | 2 +-
kas/config.py | 2 +-
kas/libcmds.py | 4 ++--
setup.py | 9 ++++-----
4 files changed, 8 insertions(+), 9 deletions(-)

--
2.26.2

[Jan Kiszka]

From: Jan Kiszka <jan.k...@siemens.com>

According to c9326cc1edc9, we had issues with producing a deterministic
order for those config elements. As sorting is a rather blunt method of
resolving it and as ordering plays a role in bitbake's processing of the
configs, we should try harder to provide more user control over that.

A key role in this play ordered dictionaries that report their content
the same order it was added. With Python 3.6 now the minimal supported
version, this is the default behavior. So it's safe to remove sorted().

Closes: https://github.com/siemens/kas/issues/36

Reported-by: Marius Kriegerowski <marius.kr...@gfz-potsdam.de>
Signed-off-by: Jan Kiszka <jan.k...@siemens.com>
---

kas/config.py | 2 +-
kas/libcmds.py | 4 ++--

2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/kas/config.py b/kas/config.py
index 0119f68..0473431 100644
--- a/kas/config.py
+++ b/kas/config.py
@@ -128,7 +128,7 @@ class Config:
Returns the local.conf header
"""
header = ''
- for key, value in sorted(self._config.get(header_name, {}).items()):
+ for key, value in self._config.get(header_name, {}).items():
header += '# {}\n{}\n'.format(key, value)
return header

diff --git a/kas/libcmds.py b/kas/libcmds.py
index d65048d..750c973 100644
--- a/kas/libcmds.py
+++ b/kas/libcmds.py
@@ -245,8 +245,8 @@ class WriteBBConfig(Command):
fds.write(ctx.config.get_bblayers_conf_header())
fds.write('BBLAYERS ?= " \\\n ')
fds.write(' \\\n '.join(
- sorted(layer for repo in ctx.config.get_repos()
- for layer in repo.layers)))
+ layer for repo in ctx.config.get_repos()
+ for layer in repo.layers))
fds.write('"\n')

def _write_local_conf(ctx):
--
2.26.2

[Claudius Heine]

Hi Jan,

On 2020-12-15 10:18, Jan Kiszka wrote:
> From: Jan Kiszka <jan.k...@siemens.com>
>
> According to c9326cc1edc9, we had issues with producing a deterministic
> order for those config elements. As sorting is a rather blunt method of
> resolving it and as ordering plays a role in bitbake's processing of the
> configs, we should try harder to provide more user control over that.
>
> A key role in this play ordered dictionaries that report their content
> the same order it was added. With Python 3.6 now the minimal supported
> version, this is the default behavior. So it's safe to remove sorted().

As I said before, that would prohibit inserting a repo in between two
others, that were included by a other kas file, by naming the layer
appropriately if required.

I guess you have looked into the bb code for this, so can you also
explain why layer priorities in bb are not enough, maybe even in the
commit message?

Thanks,
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: c...@denx.de

PGP key: 6FF2 E59F 00C6 BC28 31D8 64C1 1173 CB19 9808 B153
Keyserver: hkp://pool.sks-keyservers.net

[Jan Kiszka]

On 15.12.20 10:40, Claudius Heine wrote:
> Hi Jan,
>
> On 2020-12-15 10:18, Jan Kiszka wrote:
>> From: Jan Kiszka <jan.k...@siemens.com>
>>
>> According to c9326cc1edc9, we had issues with producing a deterministic
>> order for those config elements. As sorting is a rather blunt method of
>> resolving it and as ordering plays a role in bitbake's processing of the
>> configs, we should try harder to provide more user control over that.
>>
>> A key role in this play ordered dictionaries that report their content
>> the same order it was added. With Python 3.6 now the minimal supported
>> version, this is the default behavior. So it's safe to remove sorted().
>
> As I said before, that would prohibit inserting a repo in between two
> others, that were included by a other kas file, by naming the layer
> appropriately if required.

Cannot follow yet, can you elaborate (or provide a demo case)?

>
> I guess you have looked into the bb code for this, so can you also
> explain why layer priorities in bb are not enough, maybe even in the
> commit message?

See the referenced ticket: Marius discussed this with OE/Yocto folks.

Jan

Siemens AG, T RDA IOT
Corporate Competence Center Embedded Linux

[Claudius Heine]

Hi Jan,

On 2020-12-15 10:52, Jan Kiszka wrote:
> On 15.12.20 10:40, Claudius Heine wrote:
>> Hi Jan,
>>
>> On 2020-12-15 10:18, Jan Kiszka wrote:
>>> From: Jan Kiszka <jan.k...@siemens.com>
>>>
>>> According to c9326cc1edc9, we had issues with producing a deterministic
>>> order for those config elements. As sorting is a rather blunt method of
>>> resolving it and as ordering plays a role in bitbake's processing of the
>>> configs, we should try harder to provide more user control over that.
>>>
>>> A key role in this play ordered dictionaries that report their content
>>> the same order it was added. With Python 3.6 now the minimal supported
>>> version, this is the default behavior. So it's safe to remove sorted().
>>
>> As I said before, that would prohibit inserting a repo in between two
>> others, that were included by a other kas file, by naming the layer
>> appropriately if required.
>
> Cannot follow yet, can you elaborate (or provide a demo case)?

file-1.yml:
```
[...]
repo:
00-repo1:
[...]
10-repo2:
[...]
[...]
```

file-2.yml:
```
header:
includes:
- file-1.yml

[...]
repo:
05-repo:
[...]
[...]
```

This would result in this order (1):

- 00-repo1
- 05-repo
- 10-repo2

While your patch would result in this order (IIUC) (2):

- 00-repo1
- 10-repo2
- 05-repo

With this file and include layout and this change you cannot reproduce
the first order (1). Without you change just renaming the one repo key
in file-2.yml (for instance from `05-repo` to `20-repo` could reproduce
the second (2) order.

In that sense your patch is a step back.

However I don't know if people rely on the key-ordered dictionaries. If
nobody relies on doing ordering this way, we can safely remove it.

>
>>
>> I guess you have looked into the bb code for this, so can you also
>> explain why layer priorities in bb are not enough, maybe even in the
>> commit message?
>
> See the referenced ticket: Marius discussed this with OE/Yocto folks.

The order only matters if the priority of the layers are equal, as
stated in this issue.

So maybe the first step would be to fix the layer priorities and allow
people to add the layers in normal OE (without kas) using
`bitbake-layers add-layer` for instance in any order they like.

Just a though: Maybe the layer priorities can be overwritten with kas?

Something like this:

```
repo:
repo-1:
[...]
layers:
layer1:
state: enabled
priority: 5
layer2:
state: disabled
priority: 6
[...]
```

(Just added `state` to showcase where the current string value of the
layer would move to.)

I have not investigated if the priority of a specific layer can be
overwritten from a `local.conf` or `bblayers.conf` file. If that is
possible, then kas doesn't need to do anything here.

regards,
Claudius

[Jan Kiszka]

Do we have that documented at all anywhere? It's not intuitive for
people starting new, I and suppose Marius wasn't aware of it (like I
forgot about that as well).

The better patch may indeed be refactorings of examples to express such
ordering options.

I would expect that setting BBFILE_PRIORITY_my-important-layer = "10" in
local conf already works. Never had to use that so far, though.

Jan

[Claudius Heine]

IMO if your build breaks or succeeds comes down to the order of the
layers in the `BBLAYERS` variable, then you have an issue anyway. So I
never really thought about this too much. So this might just be like a
undocumented implementation-specific side feature of kas to allow
ordering like that.

> The better patch may indeed be refactorings of examples to express such
> ordering options.
>

The first question is more, do we want to make this a documented
official feature, or should we find a better way for this first?

Maybe make the ordering explicit and allow it to apply to the layers
instead of the repos:


file-1.yml:
```
[...]
repo:
repo1:

[...]
layers:
layer1:
state: enabled

order: -10
layer2:
state: enabled
order: 10
repo2:
[...]
layer3:
state: enabled
order: 20

[...]
```

file-2.yml:
```
header:
includes:
- file-1.yml

[...]
repo:

repo3:
[...]
layers:
layer4:
state: enabled
order: 15
[...]
```

Resulting in this:
- layer1
- layer2
- layer4
- layer3


And maybe a default value of 0, if a layer doesn't have a `order` entry.

I am not so sure about this, this needs to be tested.

At least a repo patch could solve this, but that seems like a using a
cannon to hammer a nail ;)

regards,
Claudius

[Jan Kiszka]

I was using the lexical ordering feature a lot already to model patch
series - but I never thought of transferring that to repos. It does not
work for layer key as we define them so far because we use their
unchangeable path as key. I agree and expressed in that ticket that I
never had to rely on layer ordering and was only using layer prios so far.

We do document that patches are "applied in the order of their sorted
<patches-id>". We do not document that ordering for other keys, namely
<local-conf-id> and <bblayers-conf-id> - not to speak of <repo-id>.

On the one side, this explicit order key will allow overwriting, on the
other it's a lot of verbosity for a rather special case. Undecided.

Indeed.

Anyway, I'm pulling my two patches out of next for now. We need to think
this through more carefully.

[Marius Kriegerowski]

Dear kas team!

It seems I caused quite some discussion on an issue that probably isn't
an issue in professional setups, where layer prioritization will most
likely be setup properly. For me rather on beginner level (~1 yr. yocto)
the changed order looked like there might trouble ahead (and - man -
yocto is really not forgiving apparently minor details). So, maybe it is
actually enough to log a warning that layer prios are not set or equal
to enforce people (... well, rather beginners) to handle this with more
care.

At least I would be fine with being warned/informed that something is
not obeying good practice. Maybe I would be even more grateful if kas
would warn me rather than just accepting a badly configured set of layers.

Hope that helps a little.

Best regards

Marius

[Claudius Heine]

Hi Marius,

On 2020-12-16 19:05, Marius Kriegerowski wrote:
> Dear kas team!
>
> It seems I caused quite some discussion on an issue that probably isn't
> an issue in professional setups, where layer prioritization will most
> likely be setup properly. For me rather on beginner level (~1 yr. yocto)
> the changed order looked like there might trouble ahead (and - man -
> yocto is really not forgiving apparently minor details). So, maybe it is
> actually enough to log a warning that layer prios are not set or equal
> to enforce people (... well, rather beginners) to handle this with more
> care.
>
> At least I would be fine with being warned/informed that something is
> not obeying good practice. Maybe I would be even more grateful if kas
> would warn me rather than just accepting a badly configured set of layers.

I don't think that is easy to do in kas or bitbake. Kas does not look
into the layers, their configuration or recipes and that should probably
stay that way. Bitbake just tries to build and often assumes that the
dev knows what (s)he is doing. It only complains where there is
something clearly wrong and in some cases where its clear something
could have unintended side effects. But layer prios can set with clear
intentions.

There are some linting tools for recipes available, however they only
care about recipes and not layer configurations.

Maybe just looking at the output of `bitbake-layers show-layers` and of
course `bitbake -e [target]` might help debug this stuff.

IMO this is just one of many possible pitfalls with bitbake based
projects and its tooling, which you will regularly encounter even after
many more years working with it.

Sorry for not having a good solution here,
Claudius