config: treat runtime package compile sections as active - #202
config: treat runtime package compile sections as active#202mobileoverlord wants to merge 3 commits into
Conversation
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.
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.
There was a problem hiding this comment.
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>.compilereferences. - 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.compilereferences.
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
jetm
left a comment
There was a problem hiding this comment.
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.
|
Both taken, in 63b69ba. Target scoping. Fixed rather than deferred, on your reasoning: the gap predates the branch on the 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: 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 ( CHANGELOG. Added under
|
jetm
left a comment
There was a problem hiding this comment.
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.
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.
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.
What
find_active_compile_sectionscounts a compile section as active if it is reached from an extension or fromruntimes.<n>.kernel.compile. A section reached throughruntimes.<n>.packages.<pkg>.compilewas never counted.Consequences, in order of how confusing they are:
need_target_devstaysfalse, so no target sysroot is created at all —$AVOCADO_PREFIX/sdk/target-sysrootis simply absent (verified:sdk/contains onlytarget-repoconfandx86_64).packages:list is silently discarded.--sysrootpointing 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#23adds auboot-custom-imx8mp-evkreference wired as:It failed first on
sys/types.h, then — after that was chased down — oncannot find -lgccat the U-Boot link, because targetlibgcc.aonly exists inlibgcc-s-devand that request was being dropped. The reference currently works around it by declaring the section underkernel:instead, with a comment pointing here; that workaround can be reverted once this lands.Tests
Three tests in
src/utils/config.rs:main, passes here)avocado-runtime: '*') are not mappings and must not be misreadkernel.compileand dedup tests still passcargo test --bin avocado— 1401 passed. fmt and clippy clean.Independent of #200 and #201.