From 1d131e98c62e85c99594c26014bc7900b401bca2 Mon Sep 17 00:00:00 2001 From: daimond113 Date: Mon, 10 Feb 2025 00:35:51 +0100 Subject: [PATCH] refactor: patch improvements Patches now work much better with incremental installs. They use a Git repository to reset the patch to the original state before applying it. They are now also properly cleaned up after being removed. Also, to allow more flexability the patching process is now done before type extracting, so that types can be patched. Lastly, the patch-commit command now doesn't delete the patch directory until it has done everything else to prevent data loss. --- CHANGELOG.md | 4 + src/cli/commands/patch_commit.rs | 10 +- src/cli/install.rs | 19 +-- src/download_and_link.rs | 66 +++++++-- src/linking/incremental.rs | 78 ++++++++--- src/patches.rs | 228 ++++++++++++++++--------------- 6 files changed, 245 insertions(+), 160 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ac6bbc3..f3cb8e7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Fixed - Correct script linker require paths on Windows by @daimond113 +- Improve patches in incremental installs by @daimond113 + +### Changed +- Patches are now applied before type extraction to allow patches to modify types by @daimond113 ## [0.6.0-rc.4] - 2025-02-08 ### Fixed diff --git a/src/cli/commands/patch_commit.rs b/src/cli/commands/patch_commit.rs index 88d4e57..ec04737 100644 --- a/src/cli/commands/patch_commit.rs +++ b/src/cli/commands/patch_commit.rs @@ -59,9 +59,6 @@ impl PatchCommitCommand { .context("failed to parse manifest")?; let patch = create_patch(&self.directory).context("failed to create patch")?; - fs::remove_dir_all(self.directory) - .await - .context("failed to remove patch directory")?; let patches_dir = project.package_dir().join("patches"); fs::create_dir_all(&patches_dir) @@ -75,9 +72,6 @@ impl PatchCommitCommand { ); let patch_file = patches_dir.join(&patch_file_name); - if patch_file.exists() { - anyhow::bail!("patch file already exists: {}", patch_file.display()); - } fs::write(&patch_file, patch) .await @@ -92,6 +86,10 @@ impl PatchCommitCommand { .await .context("failed to write manifest")?; + fs::remove_dir_all(self.directory) + .await + .context("failed to remove patch directory")?; + println!(concat!( "done! run `", env!("CARGO_BIN_NAME"), diff --git a/src/cli/install.rs b/src/cli/install.rs index eb44e28..c2d681e 100644 --- a/src/cli/install.rs +++ b/src/cli/install.rs @@ -300,12 +300,7 @@ pub async fn install( .download_and_link( &graph, DownloadAndLinkOptions::::new(reqwest.clone()) - .reporter( - #[cfg(feature = "patches")] - reporter.clone(), - #[cfg(not(feature = "patches"))] - reporter, - ) + .reporter(reporter) .hooks(hooks) .refreshed_sources(refreshed_sources.clone()) .prod(options.prod) @@ -315,18 +310,6 @@ pub async fn install( .await .context("failed to download and link dependencies")?; - #[cfg(feature = "patches")] - { - use pesde::graph::ConvertableGraph; - root_progress.reset(); - root_progress.set_length(0); - root_progress.set_message("patch"); - - project - .apply_patches(&downloaded_graph.clone().convert(), reporter) - .await?; - } - #[cfg(feature = "version-management")] { let mut tasks = manifest diff --git a/src/download_and_link.rs b/src/download_and_link.rs index 3e59fa2..cb2ad35 100644 --- a/src/download_and_link.rs +++ b/src/download_and_link.rs @@ -6,7 +6,7 @@ use crate::{ DependencyGraphWithTarget, }, manifest::{target::TargetKind, DependencyType}, - reporters::DownloadsReporter, + reporters::{DownloadsReporter, PatchesReporter}, source::{ ids::PackageId, traits::{GetTargetOptions, PackageRef, PackageSource}, @@ -85,7 +85,7 @@ pub struct DownloadAndLinkOptions { impl DownloadAndLinkOptions where - Reporter: DownloadsReporter + Send + Sync + 'static, + Reporter: DownloadsReporter + PatchesReporter + Send + Sync + 'static, Hooks: DownloadAndLinkHooks + Send + Sync + 'static, { /// Creates a new download options with the given reqwest client and reporter. @@ -161,7 +161,7 @@ impl Project { options: DownloadAndLinkOptions, ) -> Result> where - Reporter: DownloadsReporter + 'static, + Reporter: DownloadsReporter + PatchesReporter + 'static, Hooks: DownloadAndLinkHooks + 'static, { let DownloadAndLinkOptions { @@ -203,13 +203,13 @@ impl Project { } // step 1. download dependencies - let downloaded_graph = { + let graph_to_download = { let mut download_graph_options = DownloadGraphOptions::::new(reqwest.clone()) .refreshed_sources(refreshed_sources.clone()) .network_concurrency(network_concurrency); - if let Some(reporter) = reporter { - download_graph_options = download_graph_options.reporter(reporter.clone()); + if let Some(reporter) = reporter.clone() { + download_graph_options = download_graph_options.reporter(reporter); } let mut downloaded_graph = DependencyGraph::new(); @@ -273,9 +273,10 @@ impl Project { downloaded_graph }; - let (downloaded_wally_graph, downloaded_other_graph) = downloaded_graph - .into_iter() - .partition::, _>(|(_, node)| node.pkg_ref.is_wally_package()); + let (wally_graph_to_download, other_graph_to_download) = + graph_to_download + .into_iter() + .partition::, _>(|(_, node)| node.pkg_ref.is_wally_package()); let mut graph = Arc::new(DependencyGraphWithTarget::new()); @@ -329,7 +330,7 @@ impl Project { &mut graph, self, manifest.target.kind(), - downloaded_other_graph, + other_graph_to_download, ) .instrument(tracing::debug_span!("get targets (non-wally)")) .await?; @@ -355,11 +356,49 @@ impl Project { &mut graph, self, manifest.target.kind(), - downloaded_wally_graph, + wally_graph_to_download, ) .instrument(tracing::debug_span!("get targets (wally)")) .await?; + #[cfg(feature = "patches")] + { + use crate::patches::apply_patch; + let mut tasks = manifest + .patches + .iter() + .flat_map(|(name, versions)| { + versions + .iter() + .map(|(v_id, path)| (PackageId::new(name.clone(), v_id.clone()), path)) + }) + .filter_map(|(id, patch_path)| graph.get(&id).map(|node| (id, node, patch_path))) + .map(|(id, node, patch_path)| { + let patch_path = patch_path.to_path(self.package_dir()); + let container_folder = + node.node + .container_folder_from_project(&id, self, manifest.target.kind()); + let reporter = reporter.clone(); + + async move { + match reporter { + Some(reporter) => { + apply_patch(&id, container_folder, &patch_path, reporter.clone()) + .await + } + None => { + apply_patch(&id, container_folder, &patch_path, Arc::new(())).await + } + } + } + }) + .collect::>(); + + while let Some(task) = tasks.join_next().await { + task.unwrap()?; + } + } + // step 4. link ALL dependencies. do so with types self.link_dependencies(graph.clone(), true) .instrument(tracing::debug_span!("link (all)")) @@ -421,5 +460,10 @@ pub mod errors { /// Removing unused dependencies failed #[error("error removing unused dependencies")] RemoveUnused(#[from] crate::linking::incremental::errors::RemoveUnusedError), + + /// Patching a package failed + #[cfg(feature = "patches")] + #[error("error applying patch")] + Patch(#[from] crate::patches::errors::ApplyPatchError), } } diff --git a/src/linking/incremental.rs b/src/linking/incremental.rs index 631a891..a4a9744 100644 --- a/src/linking/incremental.rs +++ b/src/linking/incremental.rs @@ -1,12 +1,13 @@ use crate::{ - all_packages_dirs, graph::DependencyGraphWithTarget, manifest::Alias, util::remove_empty_dir, - Project, PACKAGES_CONTAINER_NAME, SCRIPTS_LINK_FOLDER, + all_packages_dirs, graph::DependencyGraphWithTarget, manifest::Alias, patches::remove_patch, + source::ids::PackageId, util::remove_empty_dir, Project, PACKAGES_CONTAINER_NAME, + SCRIPTS_LINK_FOLDER, }; use fs_err::tokio as fs; use futures::FutureExt; use std::{ collections::HashSet, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, sync::Arc, }; use tokio::task::JoinSet; @@ -14,19 +15,33 @@ use tokio::task::JoinSet; fn index_entry( entry: fs::DirEntry, packages_index_dir: &Path, - tasks: &mut JoinSet>, + tasks: &mut JoinSet>, used_paths: &Arc>, + patched_packages: &Arc>, ) { + fn get_package_name_from_container(container: &Path) -> (bool, String) { + let Component::Normal(first_component) = container.components().next().unwrap() else { + panic!("invalid container path: `{}`", container.display()); + }; + + let first_component = first_component.to_string_lossy(); + let Some((name, _)) = first_component.split_once('@') else { + return ( + false, + first_component.split_once('+').unwrap().1.to_string(), + ); + }; + + (true, name.split_once('_').unwrap().1.to_string()) + } + let path = entry.path(); let path_relative = path.strip_prefix(packages_index_dir).unwrap().to_path_buf(); - let is_wally = entry - .file_name() - .to_str() - .expect("non UTF-8 folder name in packages index") - .contains("@"); + let (is_wally, package_name) = get_package_name_from_container(&path_relative); let used_paths = used_paths.clone(); + let patched_packages = patched_packages.clone(); tasks.spawn(async move { if is_wally { #[cfg(not(feature = "wally-compat"))] @@ -40,13 +55,15 @@ fn index_entry( { if !used_paths.contains(&path_relative) { fs::remove_dir_all(path).await?; + } else if !patched_packages.contains(&path_relative) { + remove_patch(path.join(package_name)).await?; } return Ok(()); } } - let mut tasks = JoinSet::new(); + let mut tasks = JoinSet::>::new(); let mut entries = fs::read_dir(&path).await?; while let Some(entry) = entries.next_entry().await? { @@ -54,24 +71,28 @@ fn index_entry( let path_relative = path_relative.join(&version); if used_paths.contains(&path_relative) { + if !patched_packages.contains(&path_relative) { + let path = entry.path().join(&package_name); + tasks.spawn(async { remove_patch(path).await.map_err(Into::into) }); + } continue; } let path = entry.path(); - tasks.spawn(async { fs::remove_dir_all(path).await }); + tasks.spawn(async { fs::remove_dir_all(path).await.map_err(Into::into) }); } while let Some(task) = tasks.join_next().await { task.unwrap()?; } - remove_empty_dir(&path).await + remove_empty_dir(&path).await.map_err(Into::into) }); } fn packages_entry( entry: fs::DirEntry, - tasks: &mut JoinSet>, + tasks: &mut JoinSet>, expected_aliases: &Arc>, ) { let expected_aliases = expected_aliases.clone(); @@ -105,7 +126,7 @@ fn packages_entry( fn scripts_entry( entry: fs::DirEntry, - tasks: &mut JoinSet>, + tasks: &mut JoinSet>, expected_aliases: &Arc>, ) { let expected_aliases = expected_aliases.clone(); @@ -154,6 +175,24 @@ impl Project { }) .collect::>(); let used_paths = Arc::new(used_paths); + let patched_packages = manifest + .patches + .iter() + .flat_map(|(name, versions)| { + versions + .iter() + .map(|(v_id, _)| PackageId::new(name.clone(), v_id.clone())) + }) + .filter_map(|id| graph.get(&id).map(|node| (id, node))) + .map(|(id, node)| { + node.node + .container_folder(&id) + .parent() + .unwrap() + .to_path_buf() + }) + .collect::>(); + let patched_packages = Arc::new(dbg!(patched_packages)); let mut tasks = all_packages_dirs() .into_iter() @@ -161,6 +200,7 @@ impl Project { let packages_dir = self.package_dir().join(&folder); let packages_index_dir = packages_dir.join(PACKAGES_CONTAINER_NAME); let used_paths = used_paths.clone(); + let patched_packages = patched_packages.clone(); let expected_aliases = graph .iter() @@ -181,7 +221,7 @@ impl Project { let mut index_entries = match fs::read_dir(&packages_index_dir).await { Ok(entries) => entries, Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), - Err(e) => return Err(e), + Err(e) => return Err(e.into()), }; // we don't handle NotFound here because the upper level will handle it let mut packages_entries = fs::read_dir(&packages_dir).await?; @@ -195,6 +235,7 @@ impl Project { &packages_index_dir, &mut tasks, &used_paths, + &patched_packages, ); } Some(entry) = packages_entries.next_entry().map(Result::transpose) => { @@ -215,7 +256,7 @@ impl Project { remove_empty_dir(&packages_index_dir).await?; remove_empty_dir(&packages_dir).await?; - Ok::<_, std::io::Error>(()) + Ok::<_, errors::RemoveUnusedError>(()) } }) .collect::>(); @@ -267,5 +308,10 @@ pub mod errors { /// IO error #[error("IO error")] Io(#[from] std::io::Error), + + /// Removing a patch failed + #[cfg(feature = "patches")] + #[error("error removing patch")] + PatchRemove(#[from] crate::patches::errors::ApplyPatchError), } } diff --git a/src/patches.rs b/src/patches.rs index 76d3c3f..f9f85a5 100644 --- a/src/patches.rs +++ b/src/patches.rs @@ -1,16 +1,17 @@ use crate::{ - graph::DependencyGraph, reporters::{PatchProgressReporter, PatchesReporter}, source::ids::PackageId, - Project, MANIFEST_FILE_NAME, + MANIFEST_FILE_NAME, }; use fs_err::tokio as fs; use futures::TryFutureExt; use git2::{ApplyLocation, Diff, DiffFormat, DiffLineType, Repository, Signature}; -use relative_path::RelativePathBuf; -use std::{path::Path, sync::Arc}; -use tokio::task::JoinSet; -use tracing::{instrument, Instrument}; +use std::{ + path::{Path, PathBuf}, + sync::Arc, +}; +use tokio::task::{spawn_blocking, JoinSet}; +use tracing::instrument; /// Set up a git repository for patches pub fn setup_patches_repo>(dir: P) -> Result { @@ -43,7 +44,7 @@ pub fn setup_patches_repo>(dir: P) -> Result>(dir: P) -> Result, git2::Error> { - let mut patches = vec![]; + let mut patch = vec![]; let repo = Repository::open(dir.as_ref())?; let original = repo.head()?.peel_to_tree()?; @@ -54,7 +55,12 @@ pub fn create_patch>(dir: P) -> Result, git2::Error> { checkout_builder.path(MANIFEST_FILE_NAME); repo.checkout_tree(original.as_object(), Some(&mut checkout_builder))?; - let diff = repo.diff_tree_to_workdir(Some(&original), None)?; + // TODO: despite all the options, this still doesn't include untracked files + let mut diff_options = git2::DiffOptions::default(); + diff_options.include_untracked(true); + diff_options.recurse_untracked_dirs(true); + + let diff = repo.diff_tree_to_workdir(Some(&original), Some(&mut diff_options))?; diff.print(DiffFormat::Patch, |_delta, _hunk, line| { if matches!( @@ -64,119 +70,127 @@ pub fn create_patch>(dir: P) -> Result, git2::Error> { let origin = line.origin(); let mut buffer = vec![0; origin.len_utf8()]; origin.encode_utf8(&mut buffer); - patches.extend(buffer); + patch.extend(buffer); } - patches.extend(line.content()); + patch.extend(line.content()); true })?; - Ok(patches) + Ok(patch) } -impl Project { - /// Apply patches to the project's dependencies - #[instrument(skip(self, graph, reporter), level = "debug")] - pub async fn apply_patches( - &self, - graph: &DependencyGraph, - reporter: Arc, - ) -> Result<(), errors::ApplyPatchesError> - where - Reporter: PatchesReporter + Send + Sync + 'static, - { - let manifest = self.deser_manifest().await?; +// unlike a simple hard reset, this will also remove untracked files +fn reset_repo(repo: &Repository) -> Result<(), git2::Error> { + let mut checkout_builder = git2::build::CheckoutBuilder::new(); + checkout_builder.force(); + checkout_builder.remove_untracked(true); + repo.checkout_head(Some(&mut checkout_builder))?; - let mut tasks = manifest - .patches - .into_iter() - .flat_map(|(name, versions)| { - versions.into_iter().map(move |(version_id, patch_path)| { - (PackageId::new(name.clone(), version_id), patch_path) - }) - }) - .filter_map(|(package_id, patch_path)| match graph.get(&package_id) { - Some(node) => Some((package_id, patch_path, node)), - None => { - tracing::warn!( - "patch for {package_id} not applied because it is not in the graph" - ); - None - } - }) - .map(|(package_id, patch_path, node)| { - let patch_path = patch_path.to_path(self.package_dir()); - let span = tracing::info_span!("apply patch", package_id = package_id.to_string()); - let container_folder = - node.container_folder_from_project(&package_id, self, manifest.target.kind()); - let reporter = reporter.clone(); + Ok(()) +} - async move { - tracing::debug!("applying patch"); +/// Apply a patch to a dependency +#[instrument(skip(container_folder, patch_path, reporter), level = "debug")] +pub async fn apply_patch( + package_id: &PackageId, + container_folder: PathBuf, + patch_path: &Path, + reporter: Arc, +) -> Result<(), errors::ApplyPatchError> +where + Reporter: PatchesReporter + Send + Sync + 'static, +{ + let dot_git = container_folder.join(".git"); - let progress_reporter = reporter.report_patch(package_id.to_string()); + tracing::debug!("applying patch"); - let patch = fs::read(&patch_path) - .await - .map_err(errors::ApplyPatchesError::PatchRead)?; - let patch = Diff::from_buffer(&patch)?; + let progress_reporter = reporter.report_patch(package_id.to_string()); - { - let repo = setup_patches_repo(&container_folder)?; + let patch = fs::read(&patch_path) + .await + .map_err(errors::ApplyPatchError::PatchRead)?; + let patch = spawn_blocking(move || Diff::from_buffer(&patch)) + .await + .unwrap()?; - let mut apply_delta_tasks = patch - .deltas() - .filter(|delta| matches!(delta.status(), git2::Delta::Modified)) - .filter_map(|delta| delta.new_file().path()) - .map(|path| { - RelativePathBuf::from_path(path) - .unwrap() - .to_path(&container_folder) - }) - .map(|path| { - async { - if !fs::metadata(&path).await?.is_file() { - return Ok(()); - } + let mut apply_delta_tasks = patch + .deltas() + .filter(|delta| matches!(delta.status(), git2::Delta::Modified)) + .filter_map(|delta| delta.new_file().path()) + .map(|path| { + let path = container_folder.join(path); - // prevent CAS corruption by the file being modified - let content = fs::read(&path).await?; - fs::remove_file(&path).await?; - fs::write(path, content).await?; - Ok(()) - } - .map_err(errors::ApplyPatchesError::File) - }) - .collect::>(); + async { + // prevent CAS corruption by the file being modified + let content = match fs::read(&path).await { + Ok(content) => content, + Err(e) if e.kind() == std::io::ErrorKind::IsADirectory => return Ok(()), + Err(e) => return Err(e), + }; + fs::remove_file(&path).await?; + fs::write(path, content).await?; + Ok(()) + } + .map_err(errors::ApplyPatchError::File) + }) + .collect::>(); - while let Some(res) = apply_delta_tasks.join_next().await { - res.unwrap()?; - } - - repo.apply(&patch, ApplyLocation::WorkDir, None)?; - } - - tracing::debug!("patch applied"); - - fs::remove_dir_all(container_folder.join(".git")) - .await - .map_err(errors::ApplyPatchesError::DotGitRemove)?; - - progress_reporter.report_done(); - - Ok::<_, errors::ApplyPatchesError>(()) - } - .instrument(span) - }) - .collect::>(); - - while let Some(res) = tasks.join_next().await { - res.unwrap()? - } - - Ok(()) + while let Some(res) = apply_delta_tasks.join_next().await { + res.unwrap()?; } + + spawn_blocking(move || { + let repo = if dot_git.exists() { + let repo = Repository::open(&container_folder)?; + reset_repo(&repo)?; + repo + } else { + setup_patches_repo(&container_folder)? + }; + + repo.apply(&patch, ApplyLocation::WorkDir, None) + }) + .await + .unwrap()?; + + tracing::debug!("patch applied"); + + progress_reporter.report_done(); + + Ok::<_, errors::ApplyPatchError>(()) +} + +/// Remove a patch from a dependency +#[instrument(level = "debug")] +pub async fn remove_patch(container_folder: PathBuf) -> Result<(), errors::ApplyPatchError> { + let dot_git = container_folder.join(".git"); + + tracing::debug!("removing patch"); + + if dbg!(fs::metadata(&dot_git).await).is_err() { + return Ok(()); + } + + spawn_blocking(move || { + let repo = Repository::open(&container_folder)?; + reset_repo(&repo)?; + + Ok::<_, git2::Error>(()) + }) + .await + .unwrap()?; + + match fs::remove_dir_all(&dot_git).await { + Ok(()) => (), + Err(e) if e.kind() == std::io::ErrorKind::NotFound => (), + Err(e) => return Err(errors::ApplyPatchError::File(e)), + } + + tracing::debug!("patch removed"); + + Ok::<_, errors::ApplyPatchError>(()) } /// Errors that can occur when using patches @@ -186,11 +200,7 @@ pub mod errors { /// Errors that can occur when applying patches #[derive(Debug, Error)] #[non_exhaustive] - pub enum ApplyPatchesError { - /// Error deserializing the project manifest - #[error("error deserializing project manifest")] - ManifestDeserializationFailed(#[from] crate::errors::ManifestReadError), - + pub enum ApplyPatchError { /// Error interacting with git #[error("error interacting with git")] Git(#[from] git2::Error),