From f1713aa894cfa206dc3a306c1e0fc0b845f810fb Mon Sep 17 00:00:00 2001 From: Colin Walters Date: Sun, 10 Sep 2023 14:29:46 -0400 Subject: [PATCH] container: Move more metadata into `ExportOpts` This drops two extra arguments we added over time; in a few places before we had e.g. `None, None, None` being passed which just looks awkward. And we also threaded through all 3 in various places. The `ExportOpts` just needs to grow a lifetime argument, but that turned out to not be too bad when I realized we could use the elided lifetime `<'_>` in all methods that use it. --- lib/src/chunking.rs | 4 +-- lib/src/cli.rs | 3 +- lib/src/container/encapsulate.rs | 62 ++++++++++---------------------- lib/src/fixture.rs | 3 +- lib/tests/it/main.rs | 24 +++---------- 5 files changed, 28 insertions(+), 68 deletions(-) diff --git a/lib/src/chunking.rs b/lib/src/chunking.rs index 62d2a0ab..d7af2f16 100644 --- a/lib/src/chunking.rs +++ b/lib/src/chunking.rs @@ -269,7 +269,7 @@ impl Chunking { pub fn from_mapping( repo: &ostree::Repo, rev: &str, - meta: ObjectMetaSized, + meta: &ObjectMetaSized, max_layers: &Option, prior_build_metadata: Option<&oci_spec::image::ImageManifest>, ) -> Result { @@ -287,7 +287,7 @@ impl Chunking { #[allow(clippy::or_fun_call)] pub fn process_mapping( &mut self, - meta: ObjectMetaSized, + meta: &ObjectMetaSized, max_layers: &Option, prior_build_metadata: Option<&oci_spec::image::ImageManifest>, ) -> Result<()> { diff --git a/lib/src/cli.rs b/lib/src/cli.rs index 16c53d60..57a004c8 100644 --- a/lib/src/cli.rs +++ b/lib/src/cli.rs @@ -650,8 +650,7 @@ async fn container_export( skip_compression: compression_fast, // TODO rename this in the struct at the next semver break ..Default::default() }; - let pushed = - crate::container::encapsulate(repo, rev, &config, None, Some(opts), None, imgref).await?; + let pushed = crate::container::encapsulate(repo, rev, &config, Some(opts), imgref).await?; println!("{}", pushed); Ok(()) } diff --git a/lib/src/container/encapsulate.rs b/lib/src/container/encapsulate.rs index 74d749b3..c9de2875 100644 --- a/lib/src/container/encapsulate.rs +++ b/lib/src/container/encapsulate.rs @@ -187,8 +187,6 @@ fn build_oci( tag: Option<&str>, config: &Config, opts: ExportOpts, - prior_build: Option<&oci_image::ImageManifest>, - contentmeta: Option, ) -> Result { if !ocidir_path.exists() { std::fs::create_dir(ocidir_path).context("Creating OCI dir")?; @@ -230,14 +228,16 @@ fn build_oci( let mut manifest = ocidir::new_empty_manifest().build().unwrap(); - let chunking = contentmeta + let chunking = opts + .contentmeta + .as_ref() .map(|meta| { crate::chunking::Chunking::from_mapping( repo, commit, meta, &opts.max_layers, - prior_build, + opts.prior_build, ) }) .transpose()?; @@ -316,14 +316,12 @@ pub(crate) fn parse_oci_path_and_tag(path: &str) -> (&str, Option<&str>) { } /// Helper for `build()` that avoids generics -#[instrument(skip(repo, contentmeta))] +#[instrument(skip(repo, config, opts))] async fn build_impl( repo: &ostree::Repo, ostree_ref: &str, config: &Config, - prior_build: Option<&oci_image::ImageManifest>, - opts: Option, - contentmeta: Option, + opts: Option>, dest: &ImageReference, ) -> Result { let mut opts = opts.unwrap_or_default(); @@ -332,16 +330,8 @@ async fn build_impl( } let digest = if dest.transport == Transport::OciDir { let (path, tag) = parse_oci_path_and_tag(dest.name.as_str()); - let _copied: ImageReference = build_oci( - repo, - ostree_ref, - Path::new(path), - tag, - config, - opts, - prior_build, - contentmeta, - )?; + let _copied: ImageReference = + build_oci(repo, ostree_ref, Path::new(path), tag, config, opts)?; None } else { let tempdir = tempfile::tempdir_in("/var/tmp")?; @@ -350,16 +340,7 @@ async fn build_impl( // Minor TODO: refactor to avoid clone let authfile = opts.authfile.clone(); - let tempoci = build_oci( - repo, - ostree_ref, - Path::new(tempdest), - None, - config, - opts, - prior_build, - contentmeta, - )?; + let tempoci = build_oci(repo, ostree_ref, Path::new(tempdest), None, config, opts)?; let digest = skopeo::copy(&tempoci, dest, authfile.as_deref()).await?; Some(digest) @@ -381,7 +362,7 @@ async fn build_impl( /// Options controlling commit export into OCI #[derive(Clone, Debug, Default)] #[non_exhaustive] -pub struct ExportOpts { +pub struct ExportOpts<'m, 'o> { /// If true, do not perform gzip compression of the tar layers. pub skip_compression: bool, /// A set of commit metadata keys to copy as image labels. @@ -395,9 +376,15 @@ pub struct ExportOpts { // TODO semver-break: remove this /// Use only the standard OCI version label pub no_legacy_version_label: bool, + /// A reference to the metadata for a previous build; used to optimize + /// the packing structure. + pub prior_build: Option<&'m oci_image::ImageManifest>, + /// Metadata mapping between objects and their owning component/package; + /// used to optimize packing. + pub contentmeta: Option<&'o ObjectMetaSized>, } -impl ExportOpts { +impl<'m, 'o> ExportOpts<'m, 'o> { /// Return the gzip compression level to use, as configured by the export options. fn compression(&self) -> Compression { if self.skip_compression { @@ -415,19 +402,8 @@ pub async fn encapsulate>( repo: &ostree::Repo, ostree_ref: S, config: &Config, - prior_build: Option<&oci_image::ImageManifest>, - opts: Option, - contentmeta: Option, + opts: Option>, dest: &ImageReference, ) -> Result { - build_impl( - repo, - ostree_ref.as_ref(), - config, - prior_build, - opts, - contentmeta, - dest, - ) - .await + build_impl(repo, ostree_ref.as_ref(), config, opts, dest).await } diff --git a/lib/src/fixture.rs b/lib/src/fixture.rs index a2035b77..3322d04b 100644 --- a/lib/src/fixture.rs +++ b/lib/src/fixture.rs @@ -679,15 +679,14 @@ impl Fixture { .context("Computing sizes")?; let opts = ExportOpts { max_layers: std::num::NonZeroU32::new(PKGS_V0_LEN as u32), + contentmeta: Some(&contentmeta), ..Default::default() }; let digest = crate::container::encapsulate( self.srcrepo(), self.testref(), &config, - None, Some(opts), - Some(contentmeta), &imgref, ) .await diff --git a/lib/tests/it/main.rs b/lib/tests/it/main.rs index 39c38ffd..275f12de 100644 --- a/lib/tests/it/main.rs +++ b/lib/tests/it/main.rs @@ -457,13 +457,12 @@ async fn impl_test_container_import_export(chunked: bool) -> Result<()> { opts.copy_meta_keys = vec!["buildsys.checksum".to_string()]; opts.copy_meta_opt_keys = vec!["nosuchvalue".to_string()]; opts.max_layers = std::num::NonZeroU32::new(PKGS_V0_LEN as u32); + opts.contentmeta = contentmeta.as_ref(); let digest = ostree_ext::container::encapsulate( fixture.srcrepo(), fixture.testref(), &config, - None, Some(opts), - contentmeta, &srcoci_imgref, ) .await @@ -515,8 +514,6 @@ async fn impl_test_container_import_export(chunked: bool) -> Result<()> { fixture.testref(), &config, None, - None, - None, &ociarchive_dest, ) .await @@ -626,8 +623,6 @@ async fn test_unencapsulate_unbootable() -> Result<()> { fixture.testref(), &config, None, - None, - None, &srcoci_imgref, ) .await @@ -962,8 +957,6 @@ async fn test_container_write_derive() -> Result<()> { ..Default::default() }, None, - None, - None, &ImageReference { transport: Transport::OciDir, name: base_oci_path.to_string(), @@ -1346,17 +1339,10 @@ async fn test_container_import_export_registry() -> Result<()> { cmd: Some(vec!["/bin/bash".to_string()]), ..Default::default() }; - let digest = ostree_ext::container::encapsulate( - fixture.srcrepo(), - testref, - &config, - None, - None, - None, - &src_imgref, - ) - .await - .context("exporting to registry")?; + let digest = + ostree_ext::container::encapsulate(fixture.srcrepo(), testref, &config, None, &src_imgref) + .await + .context("exporting to registry")?; let mut digested_imgref = src_imgref.clone(); digested_imgref.name = format!("{}@{}", src_imgref.name, digest);