Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove all old format (tar/container) support #332

Open
cgwalters opened this issue Jun 28, 2022 · 4 comments
Open

remove all old format (tar/container) support #332

cgwalters opened this issue Jun 28, 2022 · 4 comments
Labels
semver-break A change that requires a semver bump
Milestone

Comments

@cgwalters
Copy link
Member

Let's do this alongside the next semver break in 0.9.

@cgwalters cgwalters added this to the 0.9.0 milestone Jun 28, 2022
@cgwalters cgwalters added the semver-break A change that requires a semver bump label Jun 28, 2022
@cgwalters
Copy link
Member Author

OK and when we do this, we can convert the main tar export path to be identical to a single-layer chunked container image. This will drop a lot of duplicate code in the tar export path.
(The cost though is we'll need to build up a mapping of the content objects in checksum order instead of directory order in that case too, but eh)

@cgwalters
Copy link
Member Author

Started on this

commit 7fa1c40f5c2b65071dec73917bd5c8be0ac5b9a4
Author:     Colin Walters <walters@verbum.org>
AuthorDate: Tue Jun 28 13:36:33 2022 -0400
Commit:     Colin Walters <walters@verbum.org>
CommitDate: Tue Jun 28 13:36:33 2022 -0400

    tar: Always export metadata first, then content objects

diff --git a/lib/src/tar/export.rs b/lib/src/tar/export.rs
index f7180d9..bcb1205 100644
--- a/lib/src/tar/export.rs
+++ b/lib/src/tar/export.rs
@@ -70,8 +70,6 @@ struct OstreeTarWriter<'a, W: std::io::Write> {
     out: &'a mut tar::Builder<W>,
     options: ExportOptions,
     wrote_initdirs: bool,
-    /// True if we're only writing directories
-    structure_only: bool,
     wrote_dirtree: HashSet<String>,
     wrote_dirmeta: HashSet<String>,
     wrote_content: HashSet<String>,
@@ -155,7 +153,6 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             out,
             options,
             wrote_initdirs: false,
-            structure_only: false,
             wrote_dirmeta: HashSet::new(),
             wrote_dirtree: HashSet::new(),
             wrote_content: HashSet::new(),
@@ -266,7 +263,7 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
     }
 
     /// Recursively serialize a commit object to the target tar stream.
-    fn write_commit(&mut self) -> Result<()> {
+    fn write_metadata(&mut self) -> Result<()> {
         let cancellable = gio::NONE_CANCELLABLE;
 
         let commit_bytes = self.commit_object.data_as_bytes();
@@ -299,11 +296,30 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             Utf8Path::new(TAR_PATH_PREFIX_V0),
             contents,
             true,
+            true,
             cancellable,
         )?;
         Ok(())
     }
 
+    fn write_contents(&mut self) -> Result<()> {
+        let cancellable = gio::NONE_CANCELLABLE;
+        let commit_bytes = self.commit_object.data_as_bytes();
+        let commit_bytes = commit_bytes.try_as_aligned()?;
+        let commit = gv_commit!().cast(commit_bytes);
+        let commit = commit.to_tuple();
+        let contents = hex::encode(commit.6);
+
+        // Recurse and write everything else.
+        self.append_dirtree(
+            Utf8Path::new(TAR_PATH_PREFIX_V0),
+            contents,
+            true,
+            false,
+            cancellable,
+        )
+    }
+
     fn append_commit_object(&mut self) -> Result<()> {
         self.append(
             ostree::ObjectType::Commit,
@@ -493,6 +509,7 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
         dirpath: &Utf8Path,
         checksum: String,
         is_root: bool,
+        metadata_only: bool,
         cancellable: Option<&C>,
     ) -> Result<()> {
         let v = &self
@@ -509,7 +526,7 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             c.set_error_if_cancelled()?;
         }
 
-        if !self.structure_only {
+        if !metadata_only {
             for file in files {
                 let (name, csum) = file.to_tuple();
                 let name = name.to_str();
@@ -529,7 +546,9 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
                 let meta_v = &self
                     .repo
                     .load_variant(ostree::ObjectType::DirMeta, meta_csum)?;
-                self.append(ostree::ObjectType::DirMeta, meta_csum, meta_v)?;
+                if metadata_only {
+                    self.append(ostree::ObjectType::DirMeta, meta_csum, meta_v)?;
+                }
                 // Safety: We passed the correct variant type just above
                 ostree::DirMetaParsed::from_variant(meta_v).unwrap()
             };
@@ -540,8 +559,10 @@ impl<'a, W: std::io::Write> OstreeTarWriter<'a, W> {
             let dirtree_csum = hex::encode(contents_csum);
             let subpath = &dirpath.join(name);
             let subpath = map_path(subpath);
-            self.append_dir(&*subpath, &metadata)?;
-            self.append_dirtree(&*subpath, dirtree_csum, false, cancellable)?;
+            if metadata_only {
+                self.append_dir(&*subpath, &metadata)?;
+            }
+            self.append_dirtree(&*subpath, dirtree_csum, false, metadata_only, cancellable)?;
         }
 
         Ok(())
@@ -559,7 +580,8 @@ fn impl_export<W: std::io::Write>(
     options: ExportOptions,
 ) -> Result<()> {
     let writer = &mut OstreeTarWriter::new(repo, commit_checksum, out, options)?;
-    writer.write_commit()?;
+    writer.write_metadata()?;
+    writer.write_contents()?;
     Ok(())
 }
 
@@ -636,11 +658,7 @@ pub(crate) fn export_final_chunk<W: std::io::Write>(
         ..Default::default()
     };
     let writer = &mut OstreeTarWriter::new(repo, commit_checksum, out, options)?;
-    // For the final chunk, output the commit object, plus all ostree metadata objects along with
-    // the containing directories.
-    writer.structure_only = true;
-    writer.write_commit()?;
-    writer.structure_only = false;
+    writer.write_metadata()?;
     write_chunk(writer, remainder.content)
 }
 

@cgwalters
Copy link
Member Author

OK fully pulling off the bandaid here would be a big move. Let's try to be a bit more incremental; did that in #378

@cgwalters
Copy link
Member Author

If everything goes well after shipping 0.9, let's do this in 0.10.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-break A change that requires a semver bump
Projects
None yet
Development

No branches or pull requests

1 participant