Skip to content

Support additive FAT files via files_append - #28

Merged
mobileoverlord merged 9 commits into
mainfrom
eng-2134-stone-files-append
Aug 12, 2026
Merged

Support additive FAT files via files_append#28
mobileoverlord merged 9 commits into
mainfrom
eng-2134-stone-files-append

Conversation

@jetm

@jetm jetm commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Problem

The device-tree-overlay feature needs to add compiled .dtbo files to a target's FAT boot partition at bundle time, but stone's manifest had no way to append files to an existing FAT image.

Solution

Add files_append support to the manifest so additional files are injected into a FAT partition image without redefining it.

Key changes

  • manifest: support additive FAT files via files_append
  • provision: bind guarded partition size with if let

Reviewer notes

Enabler for the Avocado device-tree-overlay feature (ENG-2134). meta-avocado's overlay delivery hook and the avocado-cli DTO commands build on this. First to merge in the stack: stone -> meta-avocado -> avocado-cli.

jetm added 2 commits July 17, 2026 13:56
`cargo clippy --all-targets -- -D warnings`, which CI enforces, flags
`partition.size.unwrap()` guarded by a preceding `partition.size.is_some()`
as `unnecessary_unwrap`. The two-step check-then-unwrap also lets the guard
and the use drift apart.

Match the value once with `if let Some(size)` and use the binding directly,
so the guard cannot diverge from the unwrap and clippy passes clean.

Signed-off-by: Javier Tia <javier@peridio.com>
Both supported boot chains deliver a custom device-tree overlay by
enumerating it in a FAT partition's `files` list, but that list is
authored by the BSP and stone replaces (never merges) a manifest's
file arrays. A project could therefore only add its own overlay by
restating the entire BSP file list, which goes stale the moment the
BSP changes its boot files.

Add a `files_append` field the FAT builder concatenates onto `files`,
keyed by output path. An identical {in,out} re-emitted on an incremental
rebuild collapses to a no-op, so a regenerative delivery step is safe to
run every build; two entries targeting the same output from different
inputs error out instead of silently overwriting a boot file. Both build
paths (bundle and provision) and the describe output honor the field, and
manifests without it still parse.

Also normalizes pre-existing rustfmt drift in the touched files (and one
adjacent bundle test) so `cargo fmt --check` passes; the repo's drift was
confined to exactly these files.

Signed-off-by: Javier Tia <javier@peridio.com>

@jetm jetm left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Cold second read of my own change. Draft, but posting rather than sitting on it - three of these mean the feature does not work end to end as described, and CI being green is consistent with all of them: every finding is either an unexercised code path or a case the 51 passing tests do not construct.

merge_fat_files itself is wired correctly. What is not wired is everything downstream of it - staging, validate, describe-manifest - and the --overlay path in front of it.

Eight findings inline. One more that could not be anchored because the file is not in the diff at all, which is itself the point:

src/commands/stone/validate.rs:181 skips files_append entirely. crate::manifest::BuildArgs::Fat { files, .. } => files.as_slice(), drops it, so validate neither checks that appended sources exist nor runs merge_fat_files (only called at bundle.rs:395 and provision.rs:214). A manifest with files: ["file_1"] and files_append: [{"in":"a.dtbo","out":"file_1"}], both inputs present, gets Validated. from validate and a hard failure from bundle. CI gating on stone validate learns nothing, and the "must surface at build time" property is absent from the one command whose whole job is checking a manifest before you build.

What I checked and found correct, so it does not get relitigated:

  • Schema compat holds. A files-only manifest deserializes identically (pinned by test_fat_build_args_files_append_defaults_empty) and merge_fat_files preserves base order. The one exception is the duplicate-out thread below.
  • Path escape is safe. add_file_to_fat (fat.rs:339-352) skips Component::RootDir and resolves .. inside the FAT volume through fatfs open_dir, so a hostile out cannot write outside the image file.
  • 8.3 vs LFN is handled - fatfs compares both long and short names. I did not check FAT12/16 root-directory entry-count limits or path-length ceilings.
  • Size overflow fails loudly, it does not truncate. Partition size is read before the merge (bundle.rs:388, provision.rs:263), and fatfs returns No space left on device from table.rs:310 via alloc_cluster (file.rs:271), which bundle.rs:418 propagates with ? before the copy at :422. No bundle is produced. Two diagnosability nits: the error is ErrorKind::Other with the detail only in the string, so callers cannot match on kind, and the message names the file rather than the overflowing partition.
  • File ordering is deterministic - Vec order, base then append, no glob and no HashMap in the file path.

One pre-existing thing worth its own change, not this one: the image is not byte-reproducible. fatfs's DEFAULT_TIME_PROVIDER uses chrono::Local::now() (time.rs:137-150), stamped on every entry (dir.rs:410-413) and every write (file.rs:43-44), and the chrono feature is on transitively because Cargo.toml declares fatfs = "0.3" without default-features = false. So output varies with the builder's timezone, not just the instant. Fixable via FsOptions::new().time_provider(...) with a fixed-epoch provider. Lives in src/fat.rs, untouched here.

Comment thread src/manifest.rs
Comment thread src/manifest.rs
Comment thread src/manifest.rs Outdated
Comment thread src/manifest.rs Outdated
Comment thread src/manifest.rs
Comment thread src/manifest.rs
Comment thread src/commands/stone/describe_manifest.rs
Comment thread tests/commands/stone/bundle/mod.rs
@jetm
jetm marked this pull request as ready for review August 7, 2026 14:20
jetm and others added 7 commits August 7, 2026 10:42
Every test for this feature was a unit test over merge_fat_files in
isolation, so nothing exercised bundle, create, validate or
describe-manifest with an append entry. That shape is not incidental: it
is exactly why every consumer outside merge_fat_files was left unwired
and still passed CI. The accessor could keep its #[allow(dead_code)] and
the suite stayed green.

All four fail on the current head, each for its own reason: the appended
input is never staged, a case-variant collision is accepted, a
base-only duplicate output now aborts a build that used to work, and
describe-manifest prints a count of appended files above a list naming
none of them.

Committed before the fixes so the failures are in the history rather
than asserted afterwards, and so a later reader can check that each one
actually discriminates.

Signed-off-by: Javier Tia <javier@peridio.com>
Four defects in one file, all reachable from a delivery hook that emits
an overlay - which is the only way this feature is meant to be used.

An overlay's files_append replaced the base list instead of extending it.
deep_merge_json element-merges arrays only when every element carries a
"name", and FileEntry is {in,out}, so two hooks each appending one .dtbo
left one of them. The key inherited the exact array-clobbering it exists
to route around, and the collision guard never saw the dropped entry.
Appending is keyed on the field name rather than on element shape so
`files` keeps its replace semantics - that difference is why
files_append had to exist in the first place.

The collision guard compared output paths verbatim while FAT resolves
them case-insensitively. fatfs uppercases both sides in eq_name and
create_file returns the existing entry without truncating, so
overlays/VC4.DTBO over overlays/vc4.dtbo produced one directory entry
holding the overlay followed by whatever of the original ran past it -
the brick the guard exists to prevent, reached through a spelling it did
not check. Normalizing case and path form closes it.

The guard also compared base entries against each other, so a manifest
with two `files` sharing an output - which built before, fatfs being
last-write-wins - started failing at bundle time. Nothing announced that
and no test pinned it. Dedup is a property of appending, so the check
now runs only over appended entries.

The error string carried its own [ERROR] prefix while main.rs routes it
through log_error, which adds one. Every other error string is bare.

Image::all_files() is the accessor the feature needed and never got:
Image::files() returns the base list alone, which is what left every
staging consumer copying base inputs and skipping appended ones.

Signed-off-by: Javier Tia <javier@peridio.com>
The accessor existed and nothing called it, so the feature worked only
as far as serde. bundle staged base inputs and skipped appended ones,
producing an .aos that names overlays/a.dtbo without copying a.dtbo -
provision then dies with "not found in any input directory", one command
later and far from the cause. create was worse: it printed "Created."
and silently omitted the file, where a missing base input correctly
aborts.

validate is the one that matters most for CI. It read base `files`
directly and never called merge_fat_files, so it printed "Validated." on
a manifest that bundle then hard-fails on - the single outcome a
pre-build check must not produce. A collision returns Err rather than
joining the missing-file tally, because that tally counts and lists
absent inputs and this is two entries that cannot coexist.

describe-manifest listed base files under a count that included appended
ones, so its own output disagreed with itself: "Files (2)" above one
name. That listing is the operator's only confirmation an append landed
at all, which makes a count with nothing to check it against the least
useful thing it could print.

Signed-off-by: Javier Tia <javier@peridio.com>
HELD - not pushed. See the parked decision on the src/manifest.rs:27 thread:
this refusal makes three shipped meta-avocado manifests fail to parse, because
they set a FAT `label` that BuildArgs::Fat has no field for.

`#[serde(default)]` with no deny_unknown_fields meant a key nested one level off
or misspelled parsed clean and yielded an image without the overlay. files_append
is the first key here whose author is a delivery hook rather than a person, so
nothing reads the output: the board boots without its device-tree overlay and a
missing line in describe-manifest is the only signal. Reproduced - `file_append`
built a bundle and exited 0.

The attribute sits on the enum rather than the variant because
deny_unknown_fields is a container attribute, and serde's derive exempts the
`type` discriminator that internal tagging puts in the same map. A test asserts
that second half, since it is the interaction that would silently break every
manifest rather than just the malformed ones.

The refusal cannot name the offending key. `Image` is untagged, so serde discards
the inner variant errors and reports "did not match any variant" with a line
number pointing at the end of the enclosing object. The test pins the refusal and
the absence of an output bundle rather than a diagnostic the parser cannot give.

Signed-off-by: Javier Tia <floss@jetm.me>
…mage

Three shipped meta-avocado manifests - stm32mp25-dk, orangepi-5-plus and
rzv2n-sr-som - set `"label": "BOOT"` on a fat build_args. src/fat.rs has carried
the volume-label write the whole time (`with_label`, `volume_label()`), but
BuildArgs::Fat had no field to plumb it from, so serde's default posture dropped
the key and those boot partitions were labelled FATFS. Nothing reported it: the
manifest asked, the build succeeded, and the image disagreed.

That is the same failure the unknown-key refusal in the previous commit exists to
prevent, which is how it surfaced - the refusal turned three silently-ignored
keys into three parse errors, and the keys turned out to be asking for something
real rather than misspelled.

Plumbed through both build paths, because they construct FatImageOptions
separately and only fixing one would leave `stone provision` disagreeing with
`stone bundle` about the same manifest. Exhaustive destructuring is what found
the second pair of sites: adding the field broke describe-manifest's two matches
at compile time rather than leaving them silently unaware of it.

describe-manifest now reports the label too. It is the operator's confirmation
surface, and the label was the one build_arg it did not show, so a manifest
asking for BOOT read identically to one asking for nothing.

Verified by reading the built image's boot sector rather than trusting the call:
BS_VolLab at offset 0x47 reads BOOT with the key present and FATFS without it, so
the default is unchanged for every manifest that does not set one. All 30 shipped
manifests parse again - 22 still fail afterwards on a pre-existing offset-unit
mismatch that predates this branch.

Signed-off-by: Javier Tia <floss@jetm.me>
`main` funnels every Err through `log_error`, which prints `[ERROR] {message}`.
Eighteen error strings across manifest.rs and provision.rs carried a literal
`[ERROR] ` of their own, so what reached the operator read:

    [ERROR] [ERROR] Failed to parse manifest JSON 'stone.json': ...

Cosmetic while the paths that produced it were genuinely exceptional. The
unknown-key refusal earlier in this branch changed that: an ordinary typo in a
build_args key now takes the parse-error path, so the doubled prefix went from
rare to the first thing anyone hits after misspelling a field.

Fixed at the source rather than at the two parse sites. Every one of these
strings has exactly one consumer - main.rs - and patching only the message this
branch made prominent would have left the other sixteen to resurface one at a
time, each looking like a fresh bug.

The prefix belongs to log_error, which is the only code that knows whether output
is going to a terminal.

Signed-off-by: Justin Schneck <j.schneck@peridio.com>
The unknown-key refusal earlier in this branch is the right call, but the message
it produced could not be acted on. serde's `deny_unknown_fields` generates a good
diagnostic:

    unknown field `file_append`, expected one of `variant`, `files`, `files_append`, `label`

`Image` was `#[serde(untagged)]`, and serde discards every variant's error when
all of them fail, so that never left the parser. What the operator saw was:

    Failed to parse manifest JSON 'stone.json':
      data did not match any variant of untagged enum Image at line 31 column 7

Line 31 is the closing brace of the enclosing `images` object; the offending key
was on line 21. Refusing a key is only useful if the refusal says which key, so
the attribute and this commit belong together.

Replaced the derived impl with a hand-written one. Dispatch on the JSON shape - a
string is the `String` form, anything else must be the object form - which is
unambiguous, so the object branch's own error propagates verbatim instead of
being one of two guesses discarded on failure.

This fixes more than the typo case. Any malformed object under `images`
collapsed to the same message: a manifest that simply omitted `size_unit` also
reported "did not match any variant" rather than "missing field `size_unit`".

`ImageObject` mirrors the variant's fields to have a derived deserializer to
delegate to, and `Image::Object` is constructed exhaustively from it on purpose:
a field added to the variant fails to compile until it is added here too, so the
two cannot drift into a silent parse regression. The same treatment is available
for `FileEntry`, which has the identical shape, if `deny_unknown_fields` ever
goes on it.

Position information is the one thing lost - `serde_json::from_value` drops
spans, so the reported line is still the enclosing object's end. The key name is
greppable, which the line number was not.

Kept `Serialize` derived. `an_image_object_round_trips_through_serde` pins the two
against each other, because a field-name mismatch between a hand-written reader
and a derived writer would otherwise stay hidden until a merged manifest failed
to re-read.

Rewrote the assertion in `bundle_rejects_a_misspelled_files_append`. It asserted
only that the parse failed, with a comment reasoning that naming the key "would
pin a diagnostic the parser cannot give" - true of the untagged enum, and the
reason this was worth fixing rather than documenting.

Verified by reverting both changes with the new tests in place: three unit tests
and three integration tests fail, reporting the untagged message and the doubled
prefix. The compatibility guards - both Image forms, the round-trip, the
bare-filename image every manifest here uses - pass either way by design.

Signed-off-by: Justin Schneck <j.schneck@peridio.com>

@mobileoverlord mobileoverlord 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.

Concept is right and the implementation is careful. The fat_output_key normalization is the standout — fatfs resolving case-insensitively while create_file returns the existing entry without truncating is exactly the kind of thing that ships as a brick, and catching it at merge time rather than after the image is written is the correct place.

Verified locally against a Pi 5 style manifest: both overlays land in overlays/, the appended source is staged into the build dir (the bug that had provision failing a command later), the label reaches BS_VolLab, an identical re-emit collapses, and a case-variant collision aborts.

Pushed two follow-up commits rather than blocking on them:

  • deny_unknown_fields is the right call, but Image being #[serde(untagged)] discarded the good diagnostic serde generates. unknown field \file_append`, expected one of ...was being collapsed todata did not match any variant of untagged enum Imageat a line number pointing past the mistake. Replaced the derived impl with a hand-written one that dispatches on JSON shape and propagates the object error verbatim. Also fixes any malformed image object — a droppedsize_unit` reported the same useless message.
  • The literal [ERROR] in 18 error strings doubled against log_error's own prefix. Pre-existing, but this branch routes ordinary typos through that path, so it stopped being cosmetic.

Two things left for you, neither blocking:

  • fat_label() is dead — bundle.rs and provision.rs destructure label directly from the variant. And fat_files_append()'s #[allow(dead_code)] is now stale since all_files() calls it.
  • The strictness is still asymmetric. expnd for expand builds clean and silently gives you a non-expanding partition; blocksize for block_size likewise. Same failure class the deny exists to prevent, and arguably worse than a missing overlay. Worth extending to Partition/StorageDevice/Image in a follow-up with its own compat discussion.

Also worth adding deny_unknown_fields to the PR description — it's a breaking parse change for any manifest carrying a stray key, which isn't obvious from the title.

@mobileoverlord
mobileoverlord merged commit d05fc71 into main Aug 12, 2026
4 checks passed
@mobileoverlord
mobileoverlord deleted the eng-2134-stone-files-append branch August 12, 2026 15:21
@mobileoverlord mobileoverlord mentioned this pull request Aug 12, 2026
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.

2 participants