Skip to content

config: treat runtime package compile sections as active - #202

Open
mobileoverlord wants to merge 3 commits into
mainfrom
fix/active-compile-sections-runtime-packages
Open

config: treat runtime package compile sections as active#202
mobileoverlord wants to merge 3 commits into
mainfrom
fix/active-compile-sections-runtime-packages

Conversation

@mobileoverlord

Copy link
Copy Markdown
Contributor

What

find_active_compile_sections counts a compile section as active if it is reached from an extension or from runtimes.<n>.kernel.compile. A section reached through runtimes.<n>.packages.<pkg>.compile was never counted.

Consequences, in order of how confusing they are:

  1. need_target_dev stays false, so no target sysroot is created at all$AVOCADO_PREFIX/sdk/target-sysroot is simply absent (verified: sdk/ contains only target-repoconf and x86_64).
  2. The section's own packages: list is silently discarded.
  3. The compile script still runs, with --sysroot pointing at a nonexistent directory. GCC tolerates that and falls back to built-in paths.

So a perfectly valid config has no effect, and the build fails much later on a missing target header or library with nothing pointing back at the cause.

How it was found

references#23 adds a uboot-custom-imx8mp-evk reference wired as:

runtimes:
  dev:
    packages:
      uboot:
        compile: uboot
sdk:
  compile:
    uboot:
      packages:
        libgcc-s-dev: '*'      # silently ignored

It failed first on sys/types.h, then — after that was chased down — on cannot find -lgcc at the U-Boot link, because target libgcc.a only exists in libgcc-s-dev and that request was being dropped. The reference currently works around it by declaring the section under kernel: instead, with a comment pointing here; that workaround can be reverted once this lands.

Tests

Three tests in src/utils/config.rs:

  • the runtime-package case (fails on main, passes here)
  • scalar package values (avocado-runtime: '*') are not mappings and must not be misread
  • the existing kernel.compile and dedup tests still pass

cargo test --bin avocado — 1401 passed. fmt and clippy clean.

Independent of #200 and #201.

find_active_compile_sections only counted compile sections reached from an
extension or from `runtimes.<n>.kernel.compile`. A section reached through
`runtimes.<n>.packages.<pkg>.compile` was never counted, so:

  * need_target_dev stayed false and no target sysroot was created at all
    ($AVOCADO_PREFIX/sdk/target-sysroot is simply absent), and
  * the section's own `packages:` were silently discarded.

The compile script still runs, with --sysroot pointing at a directory that
does not exist, so the failure surfaces much later as missing target headers
or libraries with nothing connecting it back to the ignored config. Silently
dropping a declared `packages:` list is the worst part -- the user's config
is valid and simply has no effect.

Scan runtime package entries for `compile` too. Also covers scalar package
values (`avocado-runtime: '*'`), which are not mappings.
Copilot AI lite review requested due to automatic review settings August 13, 2026 13:39
mobileoverlord added a commit to avocado-linux/references that referenced this pull request Aug 13, 2026
This reference had never built -- Build Check only ever ran on this branch,
so nothing had exercised it end to end. Remaining fixes after the HOSTCC and
OpenSSL ones:

* Host headers for U-Boot's tools/: libuuid + gnutls (tools/mkeficapsule.c)
  and libz. `make all` builds every host tool, not just mkimage.
* Static archives (nativesdk-libc6-staticdev, nativesdk-libz-staticdev):
  imx-mkimage builds mkimage_imx8 with `-static -lz`.
* libgcc-s-dev in the target sysroot: U-Boot links -lgcc, and the
  cross-canadian compiler resolves libgcc.a from the target sysroot. There is
  no USE_PRIVATE_LIBGCC escape hatch in this U-Boot.
* imx-mkimage hardcodes `CC = gcc`; pass CC on the make command line, which
  overrides even a hard `=` and propagates to the sub-make.
* Build imx-mkimage from inside its own directory: it derives the
  mkimage_imx8 output path from the invoking PWD, so `make -C` dropped the
  host tool into the reference directory where `clean` never removed it.

The compile hook moves from `runtimes.dev.packages.uboot` to
`runtimes.dev.kernel` because the CLI only provisions a target sysroot for
compile sections reached via kernel.compile or an extension; under packages
the section's `packages:` were silently ignored and no target sysroot existed
at all. See avocado-linux/avocado-cli#202 -- revert that bit once it lands.
The Linux kernel is unaffected; it still comes from avocado-runtime.

Verified locally from a clean state (avocado clean -f, install, build): the
full TF-A BL31 -> U-Boot SPL/proper -> imx-mkimage chain produces flash.bin,
and the imx-boot installed into the runtime build dir is byte-identical to it.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes find_active_compile_sections so that compile sections referenced via runtimes.<name>.packages.<pkg>.compile are treated as active, matching the behavior already implemented for extensions and runtimes.<name>.kernel.compile. This ensures the target sysroot is provisioned and any sdk.compile.<section>.packages dependencies are not silently dropped when a runtime package points at a compile section.

Changes:

  • Extend active compile-section discovery to include runtimes.<name>.packages.<pkg>.compile references.
  • Add unit tests covering the runtime-package compile reference path and ensuring scalar package values aren’t misread as mappings.
  • Retain existing behavior around deduplication and kernel.compile references.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@jetm
jetm self-requested a review August 13, 2026 17:26

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix is correct and the premise checks out: find_active_compile_sections has exactly one production caller (src/commands/sdk/install.rs:347), where it gates need_target_dev and drives the target-dev package list, so the failure the commit body describes is real. The packages.<pkg>.compile shape is genuine and documented (docs/features/sdk-cross-compilation-rpm-packaging.md:108), and find_compile_dependencies_in_ext already handles the identical shape for extensions, so this closes a real asymmetry. No panic risk on the scalar case — as_mapping() and get("compile") both return None for avocado-runtime: '*', which your second test pins. cargo fmt, cargo clippy --all-targets --all-features -- -D warnings and cargo test --lib find_active_compile_sections all pass at 457ce34 (4 tests, including the two new ones).

One thing worth a look before merge.

The new loop iterates every runtime, ignoring runtimes.<n>.target and --runtime. src/utils/config.rs:5770. Its sibling find_active_extensions, called from the same place (install.rs:185), scopes through find_target_relevant_runtimes first. The gap is pre-existing on the kernel.compile line, so this is not a regression you introduced — but it widens it from the rare kernel.compile case to the common packages.<pkg>.compile one, which is the point of the PR. In a multi-target config that means provisioning target-dev for runtimes the user did not ask for.

One nit: no CHANGELOG.md Unreleased/Fixed entry for a user-visible bugfix. Weak signal though — only 5 of the last 15 src-touching commits added one, so the convention does not look enforced here.

Disclosure on my own verification: I tried the delete-the-hunk check to prove the new test actually fails without the production change, and my tooling blocked the edit, so I did not observe the RED. The static argument holds — test_find_active_compile_sections_via_runtime_package has no extensions and no kernel.compile, so the new block is the only path that can produce ["uboot"] — but I am telling you that as reasoning rather than as something I watched fail.

find_active_compile_sections read every runtime in the file, ignoring
runtimes.<name>.target and --runtime. Its sibling find_active_extensions,
called from the same place in sdk install, scopes through
find_target_relevant_runtimes first. The gap predates this branch on the
kernel.compile line, but widening it from that rare case to the common
packages.<pkg>.compile one is this branch's doing, so it is fixed here:
in a multi-target config it provisioned a target-dev sysroot for runtimes
the user had not asked to build.

The function now takes the same (config, target, config_path,
requested_runtime) tuple as its sibling and returns Result.

New test pins it: two runtimes with different targets, each referencing
its own compile section, asserted from both directions. It goes red
against a reverted body -- ["uboot-rpi4", "uboot-x86"] instead of
["uboot-x86"] -- while the four older tests stay green under the same
mutation, which is what shows they were not covering this.

The four existing tests now build a real Config from a temp file rather
than a bare serde_yaml::Value, since scoping needs the merge.

Also adds the CHANGELOG entry this was missing, covering both the
original fix and the scoping.
@mobileoverlord

Copy link
Copy Markdown
Contributor Author

Both taken, in 63b69ba.

Target scoping. Fixed rather than deferred, on your reasoning: the gap predates the branch on the kernel.compile line, but this PR is what moves it from a rare case to the common one, so it belongs here. find_active_compile_sections now takes the same (config, target, config_path, requested_runtime) tuple as find_active_extensions and scopes through find_target_relevant_runtimes first, so the two scans called from install.rs agree about which runtimes exist.

On the RED you couldn't observe — worth closing that loop, since your static argument was right but you were explicit about not having watched it fail. I reverted the loop body to iterating every runtime and ran it:

test test_find_active_compile_sections_scopes_to_the_target ... FAILED
  left: ["uboot-rpi4", "uboot-x86"]
 right: ["uboot-x86"]

That is the new test, and the failure output is the bug verbatim — both targets' compile sections. The four older tests stayed green under that same mutation, which is what shows the coverage was genuinely absent rather than incidental.

The new test asserts from both directions (qemux86-64uboot-x86, raspberrypi4uboot-rpi4) so it fails on a scan that returns everything and on one that returns nothing. The four existing tests now build a real Config from a temp file instead of a bare serde_yaml::Value, since scoping needs the merge to resolve runtimes.<n>.target.

CHANGELOG. Added under Fixed, covering the original bug and the scoping as one entry. Your read of the convention matches mine — it is inconsistent in recent history — but the argument for adding it does not depend on that: the failure mode here is a build dying much later on missing target headers, and that is precisely the kind of thing someone greps a changelog for.

cargo fmt, clippy --all-targets --all-features -D warnings and 1393 lib tests green at 63b69ba.

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed the increment since 457ce34 - the scan-scoping and changelog commit answering my last pass. Three blocking findings inline; the two in config.rs were reproduced with throwaway probe tests, and both are cases where the narrowing now misses a section it should still find.

Seven advisory notes were withheld rather than appended here, so the blocking ones stay readable.

Comment thread src/utils/config.rs Outdated
Comment thread src/utils/config.rs
Comment thread CHANGELOG.md Outdated
Three findings from the last pass, all in the scoping the previous commit
added. Both config.rs ones reproduced first as failing tests, and each fix
was confirmed to be independently necessary by disabling the other.

Selection was merged, the body read was raw. find_target_relevant_runtimes
picks runtime names through get_merged_runtime_config, but the scan then
read the runtime body out of the un-resolved mapping, so a compile ref
declared inside a `target-<t>:` block was invisible. Worse than missing it
outright: need_target_dev stayed false so no target-dev sysroot was
installed, while the compile-deps stamp was still written and later
accepted on existence alone, so runtime build ran the compile script
against a sysroot holding none of its declared packages. The scan now reads
the merged body, falling back to the raw one when there is no merged value.

A runtime declaring `target:` only inside a `target-<t>:` block defeated
the narrowing. Override resolution strips the non-matching blocks, so the
merged runtime arrives with no `target` key and fell into the
include-for-all-targets branch: `sdk install --target qemux86-64` pulled a
raspberrypi4-only runtime's compile packages into the x86-64 target
sysroot, the exact case the previous commit's comment claimed to prevent.
declares_target_only_for_other_targets closes it, conservatively - a
`target-<current>:` block keeps the runtime in scope even when it declares
no `target:` of its own, since the author plainly wrote something for this
target. The deprecated bare-target-name form is left alone.

Because that helper is shared, commands/install.rs's independently
maintained copy of the same selection logic would have kept the hole open
on the extension and runtime steps of an install while the SDK step was
fixed. It now delegates to the shared helper, which also drops ~55 lines of
drifted duplicate.

CHANGELOG claimed `--runtime` scoping that does not exist: SdkInstallCommand
has no runtime field and passes requested_runtime: None, so `install -r dev`
still gets an unscoped scan. Dropped the clause rather than threading the
flag through, since doing that properly also means scoping
find_active_extensions and is a behavior change on its own terms. Filed
separately.

cargo fmt, clippy --all-targets --all-features -D warnings, 1403 lib tests,
1412 bin tests, doc tests, and the target_precedence/interpolation suites
are green.
nicksinas pushed a commit to avocado-linux/references that referenced this pull request Aug 18, 2026
This reference had never built -- Build Check only ever ran on this branch,
so nothing had exercised it end to end. Remaining fixes after the HOSTCC and
OpenSSL ones:

* Host headers for U-Boot's tools/: libuuid + gnutls (tools/mkeficapsule.c)
  and libz. `make all` builds every host tool, not just mkimage.
* Static archives (nativesdk-libc6-staticdev, nativesdk-libz-staticdev):
  imx-mkimage builds mkimage_imx8 with `-static -lz`.
* libgcc-s-dev in the target sysroot: U-Boot links -lgcc, and the
  cross-canadian compiler resolves libgcc.a from the target sysroot. There is
  no USE_PRIVATE_LIBGCC escape hatch in this U-Boot.
* imx-mkimage hardcodes `CC = gcc`; pass CC on the make command line, which
  overrides even a hard `=` and propagates to the sub-make.
* Build imx-mkimage from inside its own directory: it derives the
  mkimage_imx8 output path from the invoking PWD, so `make -C` dropped the
  host tool into the reference directory where `clean` never removed it.

The compile hook moves from `runtimes.dev.packages.uboot` to
`runtimes.dev.kernel` because the CLI only provisions a target sysroot for
compile sections reached via kernel.compile or an extension; under packages
the section's `packages:` were silently ignored and no target sysroot existed
at all. See avocado-linux/avocado-cli#202 -- revert that bit once it lands.
The Linux kernel is unaffected; it still comes from avocado-runtime.

Verified locally from a clean state (avocado clean -f, install, build): the
full TF-A BL31 -> U-Boot SPL/proper -> imx-mkimage chain produces flash.bin,
and the imx-boot installed into the runtime build dir is byte-identical to it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants