From cf2e78325c05b7d7463d70ebd0c8b18a71866cf9 Mon Sep 17 00:00:00 2001 From: J Robert Ray Date: Fri, 28 Jun 2024 00:25:11 -0700 Subject: [PATCH] Fix `spk export` not respecting repository arguments In particular, `spk export --local-repo-only ...` would export packages from the origin repo. This necessitated adding another level of "handle" indirection to deal with the different type flavors of SpfsRepository. Signed-off-by: J Robert Ray --- Cargo.lock | 2 + crates/spk-build/src/archive_test.rs | 13 +- crates/spk-cli/group3/Cargo.toml | 1 + crates/spk-cli/group3/src/cmd_export.rs | 26 ++- crates/spk-cli/group3/src/cmd_export_test.rs | 21 +-- crates/spk-cli/group3/src/cmd_import_test.rs | 12 +- crates/spk-storage/Cargo.toml | 1 + crates/spk-storage/src/lib.rs | 1 + crates/spk-storage/src/storage/archive.rs | 166 ++++++++----------- crates/spk-storage/src/storage/mod.rs | 1 + crates/spk-storage/src/storage/spfs.rs | 25 +++ 11 files changed, 147 insertions(+), 122 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e41d6c801..925aed228 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3909,6 +3909,7 @@ dependencies = [ "spk-build", "spk-cli-common", "spk-schema", + "spk-solve", "spk-storage", "tar", "tokio", @@ -4481,6 +4482,7 @@ dependencies = [ "tracing-subscriber", "ulid", "url", + "variantly", ] [[package]] diff --git a/crates/spk-build/src/archive_test.rs b/crates/spk-build/src/archive_test.rs index eeb45d9ab..d72fec64e 100644 --- a/crates/spk-build/src/archive_test.rs +++ b/crates/spk-build/src/archive_test.rs @@ -6,8 +6,8 @@ use rstest::rstest; use spk_schema::foundation::option_map; use spk_schema::ident_ops::NormalizedTagStrategy; use spk_schema::{recipe, Package}; -use spk_storage::export_package; use spk_storage::fixtures::*; +use spk_storage::{export_package, SpfsRepositoryHandle}; use crate::{BinaryPackageBuilder, BuildSource}; @@ -28,7 +28,16 @@ async fn test_archive_create_parents() { .await .unwrap(); let filename = rt.tmpdir.path().join("deep/nested/path/archive.spk"); - export_package::(&spec.ident().to_any(), filename) + let repo = match &*rt.tmprepo { + spk_solve::RepositoryHandle::SPFS(repo) => SpfsRepositoryHandle::Normalized(repo), + spk_solve::RepositoryHandle::SPFSWithVerbatimTags(repo) => { + SpfsRepositoryHandle::Verbatim(repo) + } + spk_solve::RepositoryHandle::Mem(_) | spk_solve::RepositoryHandle::Runtime(_) => { + panic!("only spfs repositories are supported") + } + }; + export_package::(&[repo], &spec.ident().to_any(), filename) .await .expect("export should create dirs as needed"); } diff --git a/crates/spk-cli/group3/Cargo.toml b/crates/spk-cli/group3/Cargo.toml index b67d2a3f3..58d526e6a 100644 --- a/crates/spk-cli/group3/Cargo.toml +++ b/crates/spk-cli/group3/Cargo.toml @@ -27,4 +27,5 @@ tokio = "1.20" [dev-dependencies] tar = "0.4.3" spk-build = { workspace = true } +spk-solve = { workspace = true } rstest = { workspace = true } diff --git a/crates/spk-cli/group3/src/cmd_export.rs b/crates/spk-cli/group3/src/cmd_export.rs index 18ab9b026..c22b14f57 100644 --- a/crates/spk-cli/group3/src/cmd_export.rs +++ b/crates/spk-cli/group3/src/cmd_export.rs @@ -6,10 +6,11 @@ use std::sync::Arc; use clap::{Args, ValueHint}; use colored::Colorize; -use miette::Result; +use miette::{bail, Result}; use spk_cli_common::{flags, CommandArgs, Run}; use spk_schema::ident_ops::{NormalizedTagStrategy, VerbatimTagStrategy}; use spk_storage as storage; +use storage::SpfsRepositoryHandle; #[cfg(test)] #[path = "./cmd_export_test.rs"] @@ -54,14 +55,26 @@ impl Run for Export { let options = self.options.get_options()?; let names_and_repos = self.repos.get_repos_for_non_destructive_operation().await?; - let repos = names_and_repos + let repo_handles = names_and_repos .into_iter() .map(|(_, r)| Arc::new(r)) .collect::>(); + let repos = repo_handles + .iter() + .map(|repo| match &**repo { + storage::RepositoryHandle::SPFS(repo) => Ok(SpfsRepositoryHandle::Normalized(repo)), + storage::RepositoryHandle::SPFSWithVerbatimTags(repo) => { + Ok(SpfsRepositoryHandle::Verbatim(repo)) + } + storage::RepositoryHandle::Mem(_) | storage::RepositoryHandle::Runtime(_) => { + bail!("Only spfs repositories are supported") + } + }) + .collect::>>()?; let pkg = self .requests - .parse_idents(&options, [self.package.as_str()], &repos) + .parse_idents(&options, [self.package.as_str()], repo_handles.as_slice()) .await? .pop() .unwrap(); @@ -73,12 +86,11 @@ impl Run for Export { let filename = self.filename.clone().unwrap_or_else(|| { std::path::PathBuf::from(format!("{}_{}{build}.spk", pkg.name(), pkg.version())) }); - // TODO: this doesn't take the repos as an argument, but probably - // should. It assumes/uses 'local' and 'origin' repos internally. let res = if self.legacy_spk_version_tags_for_writes { - storage::export_package::(&pkg, &filename).await + storage::export_package::(repos.as_slice(), &pkg, &filename).await } else { - storage::export_package::(&pkg, &filename).await + storage::export_package::(repos.as_slice(), &pkg, &filename) + .await }; if let Err(spk_storage::Error::PackageNotFound(_)) = res { tracing::warn!("Ensure that you are specifying at least a package and"); diff --git a/crates/spk-cli/group3/src/cmd_export_test.rs b/crates/spk-cli/group3/src/cmd_export_test.rs index 7739ed14e..46fe385ba 100644 --- a/crates/spk-cli/group3/src/cmd_export_test.rs +++ b/crates/spk-cli/group3/src/cmd_export_test.rs @@ -3,29 +3,18 @@ // https://github.com/imageworks/spk use rstest::rstest; -use spfs::config::Remote; use spfs::prelude::*; -use spfs::RemoteAddress; use spk_build::{BinaryPackageBuilder, BuildSource}; use spk_schema::foundation::option_map; use spk_schema::ident_ops::NormalizedTagStrategy; use spk_schema::{recipe, Package}; use spk_storage::fixtures::*; +use spk_storage::SpfsRepositoryHandle; #[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 rt = spfs_runtime().await; let spec = recipe!( { @@ -53,7 +42,7 @@ async fn test_export_works_with_missing_builds() { // 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 { + let repo = match &*rt.tmprepo { spk_storage::RepositoryHandle::SPFS(spfs) => { for spec in [ format!("{}", blue_spec.ident().build()), @@ -66,13 +55,15 @@ async fn test_export_works_with_missing_builds() { .unwrap(); spfs.remove_tag_stream(&tag).await.unwrap(); } + SpfsRepositoryHandle::Normalized(spfs) } _ => panic!("only implemented for spfs repos"), - } + }; let filename = rt.tmpdir.path().join("archive.spk"); filename.ensure(); spk_storage::export_package::( + &[repo], red_spec.ident().clone().to_version().to_any(None), &filename, ) diff --git a/crates/spk-cli/group3/src/cmd_import_test.rs b/crates/spk-cli/group3/src/cmd_import_test.rs index abb605c5a..d44a52f95 100644 --- a/crates/spk-cli/group3/src/cmd_import_test.rs +++ b/crates/spk-cli/group3/src/cmd_import_test.rs @@ -9,6 +9,7 @@ use spk_schema::foundation::option_map; use spk_schema::ident_ops::NormalizedTagStrategy; use spk_schema::{recipe, Package}; use spk_storage::fixtures::*; +use spk_storage::SpfsRepositoryHandle; #[rstest] #[tokio::test] @@ -30,7 +31,16 @@ async fn test_archive_io() { let filename = rt.tmpdir.path().join("archive.spk"); filename.ensure(); - spk_storage::export_package::(spec.ident().to_any(), &filename) + let repo = match &*rt.tmprepo { + spk_solve::RepositoryHandle::SPFS(repo) => SpfsRepositoryHandle::Normalized(repo), + spk_solve::RepositoryHandle::SPFSWithVerbatimTags(repo) => { + SpfsRepositoryHandle::Verbatim(repo) + } + spk_solve::RepositoryHandle::Mem(_) | spk_solve::RepositoryHandle::Runtime(_) => { + panic!("only spfs repositories are supported") + } + }; + spk_storage::export_package::(&[repo], spec.ident().to_any(), &filename) .await .expect("failed to export"); let mut actual = Vec::new(); diff --git a/crates/spk-storage/Cargo.toml b/crates/spk-storage/Cargo.toml index cc2073fc9..f8d32d4a2 100644 --- a/crates/spk-storage/Cargo.toml +++ b/crates/spk-storage/Cargo.toml @@ -49,3 +49,4 @@ tracing = { workspace = true } tracing-subscriber = "0.3.17" ulid = { workspace = true } url = "2.2" +variantly = { workspace = true } diff --git a/crates/spk-storage/src/lib.rs b/crates/spk-storage/src/lib.rs index 67056b0c2..0139010d4 100644 --- a/crates/spk-storage/src/lib.rs +++ b/crates/spk-storage/src/lib.rs @@ -20,5 +20,6 @@ pub use storage::{ RepositoryHandle, RuntimeRepository, SpfsRepository, + SpfsRepositoryHandle, Storage, }; diff --git a/crates/spk-storage/src/storage/archive.rs b/crates/spk-storage/src/storage/archive.rs index 538365cb0..ba308e077 100644 --- a/crates/spk-storage/src/storage/archive.rs +++ b/crates/spk-storage/src/storage/archive.rs @@ -5,13 +5,19 @@ use std::convert::TryFrom; use std::path::Path; +use itertools::{Itertools, Position}; use spk_schema::ident_ops::TagPathStrategy; use spk_schema::{AnyIdent, BuildIdent, VersionIdent}; +use variantly::Variantly; use super::{Repository, SpfsRepository}; -use crate::{Error, NameAndRepositoryWithTagStrategy, Result}; +use crate::{Error, NameAndRepositoryWithTagStrategy, Result, SpfsRepositoryHandle}; -pub async fn export_package(pkg: impl AsRef, filename: impl AsRef) -> Result<()> +pub async fn export_package<'a, S>( + source_repos: &[SpfsRepositoryHandle<'a>], + pkg: impl AsRef, + filename: impl AsRef, +) -> Result<()> where S: TagPathStrategy + Send + Sync, { @@ -37,13 +43,6 @@ where }) .unwrap_or_else(|| Ok(()))?; - // Don't require the "origin" repo to exist here. - let (local_repo, remote_repo) = tokio::join!( - super::local_repository(), - super::remote_repository::<_, S>("origin"), - ); - let local_repo = local_repo?; - let tar_repo = spfs::storage::tar::TarRepository::create(&filename) .await .map_err(|source| spfs::Error::FailedToOpenRepository { @@ -65,108 +64,84 @@ where let mut to_transfer = std::collections::BTreeSet::new(); to_transfer.insert(pkg.clone()); if pkg.build().is_none() { - to_transfer.extend( - local_repo - .list_package_builds(pkg.as_version()) - .await? - .into_iter() - .map(|pkg| pkg.into_any()), - ); - if remote_repo.is_err() { - return remote_repo.map(|_| ()); + for repo in source_repos { + to_transfer.extend( + repo.list_package_builds(pkg.as_version()) + .await? + .into_iter() + .map(|pkg| pkg.into_any()), + ); } - to_transfer.extend( - remote_repo - .as_ref() - .unwrap() - .list_package_builds(pkg.as_version()) - .await? - .into_iter() - .map(|pkg| pkg.into_any()), - ); } else { to_transfer.insert(pkg.with_build(None)); } - for transfer_pkg in to_transfer.into_iter() { + 'pkg: 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; } + #[derive(Variantly)] enum CopyResult { VersionNotFound, BuildNotFound, Err(Error), } - impl CopyResult { - fn or(self, other: CopyResult) -> Option { - if let CopyResult::Err(err) = self { - Some(err) - } else if let CopyResult::Err(err) = other { - Some(err) - } else { - None + let mut first_error = None; + let mut all_errors_are_build_not_found = true; + + for (position, repo) in source_repos.iter().with_position() { + let err = match copy_any(transfer_pkg.clone(), repo, &target_repo).await { + Ok(_) => continue 'pkg, + Err(Error::PackageNotFound(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 + // `PackageNotFound` 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. + all_errors_are_build_not_found = all_errors_are_build_not_found + && matches!(err, CopyResult::BuildNotFound) + && pkg.build().is_none(); + + // We'll report the error from the first repo that failed, under the + // assumption that the repo(s) listed first are more likely to be + // where the problem is fixable (e.g., the local repo). + if first_error.is_none() { + first_error = Some(err); } - } - let local_err = match copy_any(transfer_pkg.clone(), &local_repo, &target_repo).await { - Ok(_) => continue, - Err(Error::PackageNotFound(ident)) => { - if ident.build().is_some() { - CopyResult::BuildNotFound - } else { - CopyResult::VersionNotFound + match position { + Position::Last | Position::Only if all_errors_are_build_not_found => { + continue 'pkg; } - } - Err(err) => CopyResult::Err(err), - }; - if remote_repo.is_err() { - return remote_repo.map(|_| ()); - } - let remote_err = match copy_any( - transfer_pkg.clone(), - remote_repo.as_ref().unwrap(), - &target_repo, - ) - .await - { - Ok(_) => continue, - Err(Error::PackageNotFound(ident)) => { - if ident.build().is_some() { - CopyResult::BuildNotFound - } else { - CopyResult::VersionNotFound + Position::Last | Position::Only => { + return Err(first_error + .unwrap() + .err() + .unwrap_or_else(|| Error::PackageNotFound(transfer_pkg))); + } + _ => { + // Try the next repo + continue; } } - 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 - // `PackageNotFound` 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(|| Error::PackageNotFound(transfer_pkg))); } tracing::info!(path=?filename, "building archive"); @@ -177,13 +152,12 @@ where Ok(()) } -async fn copy_any( +async fn copy_any<'a, S2>( pkg: AnyIdent, - src_repo: &SpfsRepository, + src_repo: &SpfsRepositoryHandle<'a>, dst_repo: &SpfsRepository, ) -> Result<()> where - S1: TagPathStrategy + Send + Sync, S2: TagPathStrategy + Send + Sync, { match pkg.into_inner() { @@ -194,13 +168,12 @@ where } } -async fn copy_recipe( +async fn copy_recipe<'a, S2>( pkg: &VersionIdent, - src_repo: &SpfsRepository, + src_repo: &SpfsRepositoryHandle<'a>, dst_repo: &SpfsRepository, ) -> Result<()> where - S1: TagPathStrategy + Send + Sync, S2: TagPathStrategy + Send + Sync, { let spec = src_repo.read_recipe(pkg).await?; @@ -209,19 +182,18 @@ where Ok(()) } -async fn copy_package( +async fn copy_package<'a, S2>( pkg: &BuildIdent, - src_repo: &SpfsRepository, + src_repo: &SpfsRepositoryHandle<'a>, dst_repo: &SpfsRepository, ) -> Result<()> where - S1: TagPathStrategy + Send + Sync, S2: TagPathStrategy + Send + Sync, { let spec = src_repo.read_package(pkg).await?; let components = src_repo.read_components(pkg).await?; tracing::info!(%pkg, "exporting"); - let syncer = spfs::Syncer::new(src_repo, dst_repo) + let syncer = spfs::Syncer::new(src_repo.spfs_repository_handle(), dst_repo) .with_reporter(spfs::sync::ConsoleSyncReporter::default()); let desired = components.iter().map(|i| *i.1).collect(); syncer.sync_env(desired).await?; diff --git a/crates/spk-storage/src/storage/mod.rs b/crates/spk-storage/src/storage/mod.rs index a4a8f05d2..b09f2902f 100644 --- a/crates/spk-storage/src/storage/mod.rs +++ b/crates/spk-storage/src/storage/mod.rs @@ -20,4 +20,5 @@ pub use self::spfs::{ remote_repository, NameAndRepositoryWithTagStrategy, SpfsRepository, + SpfsRepositoryHandle, }; diff --git a/crates/spk-storage/src/storage/spfs.rs b/crates/spk-storage/src/storage/spfs.rs index 95c29eb84..0b5522905 100644 --- a/crates/spk-storage/src/storage/spfs.rs +++ b/crates/spk-storage/src/storage/spfs.rs @@ -1276,6 +1276,31 @@ impl StoredPackage { } } +pub enum SpfsRepositoryHandle<'a> { + Normalized(&'a SpfsRepository), + Verbatim(&'a SpfsRepository), +} + +impl<'a> SpfsRepositoryHandle<'a> { + pub fn spfs_repository_handle(&self) -> &spfs::prelude::RepositoryHandle { + match self { + Self::Normalized(repo) => &repo.inner, + Self::Verbatim(repo) => &repo.inner, + } + } +} + +impl<'a> std::ops::Deref for SpfsRepositoryHandle<'a> { + type Target = dyn crate::storage::Repository; + + fn deref(&self) -> &Self::Target { + match self { + Self::Normalized(repo) => *repo, + Self::Verbatim(repo) => *repo, + } + } +} + /// Return the local packages repository used for development. pub async fn local_repository() -> Result> { let config = spfs::get_config()?;