Merge pull request #800 from imageworks/export-ignore-missing-builds

Fix spk export failing if builds are missing

crates/spk-cli/group3/src/cmd_export.rs

@@ -10,6 +10,10 @@ use colored::Colorize;
 use spk_cli_common::{flags, CommandArgs, Run};
 use spk_storage::{self as storage};
 
+#[cfg(test)]
+#[path = "./cmd_export_test.rs"]
+mod cmd_export_test;
+
 /// Export a package as a tar file
 #[derive(Args)]
 pub struct Export {

crates/spk-cli/group3/src/cmd_export_test.rs

@@ -0,0 +1,124 @@
+use rstest::rstest;
+use spfs::config::Remote;
+use spfs::RemoteAddress;
+use spk_build::{BinaryPackageBuilder, BuildSource};
+use spk_schema::foundation::option_map;
+use spk_schema::{recipe, Package};
+use spk_storage::fixtures::*;
+
+#[rstest]
+#[tokio::test]
+async fn test_export_works_with_missing_builds() {
+    let mut rt = spfs_runtime().await;
+
+    // exporting requires a configured "origin" repo.
+    let remote_repo = spfsrepo().await;
+    rt.add_remote_repo(
+        "origin",
+        Remote::Address(RemoteAddress {
+            address: remote_repo.address().clone(),
+        }),
+    )
+    .unwrap();
+
+    let spec = recipe!(
+        {
+            "pkg": "spk-export-test/0.0.1",
+            "build": {
+                "options": [
+                    {"var": "color"},
+                ],
+                "script": "touch /spfs/file.txt",
+            },
+        }
+    );
+    rt.tmprepo.publish_recipe(&spec).await.unwrap();
+    let (blue_spec, _) = BinaryPackageBuilder::from_recipe(spec.clone())
+        .with_source(BuildSource::LocalPath(".".into()))
+        .build_and_publish(option_map! {"color" => "blue"}, &*rt.tmprepo)
+        .await
+        .unwrap();
+    let (red_spec, _) = BinaryPackageBuilder::from_recipe(spec)
+        .with_source(BuildSource::LocalPath(".".into()))
+        .build_and_publish(option_map! {"color" => "red"}, &*rt.tmprepo)
+        .await
+        .unwrap();
+
+    // Now that these two builds are created, remove the `spk/pkg` tags for one
+    // of them. The publish is still expected to succeed; it should publish
+    // the remaining valid build.
+    match &*rt.tmprepo {
+        spk_storage::RepositoryHandle::SPFS(spfs) => {
+            for spec in [
+                format!("{}", blue_spec.ident().build()),
+                format!("{}/build", blue_spec.ident().build()),
+                format!("{}/run", blue_spec.ident().build()),
+            ] {
+                let tag = spfs::tracking::TagSpec::parse(format!(
+                    "spk/pkg/spk-export-test/0.0.1/{spec}",
+                ))
+                .unwrap();
+                spfs.remove_tag_stream(&tag).await.unwrap();
+            }
+        }
+        _ => panic!("only implemented for spfs repos"),
+    }
+
+    let filename = rt.tmpdir.path().join("archive.spk");
+    filename.ensure();
+    spk_storage::export_package(
+        red_spec.ident().clone().to_version().to_any(None),
+        &filename,
+    )
+    .await
+    .expect("failed to export");
+    let mut actual = Vec::new();
+    let mut tarfile = tar::Archive::new(std::fs::File::open(&filename).unwrap());
+    for entry in tarfile.entries().unwrap() {
+        let filename = entry.unwrap().path().unwrap().to_string_lossy().to_string();
+        if filename.contains('/') && !filename.contains("tags") {
+            // ignore specific object data for this test
+            continue;
+        }
+        actual.push(filename);
+    }
+    actual.sort();
+    assert_eq!(
+        actual,
+        vec![
+            "VERSION".to_string(),
+            "objects".to_string(),
+            "payloads".to_string(),
+            "renders".to_string(),
+            "tags".to_string(),
+            "tags/spk".to_string(),
+            "tags/spk/pkg".to_string(),
+            "tags/spk/pkg/spk-export-test".to_string(),
+            "tags/spk/pkg/spk-export-test/0.0.1".to_string(),
+            format!(
+                "tags/spk/pkg/spk-export-test/0.0.1/{}",
+                red_spec.ident().build()
+            ),
+            format!(
+                "tags/spk/pkg/spk-export-test/0.0.1/{}.tag",
+                red_spec.ident().build()
+            ),
+            format!(
+                "tags/spk/pkg/spk-export-test/0.0.1/{}/build.tag",
+                red_spec.ident().build()
+            ),
+            format!(
+                "tags/spk/pkg/spk-export-test/0.0.1/{}/run.tag",
+                red_spec.ident().build()
+            ),
+            "tags/spk/spec".to_string(),
+            "tags/spk/spec/spk-export-test".to_string(),
+            "tags/spk/spec/spk-export-test/0.0.1".to_string(),
+            "tags/spk/spec/spk-export-test/0.0.1.tag".to_string(),
+            format!(
+                "tags/spk/spec/spk-export-test/0.0.1/{}.tag",
+                red_spec.ident().build()
+            ),
+        ]
+    );
+}

crates/spk-storage/src/storage/archive.rs

@@ -77,37 +77,89 @@ where
         to_transfer.insert(pkg.with_build(None));
     }
 
-    for pkg in to_transfer.into_iter() {
-        if pkg.is_embedded() {
+    for transfer_pkg in to_transfer.into_iter() {
+        if transfer_pkg.is_embedded() {
             // Don't attempt to export an embedded package; the stub
             // will be recreated if exporting its provider.
             continue;
         }
 
-        let local_err = match copy_any(pkg.clone(), &local_repo, &target_repo).await {
+        enum CopyResult {
+            VersionNotFound,
+            BuildNotFound,
+            Err(Error),
+        }
+
+        impl CopyResult {
+            fn or(self, other: CopyResult) -> Option<Error> {
+                if let CopyResult::Err(err) = self {
+                    Some(err)
+                } else if let CopyResult::Err(err) = other {
+                    Some(err)
+                } else {
+                    None
+                }
+            }
+        }
+
+        let local_err = match copy_any(transfer_pkg.clone(), &local_repo, &target_repo).await {
             Ok(_) => continue,
             Err(Error::SpkValidatorsError(
-                spk_schema::validators::Error::PackageNotFoundError(_),
-            )) => None,
-            Err(err) => Some(err),
+                spk_schema::validators::Error::PackageNotFoundError(ident),
+            )) => {
+                if ident.build().is_some() {
+                    CopyResult::BuildNotFound
+                } else {
+                    CopyResult::VersionNotFound
+                }
+            }
+            Err(err) => CopyResult::Err(err),
         };
         if remote_repo.is_err() {
             return remote_repo.map(|_| ());
         }
-        let remote_err =
-            match copy_any(pkg.clone(), remote_repo.as_ref().unwrap(), &target_repo).await {
-                Ok(_) => continue,
-                Err(Error::SpkValidatorsError(
-                    spk_schema::validators::Error::PackageNotFoundError(_),
-                )) => None,
-                Err(err) => Some(err),
-            };
+        let remote_err = match copy_any(
+            transfer_pkg.clone(),
+            remote_repo.as_ref().unwrap(),
+            &target_repo,
+        )
+        .await
+        {
+            Ok(_) => continue,
+            Err(Error::SpkValidatorsError(
+                spk_schema::validators::Error::PackageNotFoundError(ident),
+            )) => {
+                if ident.build().is_some() {
+                    CopyResult::BuildNotFound
+                } else {
+                    CopyResult::VersionNotFound
+                }
+            }
+            Err(err) => CopyResult::Err(err),
+        };
+
+        // `list_package_builds` can return builds that only exist as spfs tags
+        // under `spk/spec`, meaning the build doesn't really exist. Ignore
+        // `PackageNotFoundError` about these ... unless the build was
+        // explicitly named to be archived.
+        //
+        // Consider changing `list_package_builds` so it doesn't do that
+        // anymore, although it has a comment that it is doing so
+        // intentionally. Maybe it should return a richer type that describes
+        // if only the "spec build" exists and that info could be used here.
+        if matches!(local_err, CopyResult::BuildNotFound)
+            && matches!(remote_err, CopyResult::BuildNotFound)
+            && pkg.build().is_none()
+        {
+            continue;
+        }
+
         // we will hide the remote_err in cases when both failed,
         // because the remote was always a fallback and fixing the
         // local error is preferred
-        return Err(local_err
-            .or(remote_err)
-            .unwrap_or_else(|| spk_schema::validators::Error::PackageNotFoundError(pkg).into()));
+        return Err(local_err.or(remote_err).unwrap_or_else(|| {
+            spk_schema::validators::Error::PackageNotFoundError(transfer_pkg).into()
+        }));
     }
 
     tracing::info!(path=?filename, "building archive");