lopper: assists: Add cpu_oslist_xlnx assist for dynamic OS cpulist ge…

[harishpamarthi]

Enable third-party OS entries in cpulist.yaml by discovering OS keys from the .repo.yaml, and move cpulist generation logic from inline lop code into a dedicated assist.
cpu_oslist_xlnx.py:
Add assist to generate cpulist.yaml with per-CPU supported OS lists.
The (.repo.yaml) is required and must be passed on the command line:
lopper -f -O system-top.dts -- cpu_oslist_xlnx /path/to/.repo.yaml
Use get_os_bsp_keys() from baremetal_getsupported_comp_xlnx to build the
Cortex/MicroBlaze OS list from the manifest os: section. PMU and other
non-Cortex CPUs remain standalone-only. AI engine nodes are included with
aie_runtime. Emit plain per-CPU supported_os lists.
Error out when the .repo.yaml is not provided, the file is missing,
or the manifest is invalid.
lop-cpulist.dts:
Update comment to point consumers at cpu_oslist_xlnx for supported_os
lists (empyro continues to use lop-cpulist.dts for cpu-to-ip mapping).

[zeddii]

warning .. long reply incoming .... I'd prefer this reworked as a fix to the lop rather than a move into an assist.

To be upfront about where I'm coming from: I push back on lop-able logic migrating into assists unless there's a requirement that genuinely can't be met in a lop. "The existing lop is awkward" isn't that requirement ... it's an argument for fixing the lop. Assists are the right home for things that need real program structure; the risk is that they're also the path of least resistance, and complexity that lands there tends to stay and grow.

What's being replaced, and what's left behind

The assist reproduces lop-cpu-oslist.dts, not lop-cpulist.dts...the latter only gets a comment edit here and correctly survives for empyro. But lop-cpu-oslist.dts isn't deleted, and the comment this PR removes was the only reference to it anywhere in the repo. So after this merges there are two live generators writing cpulist.yaml with the same schema, one of them unreferenced, and nothing saying which wins.

The three problems in the existing lop are real (and all three have lop-native fixes)

1. The write/read/rewrite of cpulist.yaml between lop_0 and lop_1. This only exists because the work was split across two lops. select_N already ORs ... see the comment block at lopper/__init__.py:2084-2090. One select matching both device_type:cpu and xlnx,ip-name:ai_engine, one code block branching on which it got, one write. The round-trip, the stage-ordering dependency, and lop_1's unguarded open(...,'r') all disappear.
2. If you did want state across stages, the idiom is already in the tree and doesn't involve a file: lop-select-example.dts sets tree.process in one lop and reads it from a later sibling. Lops are meant to carry state on the tree.
3. open('cpulist.yaml', 'w') ignores -O. outdir is injected into every code-v1 environment (lopper/__init__.py:2903-2906), and lop-cpulist.dts already does this correctly. It's a one-line fix.

The manifest path is supported too ...this was the one requirement I thought might justify the move, and it doesn't. A code-v1 lop takes an options property (lopper/__init__.py:2894-2900) and each key becomes a bare name in the exec environment, by the same mechanism that gives you outdir:

options = "repo_yaml:/path/to/.repo.yaml";
code = "
       from baremetal_getsupported_comp_xlnx import get_os_bsp_keys
       cortex_os_list = get_os_bsp_keys( utils.load_yaml( repo_yaml ) )
       ";

os.environ from the code block works as well if the caller would rather not template the lop.

Why the lop form matters here, concretely

This isn't a stylistic preference. I reviewed the new file and found six issues. Five of them cannot occur in a code-v1 lop, because the lop form doesn't offer the constructs that produced them:

• a module-level mutable constant shared by reference across entries, which lands YAML anchors in the generated file (inline below)
• free choice of dumper and parameters, so key ordering silently drifts from the sibling generator that emits the same class of artifact
• a module-level constant whose name outlived its meaning — NON_CORTEX_OS holds two Cortex CPU ip-names and isn't a list of OSes
• import bootstrapping: a sys.path.append placed after the imports that would need it, and dead anyway because the loader seeds the path first (lopper/__init__.py:2616-2618)
• free error handling, used to swallow an invalid manifest (inline below)

A code-v1 block has no module-level state, no import bootstrap, and a bounded injected environment. Those constraints are the point — they keep a transformation to "read the tree, write the tree," which is what keeps lops predictable and reviewable. Every one of the five above is the assist form inviting a decision that the lop form simply doesn't present. That's the cost I'm weighing against the ~15 lines of lop diff this would otherwise be.

Testability isn't a reason to move either. Lops are tested in this repo today — tests/test_lops.py and tests/test_lops_code.py drive code-v1 blocks end to end through the fixture at tests/conftest.py:278. A reworked lop-cpu-oslist.dts can and should get a test the same way. As it stands this PR adds a 106-line assist with no tests at all, so the testability argument doesn't favour it even in principle.

One bug to carry across either way: the propval(...)[0] at line 70 can raise, and it's inherited verbatim from the lop — so it needs fixing wherever this ends up. Details inline.

Happy to be argued out of this. If there's a constraint driving the assist form that I've missed — something about how the caller invokes this, or a dependency that can't be reached from a code block ... say so and I'll reconsider. But on what's here, the lop fix is the smaller and safer change.

[zeddii]

This one needs fixing regardless of where the code lands, because it's carried over verbatim from lop-cpu-oslist.dts:

cpu_ip_name =  node.propval('xlnx,ip-name')[0]   # the lop, today
if not cpu_ip_name:
    continue

It indexes before it checks, and propval() has two distinct empty forms — [''] when the property is absent, [] when it's present with no value (a bare xlnx,ip-name;). The guard on the next line handles the first correctly; this line raises on the second before the guard runs. Confirmed against a tree with a valueless property:

absent    : propval -> ['']   [0] -> ''    -> guard catches it
valueless : propval -> []     [0] -> IndexError: list index out of range

The correct idiom is two lines above at 68-69, and again at 79-80 — check, then index:

ip_name = node.propval("xlnx,ip-name")
if not ip_name or not ip_name[0]:
    continue

Worth pulling into the lop rework as a separate fix, since it's a pre-existing bug rather than something this change introduced.

[zeddii]

Flagging this as a concrete example of the point in my summary — this is a decision the assist form let you make, and it contradicts the commit message, which says:

Error out when the .repo.yaml is not provided, the file is missing, or the manifest is invalid.

The first two do error out. The third doesn't. common_utils.load_yaml catches its own parse failure, warns, and returns a falsy value, which or {} converts into an empty schema. I ran both invalid cases against the real helper:

malformed yaml : load_yaml -> {}    -> get_os_bsp_keys({}) -> ['standalone', 'freertos']
empty file     : load_yaml -> None  -> get_os_bsp_keys({}) -> ['standalone', 'freertos']

So a broken manifest doesn't stop anything: every third-party OS silently vanishes, cpulist.yaml is written anyway, and the assist returns True. A console warning plus a wrong artifact plus a success exit is the worst of the three outcomes — whatever consumes cpulist.yaml gets a plausible-looking file with the third-party entries missing, which is the exact failure this change exists to prevent.

The manifest is mandatory for this to mean anything, so it should fail hard:

schema = utils.load_yaml(abs_path)
if not schema:
    _error(f"cpu_oslist_xlnx: could not parse {abs_path}", True)

[zeddii]

This is the clearest example of what I mean about the assist form injecting problems the lop form can't have.

dict() is a shallow copy, so every AI-engine entry shares one supported_os list object — and this dumps with a plain yaml.dump, so the sharing is visible in the output. Two AIE nodes produce anchors in the generated file:

ai_engine_0:
    ip_name: ai_engine
    supported_os: &id001
    - aie_runtime
ai_engine_1:
    ip_name: ai_engine
    supported_os: *id001

Valid YAML, any parser resolves it, but it's surprising in a generated artifact and it defeats naive text matching on the file. Note baremetal_getsupported_comp_xlnx dumps through VerboseSafeDumper, whose entire purpose is ignore_aliases -> True — the sibling generator structurally cannot emit this.

The requirement for a module-level mutable AIE_ENTRY shared across iterations is what creates it; a code block building a dict inline has nowhere to put that constant. The CPU branch above already sidesteps it with list(...) at line 75.

Only reachable on an SDT with more than one AIE node, so it may not have shown up on the parts you've tested.

[zeddii]

This sentence was the only reference to lop-cpu-oslist.dts anywhere in the repo — I grepped .py, .dts, .yaml, .sh and .md. Removing it orphans that lop: it stays in the tree, stays shippable via -i, and keeps writing cpulist.yaml with the same schema the new assist writes, with nothing left to explain the relationship between the two.

If the lop rework goes ahead this line should point at the reworked lop-cpu-oslist.dts instead. If for some reason it doesn't, then lop-cpu-oslist.dts needs to be deleted in this PR rather than left behind — leaving two live generators of the same filename with no stated precedence is worse than either option on its own.

[harishpamarthi]

Thanks for the review. We explored fixing this in lop-cpu-oslist.dts via lop options, but that mechanism only accepts static key:value entries baked into the lop file. Since we need
the workspace .repo.yaml path supplied at invocation time—and we prefer not to rely on static paths or environment variables—we are proposing to shift this logic to an assist,
where CLI arguments are already supported via options['args'].

│ If this approach works for you, we will incorporate your feedback and submit V2. We would also appreciate any suggestions for passing a runtime CLI argument into a code lop without
static options or env vars.

[zeddii]

Thanks — that's a fair objection, and you were right about the mechanism as it stood. options is static, baked into the lop file, and there was no way to hand a code lop a value that's only known at invocation time. That was a real gap, not something you'd missed.

It isn't a gap any more. I've landed two commits on master:

• b197fd3f — config: keep dot bearing values out of section splitting
• a6ae58c1 — lops: let a code block read the runtime configuration

--cfgval already existed as a runtime CLI mechanism and was already carried on the SDT, but it was never exposed to code lops, and its parser split the whole section.key=value string on ., so any value containing a dot was destroyed — which is every path, and .repo.yaml especially:

--cfgval cpulist.repo_yaml=/ws/build/.repo.yaml
  before -> {'cpulist': {'yaml': 'True'}, 'repo_yaml=/ws/build/': {...}, 'repo': {...}}
  after  -> {'cpulist': {'repo_yaml': '/ws/build/.repo.yaml'}}

So the invocation you need now works:

lopper -f --cfgval cpulist.repo_yaml=/ws/build/.repo.yaml \
       -i lop-cpu-oslist.dts system-top.dts out.dts

lop_0 {
        compatible = "system-device-tree-v1,lop,code-v1";
        inherit = "baremetal_getsupported_comp_xlnx,common_utils";
        code = "
               repo_yaml = config['cpulist']['repo_yaml']
               cortex_os_list = baremetal_getsupported_comp_xlnx.get_os_bsp_keys(
                                    common_utils.load_yaml( repo_yaml ) )
               ";
};

One correction to my earlier review: the import example I gave used a bare from baremetal_getsupported_comp_xlnx import ..., which does not work inside a code block — the assists directory isn't on the path in that context and you'll get ModuleNotFoundError. Use inherit as above; the named modules are then bound in the block. Sorry for the misdirection, I hit it myself when I tested this end to end:

>>> LOP GOT PATH: /ws/build/.repo.yaml
>>> LOP OS KEYS : ['standalone', 'freertos', 'ucos', 'micrium']

No static path, no environment variable, and it generalizes — any code lop can now be parameterized per invocation, so this particular reason to reach for an assist is gone for everyone, not just here.

---

On the wider point, since it's worth saying plainly rather than leaving as a preference.

An assist is easier now. The cost shows up later, and it isn't hypothetical — it's measurable on this very patch. I found six issues in the new file. Five of them are not possible in a code lop, because the lop form doesn't offer the constructs that produced them:

Issue in the assist	Why a lop can't have it
dict(AIE_ENTRY) shallow copy leaks YAML anchors into cpulist.yaml	no module-level constant to share by reference
yaml.dump default sort_keys=True, so key order drifts from the sibling generator	inline dict, one dumper, no parameters to pick
NON_CORTEX_OS holds two Cortex CPU names and isn't a list of OSes	no module scope for a name to outlive its meaning
sys.path.append placed after the imports needing it, and dead regardless	no import bootstrap
or {} swallowing an invalid manifest, contradicting the commit message	no free error handling

The sixth — propval(...)[0] before the emptiness check — is inherited verbatim from the lop and needs fixing wherever this lands.

None of those are mistakes anyone should feel bad about. That's the point: they're the ordinary cost of having more room to move. A code-v1 block has no module-level state, no import bootstrap, and a bounded injected environment, so five of six simply can't be written. Those constraints are doing real work, and they keep paying out every time someone reads or changes the file afterwards. Trading them away for one parameter was a bad exchange, which is why fixing the parameter problem was the better place to spend the effort.

If you'd rather still go the assist route, say so and make the argument — I'd want the six above addressed first, plus a test, since that's the bar the lop form was clearing for free. But the lop rework should now be roughly the diff it always should have been: merge the two lops into one using an OR'd select, os.path.join(outdir, ...) for the output path, config[...] for the manifest, and the [0]-before-check fix.

Whichever way you go, lop-cpu-oslist.dts needs resolving in the same PR — either reworked, or deleted if it's genuinely superseded. Leaving it in the tree writing the same filename with nothing referencing it is something I'd like to avoid.