config: reject a non-list packages section instead of dropping it - #73
config: reject a non-list packages section instead of dropping it#73JoaoMorais03 wants to merge 1 commit into
Conversation
A mapping or scalar under packages: was silently ignored, so a common YAML mistake produced a build with no package dependencies and no error. Signed-off-by: João Morais <118842104+JoaoMorais03@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
packages[].name is not validated as a string (can propagate non-strings into resolver/CLI), and the new version-to-string coercion behavior lacks a direct regression test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Tightens build.yaml config validation so a present-but-invalid packages: section fails fast instead of being silently ignored, preventing accidental “no dependencies” projects due to common YAML shape mistakes.
Changes:
- Require
packages(when present) to be a list of mappings and error on invalid shapes/items. - Enforce
namepresence and coerce non-stringversionscalars tostrduring parsing. - Add config validation tests covering invalid
packagesshapes and valid parsing/omission behavior.
File summaries
| File | Description |
|---|---|
ebuild/core/config.py |
Adds strict validation for packages: entries and coerces non-string versions to strings. |
tests/ebuild/test_config_validation.py |
Adds regression tests for invalid packages: shapes/items and successful parsing/omission. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| pkg_name = pkg.get("name", "") | ||
| pkg_version = pkg.get("version") | ||
| if not pkg_name: | ||
| raise ConfigError("Package definition must have a 'name' field.") | ||
| if pkg_version is not None and not isinstance(pkg_version, str): | ||
| pkg_version = str(pkg_version) | ||
| packages.append(PackageDep(name=pkg_name, version=pkg_version)) |
| def test_omitted_packages_is_empty(tmp_path): | ||
| path = write_config(tmp_path, {"project": {"name": "demo"}}) | ||
| config = load_config(path) | ||
| assert config.packages == [] |
srpatcha
left a comment
srpatcha
left a comment
There was a problem hiding this comment.
Verified — and it is the same failure shape as several others in this repo
The old code:
if isinstance(raw_packages, list):
for p in raw_packages:
if isinstance(p, dict):
...Two silent skips, no else on either. A packages: section written as a mapping instead of a list, or a list containing a bare string, parsed to zero dependencies and the build went on to succeed without them. The developer gets a binary missing everything they asked for and nothing anywhere says why.
That is the same shape as the MISS case I have been adding to the build summary, as ebuild add writing entries that cannot resolve, and as the ecosystem runner reporting passes for tests that never ran. Raising ConfigError at the point of the malformed input is the right treatment: the message names the field, the expected shape, and what was found.
raise ConfigError(
"Invalid package definition: expected a YAML mapping, "
f"got {type(pkg).__name__}."
)Requiring name is right too — a package entry without one is not a package, and previously it was dropped without comment.
One thing I would consider, not blocking
if pkg_version is not None and not isinstance(pkg_version, str):
pkg_version = str(pkg_version)This is coercion where the rest of the change is validation, and it is load-bearing: YAML parses version: 1.2 as a float, and str(1.2) is "1.2" — but version: 1.20 also parses as a float and stringifies to "1.2", silently losing the trailing zero. Worth either rejecting a non-string version and telling the author to quote it, or keeping the coercion with a comment about which YAML scalars it is absorbing. Either is defensible; the silent narrowing is the part I would not leave undocumented.
Merge order
Currently red on master for a reason unrelated to this PR — origin/master has a SyntaxError in ebuild/build/dispatch.py that makes the suite uncollectable. Merged on top of #66, which repairs it: 215 passed. Needs #66 first, then this goes in as-is.
Verification
Merged onto origin/master + #66; pytest 215 passed.
Contribution after reviewing the project:
load_config()already rejects a non-listtargets:/toolchain/backend_config, but apackages:that is a mapping or scalar was dropped with no error. A common YAML mistake (packages: {name: zlib, version: "1.2.13"}instead of a list) therefore produced a project with zero package deps.Issue
ebuild/core/config.pytreatedpackages:as optional and only walked it whenisinstance(raw_packages, list). Anything else — including a mapping, which is what you get if the-is omitted — was silently ignored. Individual list items that were not mappings, and mappings withoutname, were also skipped.Approach
If
packagesis present, require a list of mappings with anamefield (same shape astargets). Non-string YAML versions (e.g.version: 1.0parsed as a float) are coerced tostrsoPackageDep.versionstays a string. Omittedpackagesstill means no dependencies.Testing
Added cases in
tests/ebuild/test_config_validation.py:packages:raisesConfigErrornameraisespackagesis emptyValidated by reading the parser and matching the existing target/toolchain tests. I did not run the full pytest suite in this environment (no local checkout of the repo).
Limitations
Does not invent a shorthand
packages: [zlib]form. Does not validate thatversionis a useful version string, only that it can be stored asstr. Duplicate package names are still allowed.