diff --git a/tools/tailor/crates/tailor-config/src/fragment.rs b/tools/tailor/crates/tailor-config/src/fragment.rs index e899e91254..01fca4c6e5 100644 --- a/tools/tailor/crates/tailor-config/src/fragment.rs +++ b/tools/tailor/crates/tailor-config/src/fragment.rs @@ -41,6 +41,12 @@ pub(crate) struct Fragment { pub(crate) params: IndexMap, #[serde(default)] pub(crate) rpm_sources: Vec, + /// Extra local paths (scripts, injected assets referenced only from `config:` text, etc.) that + /// invalidate this cell's incremental fingerprint when their content changes; concatenated across + /// matched fragments (base -> most-specific), mirroring `rpmSources`. Resolved the same way as the + /// image-level `extraDependencies` (relative to the image directory, not this fragment's own). + #[serde(default)] + pub(crate) extra_dependencies: Vec, /// Extra IC command-line flags this fragment contributes; concatenated across matched fragments /// (base → most-specific), mirroring `rpmSources`. #[serde(default)] diff --git a/tools/tailor/crates/tailor-config/src/render.rs b/tools/tailor/crates/tailor-config/src/render.rs index 45ac3b6cfa..d8acd91d00 100644 --- a/tools/tailor/crates/tailor-config/src/render.rs +++ b/tools/tailor/crates/tailor-config/src/render.rs @@ -63,6 +63,9 @@ pub struct RenderedCell { pub outputs: Vec, /// Local RPM sources (directories or `.repo` files) passed to IC as `--rpm-source`. pub rpm_sources: Vec, + /// Extra local dependency paths, concatenated across matched fragments; content-hashed into the + /// incremental fingerprint alongside the image-level `extraDependencies`. + pub extra_dependencies: Vec, /// Extra IC command-line flags appended verbatim, concatenated across matched fragments. pub extra_params: Vec, /// The resolved per-cell `signing:` opt-in after fragment merging. @@ -124,7 +127,14 @@ pub fn merge_plan( .map(|f| MergeStep { label: f.label.clone(), reason: f.reason(), - includes: collect_includes(f.doc.config.as_ref()), + // `base:` fragments can `$include` a shared snippet too (`resolve_base` below), so the + // explain plan must report those targets alongside `config:`'s, or it silently omits + // whichever base the fragment actually spliced in. + includes: { + let mut includes = collect_includes(f.doc.config.as_ref()); + includes.extend(collect_includes(f.doc.base.as_ref())); + includes + }, }) .collect()) } @@ -192,7 +202,7 @@ fn render_cell( let mut ic_config = Value::Mapping(config); interpolate::interpolate_tree(&mut ic_config, &context)?; - let base = resolve_base(image, &tuple, &matched, &context)?; + let base = resolve_base(image, image_dir, &tuple, &matched, &context)?; let outputs = resolve_outputs(image, &tuple, &matched)?; for output in &outputs { validate_output_compression(image, &tuple, output)?; @@ -201,6 +211,10 @@ fn render_cell( .iter() .flat_map(|f| f.doc.rpm_sources.clone()) .collect(); + let extra_dependencies = matched + .iter() + .flat_map(|f| f.doc.extra_dependencies.clone()) + .collect(); let extra_params = matched .iter() .flat_map(|f| f.doc.extra_params.clone()) @@ -225,6 +239,7 @@ fn render_cell( base, outputs, rpm_sources, + extra_dependencies, extra_params, signing, skip, @@ -256,13 +271,17 @@ fn merge_params(matched: &[&LoadedFragment]) -> Result, fn resolve_base( image: &ImageDefinition, + image_dir: &Path, tuple: &AxisTuple, matched: &[&LoadedFragment], context: &interpolate::Context, ) -> Result { let mut base: Option = None; for fragment in matched { - if let Some(value) = fragment.doc.base.clone() { + if let Some(mut value) = fragment.doc.base.clone() { + // Mirror config:'s $include handling (resolved before merge, see render_cell above) so a + // base: fragment can splice a shared snippet too, instead of only config: being able to. + include::resolve_includes(&mut value, image_dir)?; base = Some(merge::merge_field( base, value, @@ -500,6 +519,41 @@ mod tests { .collect() } + #[test] + fn merge_plan_reports_includes_from_base_as_well_as_config() { + // `resolve_base` splices `$include` under `base:` just like `config:` does, so the explain + // plan must surface it too -- otherwise `tailor explain` silently omits a library an image + // actually depends on. + let tmp = TempDir::new().unwrap(); + write( + tmp.path(), + "image.yaml", + indoc! {" + name: based + matrix: + arch: [amd64] + outputs: + - format: cosi + config: {} + "}, + ); + write( + tmp.path(), + "by-arch/amd64.yaml", + "base:\n $include: common/base-amd64.yaml\n", + ); + write(tmp.path(), "common/base-amd64.yaml", "path: ./amd64.img\n"); + let image = load_image(tmp.path().join("image.yaml")).unwrap(); + let mut axes = BTreeMap::new(); + axes.insert("arch".to_owned(), "amd64".to_owned()); + let plan = merge_plan(&image, tmp.path(), &axes).unwrap(); + let step = plan + .iter() + .find(|step| step.label == "by-arch/amd64.yaml") + .expect("by-arch step"); + assert_eq!(step.includes, vec!["common/base-amd64.yaml".to_owned()]); + } + #[test] fn compression_parses_and_is_validated_against_the_format() { // A raw disk-image format accepts compression; the parsed OutputSpec carries it. @@ -677,6 +731,160 @@ mod tests { ); } + #[test] + fn a_fragment_signs_only_its_own_matched_cells() { + // Regression: signing is resolved per cell from the matched fragment chain + // (resolve_signing_ref), not image-wide -- a by- fragment's signing: opt-in must + // apply only to the cells it matches, leaving sibling cells unsigned. + let tmp = TempDir::new().unwrap(); + write( + tmp.path(), + "image.yaml", + indoc! {" + name: xs + matrix: + arch: [amd64, arm64] + base: + path: ./b.img + config: + os: { hostname: xs } + "}, + ); + write( + tmp.path(), + "by-arch/arm64.yaml", + indoc! {" + signing: release + "}, + ); + let image = load_image(tmp.path().join("image.yaml")).unwrap(); + let cells = render_image(&image, tmp.path()).unwrap(); + + let amd64 = cells + .iter() + .find(|c| c.tuple.get("arch") == Some("amd64")) + .unwrap(); + let arm64 = cells + .iter() + .find(|c| c.tuple.get("arch") == Some("arm64")) + .unwrap(); + + assert_eq!( + amd64.signing, None, + "unmatched sibling cell must stay unsigned" + ); + assert_eq!( + arm64.signing, + Some(SigningRef::Profile("release".to_owned())), + "the matched fragment's signing opt-in must apply to its own cell" + ); + } + + #[test] + fn a_more_specific_fragment_can_override_signing_to_false() { + // Regression: resolve_signing_ref must be last-wins across the matched chain (base -> + // most-specific), so a more-specific `signing: false` can override a less-specific + // `signing: true` -- not get silently ignored in favor of the image-wide opt-in. + let tmp = TempDir::new().unwrap(); + write( + tmp.path(), + "image.yaml", + indoc! {" + name: xf + matrix: + arch: [amd64, arm64] + signing: true + base: + path: ./b.img + config: + os: { hostname: xf } + "}, + ); + write( + tmp.path(), + "by-arch/arm64.yaml", + indoc! {" + signing: false + "}, + ); + let image = load_image(tmp.path().join("image.yaml")).unwrap(); + let cells = render_image(&image, tmp.path()).unwrap(); + + let amd64 = cells + .iter() + .find(|c| c.tuple.get("arch") == Some("amd64")) + .unwrap(); + let arm64 = cells + .iter() + .find(|c| c.tuple.get("arch") == Some("arm64")) + .unwrap(); + + assert_eq!( + amd64.signing, + Some(SigningRef::Enabled(true)), + "the base document's image-wide opt-in applies where no fragment overrides it" + ); + assert_eq!( + arm64.signing, + Some(SigningRef::Enabled(false)), + "a more-specific fragment's signing: false must win over the less-specific opt-in" + ); + } + + #[test] + fn extra_dependencies_concatenate_base_then_fragment() { + // Regression: a by-variant/by-scenario fragment's own `extraDependencies:` was silently + // dropped (the schema had no such field), so edits to a fragment-declared local asset never + // busted the incremental fingerprint for that variant. `extraDependencies` must merge across + // matched fragments exactly like `extraParams`/`rpmSources` (base -> most-specific). + let tmp = TempDir::new().unwrap(); + write( + tmp.path(), + "image.yaml", + indoc! {" + name: xd + matrix: + variant: [plain, extra] + extraDependencies: + - ./base-dep.txt + base: + path: ./b.img + config: + os: { hostname: xd } + "}, + ); + write( + tmp.path(), + "by-variant/extra.yaml", + indoc! {" + extraDependencies: + - ./variant-only-dep.txt + "}, + ); + let image = load_image(tmp.path().join("image.yaml")).unwrap(); + let cells = render_image(&image, tmp.path()).unwrap(); + + let plain = cells + .iter() + .find(|c| c.tuple.get("variant") == Some("plain")) + .unwrap(); + let extra = cells + .iter() + .find(|c| c.tuple.get("variant") == Some("extra")) + .unwrap(); + + let deps = |c: &RenderedCell| -> Vec { + c.extra_dependencies + .iter() + .map(|p| p.to_string_lossy().into_owned()) + .collect() + }; + assert_eq!(deps(plain), ["./base-dep.txt"]); + // base -> most-specific: the fragment's own extraDependencies is appended after the base + // document's, not replacing it. + assert_eq!(deps(extra), ["./base-dep.txt", "./variant-only-dep.txt"]); + } + #[test] fn lite_cell_keeps_path_base_appends_packages_and_resolves_include_and_nested_params() { let tmp = TempDir::new().unwrap(); @@ -713,6 +921,70 @@ mod tests { assert_eq!(install(pro), ["boot-pro"]); } + #[test] + fn base_include_splices_a_shared_snippet_before_merge() { + // A by-arch fragment can $include a shared base snippet instead of inlining it, exactly + // like config: already could - regression test for the "must be resolved before merge" + // bug where base: never ran $include resolution. + let tmp = TempDir::new().unwrap(); + write( + tmp.path(), + "image.yaml", + indoc! {" + name: shared-base + matrix: + arch: [amd64, arm64] + outputs: + - format: cosi + config: {} + "}, + ); + write( + tmp.path(), + "by-arch/amd64.yaml", + "base: + $include: common/base-amd64.yaml +", + ); + write( + tmp.path(), + "by-arch/arm64.yaml", + "base: + $include: common/base-arm64.yaml +", + ); + write( + tmp.path(), + "common/base-amd64.yaml", + "path: ./amd64.img +", + ); + write( + tmp.path(), + "common/base-arm64.yaml", + "path: ./arm64.img +", + ); + + let image = load_image(tmp.path().join("image.yaml")).unwrap(); + let cells = render_image(&image, tmp.path()).unwrap(); + let by_arch = |arch: &str| -> &RenderedCell { + cells + .iter() + .find(|c| c.tuple.get("arch") == Some(arch)) + .unwrap_or_else(|| panic!("no {arch} cell")) + }; + + match &by_arch("amd64").base { + BaseSource::Path { path, .. } => assert_eq!(path, Path::new("./amd64.img")), + other => panic!("expected a path base, got {other:?}"), + } + match &by_arch("arm64").base { + BaseSource::Path { path, .. } => assert_eq!(path, Path::new("./arm64.img")), + other => panic!("expected a path base, got {other:?}"), + } + } + #[test] fn an_image_with_no_base_is_an_error() { let tmp = TempDir::new().unwrap(); diff --git a/tools/tailor/crates/tailor-core/src/domain.rs b/tools/tailor/crates/tailor-core/src/domain.rs index f80a015432..271ebd439b 100644 --- a/tools/tailor/crates/tailor-core/src/domain.rs +++ b/tools/tailor/crates/tailor-core/src/domain.rs @@ -60,6 +60,12 @@ pub struct Cell { pub base_image: Option, /// Local RPM sources passed to IC as `--rpm-source`. pub rpm_sources: Vec, + /// The complete, merged `extraDependencies:` dependency list for this cell -- `image.yaml` + /// is itself parsed as the first matched fragment, so this already includes the image-level + /// paths concatenated with every matched fragment's own (base -> most-specific); it's the sole + /// input hashed into the fingerprint, so an edit to any of those declared assets invalidates + /// this cell. + pub extra_dependencies: Vec, /// Extra IC command-line flags appended verbatim after every tailor-managed flag. pub extra_params: Vec, /// The resolved per-cell `signing:` opt-in after fragment merging. diff --git a/tools/tailor/crates/tailor-core/src/orchestrator.rs b/tools/tailor/crates/tailor-core/src/orchestrator.rs index fcc1a31cbf..be7a3ec1e6 100644 --- a/tools/tailor/crates/tailor-core/src/orchestrator.rs +++ b/tools/tailor/crates/tailor-core/src/orchestrator.rs @@ -144,9 +144,13 @@ impl Orchestrator { let resolved_tools_dir = resolved_tools_dir(tools_dir_sources, &cell)?; // Hash the cell's declared local dependencies so an edit to an IC-config-referenced // asset (an `additionalFiles` script, a local RPM) invalidates the incremental - // fingerprint — the config text only *names* these files (`deps.rs`). + // fingerprint — the config text only *names* these files (`deps.rs`). `image.yaml` + // is itself parsed as the first matched fragment, so `cell.extra_dependencies` + // already is the full base + matched-fragment merge (base -> most-specific) -- no + // separate image-level set needs to be chained in here, that would just hash the + // base document's own paths a second time. let extra_dependency_hashes = deps::hash_dependencies( - &cell.target.definition.extra_dependencies, + &cell.extra_dependencies, &cell.target.dir, None, hash_cache_dir, @@ -555,6 +559,7 @@ pub fn cells(target: &Arc) -> Result, CoreError> { base: base.clone(), base_image: base_image.clone(), rpm_sources: rc.rpm_sources.clone(), + extra_dependencies: rc.extra_dependencies.clone(), extra_params: rc.extra_params.clone(), signing: rc.signing.clone(), input_deps: Vec::new(), diff --git a/tools/tailor/crates/tailor-core/src/selector.rs b/tools/tailor/crates/tailor-core/src/selector.rs index 469a6c123a..551e10176c 100644 --- a/tools/tailor/crates/tailor-core/src/selector.rs +++ b/tools/tailor/crates/tailor-core/src/selector.rs @@ -169,6 +169,7 @@ mod tests { }, base_image: None, rpm_sources: vec![], + extra_dependencies: vec![], extra_params: vec![], input_deps: vec![], tools_dir: None, diff --git a/tools/tailor/crates/tailor-exec/src/arg_builder.rs b/tools/tailor/crates/tailor-exec/src/arg_builder.rs index 8840c8b284..3c6c58a595 100644 --- a/tools/tailor/crates/tailor-exec/src/arg_builder.rs +++ b/tools/tailor/crates/tailor-exec/src/arg_builder.rs @@ -1291,6 +1291,7 @@ mod tests { base, base_image: None, rpm_sources: vec![PathBuf::from("/rpms/one")], + extra_dependencies: Vec::new(), extra_params: Vec::new(), input_deps: Vec::new(), tools_dir: None, diff --git a/tools/tailor/crates/tailor-exec/src/executor.rs b/tools/tailor/crates/tailor-exec/src/executor.rs index 7855a9ef0d..66ee73bb14 100644 --- a/tools/tailor/crates/tailor-exec/src/executor.rs +++ b/tools/tailor/crates/tailor-exec/src/executor.rs @@ -799,6 +799,7 @@ mod tests { }, base_image: None, rpm_sources: Vec::new(), + extra_dependencies: Vec::new(), extra_params: Vec::new(), input_deps: Vec::new(), tools_dir: None, diff --git a/tools/tailor/crates/tailor/src/run.rs b/tools/tailor/crates/tailor/src/run.rs index 32624ee409..fb930d85fe 100644 --- a/tools/tailor/crates/tailor/src/run.rs +++ b/tools/tailor/crates/tailor/src/run.rs @@ -1445,6 +1445,7 @@ fn convert_cell(input: &Path, dir: &Path, arch: Arch, format: OutputFormat, slug }, base_image: None, rpm_sources: Vec::new(), + extra_dependencies: Vec::new(), extra_params: Vec::new(), signing: None, input_deps: Vec::new(), diff --git a/tools/tailor/docs/reference/image-yaml.md b/tools/tailor/docs/reference/image-yaml.md index f15b7c95a7..8b2c4e91a4 100644 --- a/tools/tailor/docs/reference/image-yaml.md +++ b/tools/tailor/docs/reference/image-yaml.md @@ -17,7 +17,7 @@ An image definition lives in an `image.yaml`. The top level belongs to tailor. T | `rpmSources` | path list | no | Each path is a directory of RPMs or a `.repo` file; passed as IC `--rpm-source`. | | `operation` | `customize` or `convert` | no | Default: `customize`. | | `signing` | `true` or profile id | no | Opt in to the signed-image pipeline. `true` ⇒ the workspace `signing.default` profile; a string ⇒ that named profile; omitted ⇒ unsigned. See [Sign an image](../how-to/sign-an-image.md). | -| `extraDependencies` | path list | no | Extra files/directories to hash for incremental checks; use for IC-config-referenced assets. | +| `extraDependencies` | path list | no | Extra files/directories to hash for incremental checks; use for IC-config-referenced assets. Mergeable like `rpmSources`/`extraParams` -- see below. | | `dependsOn` | string list | no | Order-only build dependencies on other workspace images. See [Inter-image dependencies](#inter-image-dependencies). | | `inputs` | list of `{name, image, output?, cell?}` | no | Named producer artifacts embedded in `config:` via `${inputs.}`. See [Embedding an artifact with `inputs:`](#embedding-an-artifact-with-inputs). | | `extraParams` | list of `{param, value?}` | no | Extra Image Customizer command-line flags, appended verbatim after every flag tailor manages. For experimental/non-standard IC builds. See [Extra params](#extra-params). | @@ -81,6 +81,11 @@ so changing a `param` or `value` rebuilds the affected cells. every matched fragment, base → most-specific, so a `by-/.yaml` fragment can add a flag for just its cells. +`extraDependencies` is mergeable the same way: a fragment's own `extraDependencies:` concatenates +with the base document's (and every other matched fragment's), base → most-specific, and resolves +relative to the image directory just like the base document's paths do -- so a `by-/.yaml` +fragment can declare extra local dependencies that only its own cells need hashed. + A flag tailor already emits itself (`--config-file`, `--build-dir`, `--output-image-file`, `--output-image-format`, `--rpm-source`, `--tools-dir`, `--image`, `--image-file`, `--image-cache-dir`, `--cosi-compression-level`, and the `--log-*` flags) is **rejected** — tailor