From 53bdf0ced6731efa9edc3363bcc06d58110bdaf9 Mon Sep 17 00:00:00 2001 From: daimond113 <72147841+daimond113@users.noreply.github.com> Date: Fri, 17 Jan 2025 21:15:12 +0100 Subject: [PATCH] fix: do gix operations inside spawn_blocking Additionally refactors the source's code to be much neater and easier to read. --- src/source/git/mod.rs | 524 +++++++++++++++++++++--------------------- 1 file changed, 261 insertions(+), 263 deletions(-) diff --git a/src/source/git/mod.rs b/src/source/git/mod.rs index dece858..7ef8622 100644 --- a/src/source/git/mod.rs +++ b/src/source/git/mod.rs @@ -1,6 +1,6 @@ use crate::{ deser_manifest, - manifest::{target::Target, Manifest}, + manifest::{target::Target, Alias, DependencyType, Manifest}, names::PackageNames, reporters::DownloadProgressReporter, source::{ @@ -56,6 +56,113 @@ impl GitPackageSource { } } +fn transform_pesde_dependencies( + manifest: &Manifest, + repo_url: Url, + rev: &str, + root_tree: &gix::Tree, +) -> Result, errors::ResolveError> { + let dependencies = manifest + .all_dependencies() + .map_err(|e| errors::ResolveError::CollectDependencies(Box::new(repo_url.clone()), e))?; + + dependencies + .into_iter() + .map(|(alias, (mut spec, ty))| { + match &mut spec { + DependencySpecifiers::Pesde(specifier) => { + let index_name = specifier + .index + .as_deref() + .unwrap_or(DEFAULT_INDEX_NAME) + .to_string(); + specifier.index = Some( + manifest + .indices + .get(&index_name) + .ok_or_else(|| { + errors::ResolveError::PesdeIndexNotFound( + index_name.clone(), + Box::new(repo_url.clone()), + ) + })? + .to_string(), + ); + } + #[cfg(feature = "wally-compat")] + DependencySpecifiers::Wally(specifier) => { + let index_name = specifier + .index + .as_deref() + .unwrap_or(DEFAULT_INDEX_NAME) + .to_string(); + specifier.index = Some( + manifest + .wally_indices + .get(&index_name) + .ok_or_else(|| { + errors::ResolveError::WallyIndexNotFound( + index_name.clone(), + Box::new(repo_url.clone()), + ) + })? + .to_string(), + ); + } + DependencySpecifiers::Git(_) => {} + DependencySpecifiers::Workspace(specifier) => { + let lockfile = read_file(root_tree, [LOCKFILE_FILE_NAME]).map_err(|e| { + errors::ResolveError::ReadLockfile(Box::new(repo_url.clone()), e) + })?; + + let lockfile = match lockfile { + Some(l) => match toml::from_str::(&l) { + Ok(l) => l, + Err(e) => { + return Err(errors::ResolveError::DeserLockfile( + Box::new(repo_url.clone()), + e, + )) + } + }, + None => { + return Err(errors::ResolveError::NoLockfile(Box::new( + repo_url.clone(), + ))) + } + }; + + let target = specifier.target.unwrap_or(manifest.target.kind()); + + let path = lockfile + .workspace + .get(&specifier.name) + .and_then(|targets| targets.get(&target)) + .ok_or_else(|| { + errors::ResolveError::NoPathForWorkspaceMember( + specifier.name.to_string(), + target, + Box::new(repo_url.clone()), + ) + })? + .clone(); + + spec = DependencySpecifiers::Git(GitDependencySpecifier { + repo: repo_url.clone(), + rev: rev.to_string(), + path: Some(path), + }) + } + DependencySpecifiers::Path(_) => { + return Err(errors::ResolveError::Path(Box::new(repo_url.clone()))) + } + } + + Ok((alias, (spec, ty))) + }) + .collect() +} + impl PackageSource for GitPackageSource { type Specifier = GitDependencySpecifier; type Ref = GitPackageRef; @@ -77,191 +184,80 @@ impl PackageSource for GitPackageSource { ) -> Result, Self::ResolveError> { let ResolveOptions { project, .. } = options; - let repo = gix::open(self.path(project)) - .map_err(|e| errors::ResolveError::OpenRepo(Box::new(self.repo_url.clone()), e))?; - let rev = repo - .rev_parse_single(BStr::new(&specifier.rev)) - .map_err(|e| { - errors::ResolveError::ParseRev( - specifier.rev.clone(), - Box::new(self.repo_url.clone()), - e, - ) + let path = self.path(project); + let repo_url = self.repo_url.clone(); + let specifier = specifier.clone(); + + let (name, version_id, dependencies, tree_id) = spawn_blocking(move || { + let repo = gix::open(path).map_err(|e| { + errors::ResolveError::OpenRepo(Box::new(repo_url.clone()), Box::new(e)) })?; - - // TODO: possibly use the search algorithm from src/main.rs to find the workspace root - - let root_tree = rev - .object() - .map_err(|e| { - errors::ResolveError::ParseRevToObject(Box::new(self.repo_url.clone()), e) - })? - .peel_to_tree() - .map_err(|e| { - errors::ResolveError::ParseObjectToTree(Box::new(self.repo_url.clone()), e) - })?; - - let tree = if let Some(path) = &specifier.path { - root_tree - .lookup_entry_by_path(path.as_str()) + let rev = repo + .rev_parse_single(BStr::new(&specifier.rev)) .map_err(|e| { - errors::ResolveError::ReadTreeEntry( - Box::new(self.repo_url.clone()), - path.clone(), - e, + errors::ResolveError::ParseRev( + specifier.rev.clone(), + Box::new(repo_url.clone()), + Box::new(e), ) - })? - .ok_or_else(|| { - errors::ResolveError::NoEntryAtPath( - Box::new(self.repo_url.clone()), - path.clone(), - ) - })? + })?; + + // TODO: possibly use the search algorithm from src/main.rs to find the workspace root + + let root_tree = rev .object() - .map_err(|e| { - errors::ResolveError::ParseEntryToObject(Box::new(self.repo_url.clone()), e) - })? + .map_err(|e| errors::ResolveError::ParseRevToObject(Box::new(repo_url.clone()), e))? .peel_to_tree() .map_err(|e| { - errors::ResolveError::ParseObjectToTree(Box::new(self.repo_url.clone()), e) - })? - } else { - root_tree.clone() - }; + errors::ResolveError::ParseObjectToTree(Box::new(repo_url.clone()), e) + })?; - let manifest = match read_file(&tree, [MANIFEST_FILE_NAME]) - .map_err(|e| errors::ResolveError::ReadManifest(Box::new(self.repo_url.clone()), e))? - { - Some(m) => match toml::from_str::(&m) { - Ok(m) => Some(m), - Err(e) => { - return Err(errors::ResolveError::DeserManifest( - Box::new(self.repo_url.clone()), - e, - )) - } - }, - None => None, - }; - - let (name, version_id, dependencies) = match manifest { - Some(manifest) => { - let dependencies = manifest - .all_dependencies() + let tree = if let Some(path) = &specifier.path { + root_tree + .lookup_entry_by_path(path.as_str()) .map_err(|e| { - errors::ResolveError::CollectDependencies( - Box::new(self.repo_url.clone()), + errors::ResolveError::ReadTreeEntry( + Box::new(repo_url.clone()), + path.clone(), e, ) })? - .into_iter() - .map(|(alias, (mut spec, ty))| { - match &mut spec { - DependencySpecifiers::Pesde(specifier) => { - let index_name = specifier - .index - .as_deref() - .unwrap_or(DEFAULT_INDEX_NAME) - .to_string(); - specifier.index = Some( - manifest - .indices - .get(&index_name) - .ok_or_else(|| { - errors::ResolveError::PesdeIndexNotFound( - index_name.clone(), - Box::new(self.repo_url.clone()), - ) - })? - .to_string(), - ); - } - #[cfg(feature = "wally-compat")] - DependencySpecifiers::Wally(specifier) => { - let index_name = specifier - .index - .as_deref() - .unwrap_or(DEFAULT_INDEX_NAME) - .to_string(); - specifier.index = Some( - manifest - .wally_indices - .get(&index_name) - .ok_or_else(|| { - errors::ResolveError::WallyIndexNotFound( - index_name.clone(), - Box::new(self.repo_url.clone()), - ) - })? - .to_string(), - ); - } - DependencySpecifiers::Git(_) => {} - DependencySpecifiers::Workspace(specifier) => { - let lockfile = read_file(&root_tree, [LOCKFILE_FILE_NAME]) - .map_err(|e| { - errors::ResolveError::ReadLockfile( - Box::new(self.repo_url.clone()), - e, - ) - })?; + .ok_or_else(|| { + errors::ResolveError::NoEntryAtPath( + Box::new(repo_url.clone()), + path.clone(), + ) + })? + .object() + .map_err(|e| { + errors::ResolveError::ParseEntryToObject(Box::new(repo_url.clone()), e) + })? + .peel_to_tree() + .map_err(|e| { + errors::ResolveError::ParseObjectToTree(Box::new(repo_url.clone()), e) + })? + } else { + root_tree.clone() + }; - let lockfile = match lockfile { - Some(l) => match toml::from_str::(&l) { - Ok(l) => l, - Err(e) => { - return Err(errors::ResolveError::DeserLockfile( - Box::new(self.repo_url.clone()), - e, - )) - } - }, - None => { - return Err(errors::ResolveError::NoLockfile(Box::new( - self.repo_url.clone(), - ))) - } - }; - - let target = specifier.target.unwrap_or(manifest.target.kind()); - - let path = lockfile - .workspace - .get(&specifier.name) - .and_then(|targets| targets.get(&target)) - .ok_or_else(|| { - errors::ResolveError::NoPathForWorkspaceMember( - specifier.name.to_string(), - target, - Box::new(self.repo_url.clone()), - ) - })? - .clone(); - - spec = DependencySpecifiers::Git(GitDependencySpecifier { - repo: self.repo_url.clone(), - rev: rev.to_string(), - path: Some(path), - }) - } - DependencySpecifiers::Path(_) => { - return Err(errors::ResolveError::Path(Box::new( - self.repo_url.clone(), - ))) - } - } - - Ok((alias, (spec, ty))) - }) - .collect::>()?; - let name = PackageNames::Pesde(manifest.name); - let version_id = VersionId(manifest.version, manifest.target.kind()); - - (name, version_id, dependencies) - } + let manifest = match read_file(&tree, [MANIFEST_FILE_NAME]) + .map_err(|e| errors::ResolveError::ReadManifest(Box::new(repo_url.clone()), e))? + { + Some(m) => match toml::from_str::(&m) { + Ok(m) => Some(m), + Err(e) => { + return Err(errors::ResolveError::DeserManifest( + Box::new(repo_url.clone()), + e, + )) + } + }, + None => None, + }; #[cfg(feature = "wally-compat")] - None => { + let Some(manifest) = manifest + else { use crate::{ manifest::target::TargetKind, source::wally::{ @@ -270,49 +266,62 @@ impl PackageSource for GitPackageSource { }, }; - match read_file(&tree, [WALLY_MANIFEST_FILE_NAME]).map_err(|e| { - errors::ResolveError::ReadManifest(Box::new(self.repo_url.clone()), e) - })? { - Some(m) => match toml::from_str::(&m) { - Ok(manifest) => { - let dependencies = manifest.all_dependencies().map_err(|e| { - errors::ResolveError::CollectDependencies( - Box::new(self.repo_url.clone()), - e, - ) - })?; - let name = PackageNames::Wally(manifest.package.name); - let version_id = VersionId( - manifest.package.version, - match manifest.package.realm { - Realm::Server => TargetKind::RobloxServer, - _ => TargetKind::Roblox, - }, - ); + let manifest = read_file(&tree, [WALLY_MANIFEST_FILE_NAME]).map_err(|e| { + errors::ResolveError::ReadManifest(Box::new(repo_url.clone()), e) + })?; - (name, version_id, dependencies) - } - Err(e) => { - return Err(errors::ResolveError::DeserManifest( - Box::new(self.repo_url.clone()), - e, - )) - } - }, - None => { - return Err(errors::ResolveError::NoManifest(Box::new( - self.repo_url.clone(), - ))) + let Some(manifest) = manifest else { + return Err(errors::ResolveError::NoManifest(Box::new(repo_url.clone()))); + }; + + let manifest = match toml::from_str::(&manifest) { + Ok(manifest) => manifest, + Err(e) => { + return Err(errors::ResolveError::DeserManifest( + Box::new(repo_url.clone()), + e, + )) } - } - } + }; + let dependencies = manifest.all_dependencies().map_err(|e| { + errors::ResolveError::CollectDependencies(Box::new(repo_url.clone()), e) + })?; + + return Ok(( + PackageNames::Wally(manifest.package.name), + VersionId( + manifest.package.version, + match manifest.package.realm { + Realm::Shared => TargetKind::Roblox, + Realm::Server => TargetKind::RobloxServer, + }, + ), + dependencies, + tree.id.to_string(), + )); + }; #[cfg(not(feature = "wally-compat"))] - None => { - return Err(errors::ResolveError::NoManifest(Box::new( - self.repo_url.clone(), - ))) - } - }; + let Some(manifest) = manifest + else { + return Err(errors::ResolveError::NoManifest(Box::new(repo_url.clone()))); + }; + + let dependencies = transform_pesde_dependencies( + &manifest, + repo_url.clone(), + &specifier.rev, + &root_tree, + )?; + + Ok(( + PackageNames::Pesde(manifest.name), + VersionId(manifest.version, manifest.target.kind()), + dependencies, + tree.id.to_string(), + )) + }) + .await + .unwrap()?; let new_structure = matches!(name, PackageNames::Pesde(_)); @@ -322,7 +331,7 @@ impl PackageSource for GitPackageSource { version_id, GitPackageRef { repo: self.repo_url.clone(), - tree_id: tree.id.to_string(), + tree_id, new_structure, dependencies, }, @@ -362,65 +371,52 @@ impl PackageSource for GitPackageSource { Err(e) => return Err(errors::DownloadError::Io(e)), } - let repo = gix::open(self.path(project)) - .map_err(|e| errors::DownloadError::OpenRepo(Box::new(self.repo_url.clone()), e))? - .into_sync(); + let path = self.path(project); let repo_url = self.repo_url.clone(); - let tree_id = pkg_ref.tree_id.clone(); + let tree_id = match pkg_ref.tree_id.parse::() { + Ok(oid) => oid, + Err(e) => return Err(errors::DownloadError::ParseTreeId(Box::new(repo_url), e)), + }; - let (repo, records) = spawn_blocking(move || { - let repo = repo.to_thread_local(); + let records = spawn_blocking(move || { + let repo = gix::open(path) + .map_err(|e| errors::DownloadError::OpenRepo(Box::new(repo_url.clone()), e))?; let mut recorder = Recorder::default(); - { - let object_id = match tree_id.parse::() { - Ok(oid) => oid, - Err(e) => { - return Err(errors::DownloadError::ParseTreeId(Box::new(repo_url), e)) - } - }; - let object = match repo.find_object(object_id) { - Ok(object) => object, - Err(e) => { - return Err(errors::DownloadError::ParseOidToObject( - object_id, - Box::new(repo_url), - e, - )) - } - }; - - let tree = match object.peel_to_tree() { - Ok(tree) => tree, - Err(e) => { - return Err(errors::DownloadError::ParseObjectToTree( - Box::new(repo_url), - e, - )) - } - }; - - if let Err(e) = tree.traverse().breadthfirst(&mut recorder) { - return Err(errors::DownloadError::TraverseTree(Box::new(repo_url), e)); + let object = match repo.find_object(tree_id) { + Ok(object) => object, + Err(e) => { + return Err(errors::DownloadError::ParseOidToObject( + tree_id, + Box::new(repo_url), + e, + )) } + }; + + let tree = match object.peel_to_tree() { + Ok(tree) => tree, + Err(e) => { + return Err(errors::DownloadError::ParseObjectToTree( + Box::new(repo_url), + e, + )) + } + }; + + if let Err(e) = tree.traverse().breadthfirst(&mut recorder) { + return Err(errors::DownloadError::TraverseTree(Box::new(repo_url), e)); } - Ok::<_, errors::DownloadError>((repo.into_sync(), recorder.records)) - }) - .await - .unwrap()?; - - let records = { - let repo = repo.to_thread_local(); - - records + recorder + .records .into_iter() .map(|entry| { let object = repo.find_object(entry.oid).map_err(|e| { errors::DownloadError::ParseOidToObject( entry.oid, - Box::new(self.repo_url.clone()), + Box::new(repo_url.clone()), e, ) })?; @@ -434,8 +430,10 @@ impl PackageSource for GitPackageSource { }, )) }) - .collect::, _>>()? - }; + .collect::, _>>() + }) + .await + .unwrap()?; let mut tasks = records .into_iter() @@ -537,14 +535,14 @@ pub mod errors { pub enum ResolveError { /// An error occurred opening the Git repository #[error("error opening Git repository for url {0}")] - OpenRepo(Box, #[source] gix::open::Error), + OpenRepo(Box, #[source] Box), /// An error occurred parsing rev #[error("error parsing rev {0} for repository {1}")] ParseRev( String, Box, - #[source] gix::revision::spec::parse::single::Error, + #[source] Box, ), /// An error occurred parsing rev to object