From 6082812f8fc8aa6928af54a5a575376c47a9132b Mon Sep 17 00:00:00 2001 From: Barsik Date: Mon, 8 Apr 2024 21:54:47 +0300 Subject: [PATCH 1/4] Refactor commit operations and implement package version revert mechanism on failure. Refactored separable git operations into individual functions (e.g., 'push' separated from 'commit'). Implemented package version revert mechanism to automatically revert to the old version when git operations fail. This involves the introduction of 'version_revert' and git 'reset' functions to handle version control more efficiently. --- module/move/willbe/src/entity/manifest.rs | 15 +++++ module/move/willbe/src/entity/package.rs | 33 ++++++++--- module/move/willbe/src/entity/version.rs | 67 +++++++++++++++++++++++ module/move/willbe/src/tool/git.rs | 53 ++++++++++++++++++ 4 files changed, 161 insertions(+), 7 deletions(-) diff --git a/module/move/willbe/src/entity/manifest.rs b/module/move/willbe/src/entity/manifest.rs index 619a5bab8c..aada66ec97 100644 --- a/module/move/willbe/src/entity/manifest.rs +++ b/module/move/willbe/src/entity/manifest.rs @@ -137,6 +137,21 @@ pub( crate ) mod private impl Manifest { + /// Returns a mutable reference to the TOML document. + /// + /// If the TOML document has not been loaded yet, this function will load it + /// by calling the `load` method. If loading fails, this function will panic. + /// + /// # Returns + /// + /// A mutable reference to the TOML document. + pub fn data( &mut self ) -> &mut toml_edit::Document + { + if self.manifest_data.is_none() { self.load().unwrap() } + + self.manifest_data.as_mut().unwrap() + } + /// Returns path to `Cargo.toml`. pub fn manifest_path( &self ) -> &AbsolutePath { diff --git a/module/move/willbe/src/entity/package.rs b/module/move/willbe/src/entity/package.rs index 4fb858191e..8bcf6d7dc9 100644 --- a/module/move/willbe/src/entity/package.rs +++ b/module/move/willbe/src/entity/package.rs @@ -33,6 +33,7 @@ mod private use former::Former; use workspace::WorkspacePackage; use diff::crate_diff; + use version::version_revert; /// #[ derive( Debug, Clone ) ] @@ -311,7 +312,7 @@ mod private pub dry : bool, } - fn perform_git_operations( o : GitThingsOptions ) -> Result< ExtendedGitReport > + fn perform_git_commit( o : GitThingsOptions ) -> Result< ExtendedGitReport > { let mut report = ExtendedGitReport::default(); if o.items.is_empty() { return Ok( report ); } @@ -328,8 +329,6 @@ mod private report.add = Some( res ); let res = git::commit( &o.git_root, &o.message, o.dry ).map_err( | e | format_err!( "{report:?}\n{e:#?}" ) )?; report.commit = Some( res ); - let res = git::push( &o.git_root, o.dry ).map_err( | e | format_err!( "{report:?}\n{e:#?}" ) )?; - report.push = Some( res ); Ok( report ) } @@ -437,12 +436,32 @@ mod private report.get_info = Some( cargo::pack( pack ).map_err( | e | format_err!( "{report}\n{e:#?}" ) )? ); // qqq : redundant field? report.publish_required = true; - report.bump = Some( version::version_bump( version_bump ).map_err( | e | format_err!( "{report}\n{e:#?}" ) )? ); - let git = perform_git_operations( git_things ).map_err( |e | format_err!( "{report}\n{e:#?}" ) )?; + let bump_report = version::version_bump( version_bump ).map_err( | e | format_err!( "{report}\n{e:#?}" ) )?; + report.bump = Some( bump_report.clone() ); + let git_root = git_things.git_root.clone(); + let git = match perform_git_commit( git_things ).map_err( |e | format_err!( "{report}\n{e:#?}" ) ) + { + Ok( git ) => git, + Err( e ) => + { + version_revert( &bump_report ).map_err( | le | format_err!( "{report}\n{e:#?}\n{le:#?}" ) )?; + return Err( format_err!( "{report}\n{e:#?}" ) ); + } + }; report.add = git.add; report.commit = git.commit; - report.push = git.push; - report.publish = Some( cargo::publish( publish ).map_err( | e | format_err!( "{report}\n{e:#?}" ) )? ); + report.publish = match cargo::publish( publish ).map_err( | e | format_err!( "{report}\n{e:#?}" ) ) + { + Ok( publish ) => Some( publish ), + Err( e ) => + { + git::reset( git_root.as_ref(), true, 1, false ).map_err( | le | format_err!( "{report}\n{e:#?}\n{le:#?}" ) )?; + return Err( format_err!( "{report}\n{e:#?}" ) ); + } + }; + + let res = git::push( &git_root, dry ).map_err( | e | format_err!( "{report}\n{e:#?}" ) )?; + report.push = Some( res ); Ok( report ) } diff --git a/module/move/willbe/src/entity/version.rs b/module/move/willbe/src/entity/version.rs index 50f851ce7d..d20329839e 100644 --- a/module/move/willbe/src/entity/version.rs +++ b/module/move/willbe/src/entity/version.rs @@ -300,6 +300,71 @@ mod private Ok( report ) } + + /// Reverts the version of a package in the provided `ExtendedBumpReport`. + /// + /// # Arguments + /// + /// * `report` - The `ExtendedBumpReport` containing the bump information. + /// + /// # Returns + /// + /// Returns `Ok(())` if the version is reverted successfully. Returns `Err` with an error message if there is any issue with reverting the version. + pub fn version_revert( report : &ExtendedBumpReport ) -> Result< () > + { + let Some( name ) = report.name.as_ref() else { return Ok( () ) }; + let Some( old_version ) = report.old_version.as_ref() else { return Ok( () ) }; + let Some( new_version ) = report.new_version.as_ref() else { return Ok( () ) }; + + let dependencies = | item_maybe_with_dependencies : &mut toml_edit::Item | + { + if let Some( dependency ) = item_maybe_with_dependencies.get_mut( "dependencies" ).and_then( | ds | ds.get_mut( name ) ) + { + if let Some( current_version ) = dependency.get( "version" ).and_then( | v | v.as_str() ).map( | v | v.to_string() ) + { + let version = &mut dependency[ "version" ]; + if let Some( current_version ) = current_version.strip_prefix( '~' ) + { + if current_version != new_version { return Err( format_err!( "The current version of the package does not match the expected one. Expected: `{new_version}` Current: `{}`", version.as_str().unwrap_or_default() ) ); } + *version = value( format!( "~{}", old_version ) ); + } + else + { + if version.as_str().unwrap() != new_version { return Err( format_err!( "The current version of the package does not match the expected one. Expected: `{new_version}` Current: `{}`", version.as_str().unwrap_or_default() ) ); } + *version = value( old_version.clone() ); + } + } + } + + Ok( () ) + }; + + for path in &report.changed_files + { + let mut manifest = manifest::open( path.clone() )?; + let data = manifest.data(); + if let Some( workspace ) = data.get_mut( "workspace" ) + { + dependencies( workspace )?; + } + if let Some( package ) = data.get_mut( "package" ) + { + if package.get_mut( "name" ).unwrap().as_str().unwrap() == name + { + let version = &mut package[ "version" ]; + if version.as_str().unwrap() != new_version { return Err( format_err!( "The current version of the package does not match the expected one. Expected: `{new_version}` Current: `{}`", version.as_str().unwrap_or_default() ) ); } + *version = value( old_version.clone() ); + } + else + { + dependencies( package )?; + } + } + manifest.store()?; + } + + Ok( () ) + } } // @@ -321,4 +386,6 @@ crate::mod_interface! protected use ExtendedBumpReport; /// Bumps the version of a package and its dependencies. protected use version_bump; + /// Reverts the version of a package. + protected use version_revert; } diff --git a/module/move/willbe/src/tool/git.rs b/module/move/willbe/src/tool/git.rs index 844e921ac6..c90dc08109 100644 --- a/module/move/willbe/src/tool/git.rs +++ b/module/move/willbe/src/tool/git.rs @@ -138,6 +138,58 @@ mod private .run().map_err( | report | err!( report.to_string() ) ) } } + + /// This function is a wrapper around the `git reset` command. + /// + /// # Args : + /// + /// - `path`: The path to the directory on which the `git reset` command will be executed. + /// - `hard`: A boolean indicating whether to perform a hard reset or not. + /// - `commits_count`: The number of commits to reset(at least 1). + /// - `dry`: A boolean indicating whether to execute the command in dry-run mode or not. + /// + /// # Returns : + /// This function returns a `Result` containing a `Report` if the command is executed successfully. The `Report` contains the command executed, the output +// git reset command wrapper + pub fn reset< P >( path : P, hard : bool, commits_count : usize, dry : bool ) -> Result< Report > + where + P : AsRef< Path >, + { + if commits_count < 1 { return Err( err!( "Cannot reset, the count of commits must be greater than 0" ) ) } + let ( program, args ) = + ( + "git", + Some( "reset" ) + .into_iter() + .chain( if hard { Some( "--hard" ) } else { None } ) + .map( String::from ) + .chain( Some( format!( "HEAD~{}", commits_count ) ) ) + .collect::< Vec< _ > >() + ); + + if dry + { + Ok + ( + Report + { + command : format!( "{program} {}", args.join( " " ) ), + out : String::new(), + err : String::new(), + current_path : path.as_ref().to_path_buf(), + error : Ok( () ), + } + ) + } + else + { + Run::former() + .bin_path( program ) + .args( args.into_iter().map( OsString::from ).collect::< Vec< _ > >() ) + .current_path( path.as_ref().to_path_buf() ) + .run().map_err( | report | err!( report.to_string() ) ) + } + } /// Retrieves the remote URL of a Git repository. /// @@ -169,5 +221,6 @@ crate::mod_interface! protected use add; protected use commit; protected use push; + protected use reset; protected use ls_remote_url; } From cf927e6429c0fa8f1ce63eba570bf07c5219e057 Mon Sep 17 00:00:00 2001 From: Barsik Date: Mon, 8 Apr 2024 23:00:54 +0300 Subject: [PATCH 2/4] Add version bump and revert tests Two tests are added to `version.rs`, testing version bump and version revert functionalities. The new tests cover the whole process of version change in a package, from the initial version to the updated version, as well as the reverting back to the original version. --- .../move/willbe/tests/inc/entity/version.rs | 149 +++++++++++++++++- 1 file changed, 148 insertions(+), 1 deletion(-) diff --git a/module/move/willbe/tests/inc/entity/version.rs b/module/move/willbe/tests/inc/entity/version.rs index 32362ba6fa..328bd07834 100644 --- a/module/move/willbe/tests/inc/entity/version.rs +++ b/module/move/willbe/tests/inc/entity/version.rs @@ -1,5 +1,26 @@ -use crate::the_module::version::Version; +use crate::*; + +use std::path::{ Path, PathBuf }; use std::str::FromStr; +use std::io::Write; +use assert_fs::prelude::*; +use the_module:: +{ + CrateDir, + Manifest, + version::Version, + _path::AbsolutePath, + package::Package, + version::{ BumpOptions, version_bump, version_revert }, +}; + +const TEST_MODULE_PATH : &str = "../../test/"; + +fn package_path< P : AsRef< Path > >( path : P ) -> PathBuf +{ + let root_path = Path::new( env!( "CARGO_MANIFEST_DIR" ) ).join( TEST_MODULE_PATH ); + root_path.join( path ) +} #[ test ] fn patch() @@ -78,3 +99,129 @@ fn major_with_patches() // Assert assert_eq!( "2.0.0", &new_version.to_string() ); } + +#[ test ] +fn package_version_bump() +{ + // Arrange + let c = package_path( "c" ); + let temp = assert_fs::TempDir::new().unwrap(); + let temp_module = temp.child( "module" ); + std::fs::create_dir( &temp_module ).unwrap(); + temp_module.child( "c" ).copy_from( &c, &[ "**" ] ).unwrap(); + let c_temp_path = temp_module.join( "c" ); + let c_temp_absolute_path = AbsolutePath::try_from( c_temp_path ).unwrap(); + let c_temp_crate_dir = CrateDir::try_from( c_temp_absolute_path.clone() ).unwrap(); + let c_package = Package::try_from( c_temp_absolute_path.clone() ).unwrap(); + let version = c_package.version().unwrap(); + + let root_manifest_path = temp.join( "Cargo.toml" ); + let mut cargo_toml = std::fs::File::create( &root_manifest_path ).unwrap(); + let root_manifest_absolute_path = AbsolutePath::try_from( root_manifest_path.as_path() ).unwrap(); + write!( cargo_toml, r#" +[workspace] +resolver = "2" +members = [ + "module/*", +] +[workspace.dependencies.test_experimental_c] +version = "{version}" +path = "module/c" +default-features = true +"# ).unwrap(); + let version = Version::try_from( &version ).unwrap(); + let bumped_version = version.clone().bump(); + + // Act + let options = BumpOptions + { + crate_dir : c_temp_crate_dir, + old_version : version.clone(), + new_version : bumped_version.clone(), + dependencies : vec![ CrateDir::try_from( root_manifest_absolute_path.parent().unwrap() ).unwrap() ], + dry : false, + }; + let bump_report = version_bump( options ).unwrap(); + + // Assert + assert_eq!( Some( version.to_string() ), bump_report.old_version ); + assert_eq!( Some( bumped_version.to_string() ), bump_report.new_version ); + assert_eq! + ( + { + let mut v = vec![ root_manifest_absolute_path.clone(), c_temp_absolute_path.join( "Cargo.toml" ) ]; + v.sort(); + v + }, + { + let mut v = bump_report.changed_files; + v.sort(); + v + } + ); + let c_package = Package::try_from( c_temp_absolute_path.clone() ).unwrap(); + let name = c_package.name().unwrap(); + assert_eq!( bumped_version.to_string(), c_package.version().unwrap() ); + let mut root_manifest = Manifest::try_from( root_manifest_absolute_path ).unwrap(); + root_manifest.load().unwrap(); + let data = root_manifest.data(); + let current_version_item = data.get( "workspace" ).and_then( | w | w.get( "dependencies" ) ).and_then( | d | d.get( &name ) ).and_then( | p | p.get( "version" ) ).unwrap(); + let current_version = current_version_item.as_str().unwrap(); + assert_eq!( &bumped_version.to_string(), current_version ); +} + +#[ test ] +fn package_version_bump_revert() +{ + // Arrange + let c = package_path( "c" ); + let temp = assert_fs::TempDir::new().unwrap(); + let temp_module = temp.child( "module" ); + std::fs::create_dir( &temp_module ).unwrap(); + temp_module.child( "c" ).copy_from( &c, &[ "**" ] ).unwrap(); + let c_temp_path = temp_module.join( "c" ); + let c_temp_absolute_path = AbsolutePath::try_from( c_temp_path ).unwrap(); + let c_temp_crate_dir = CrateDir::try_from( c_temp_absolute_path.clone() ).unwrap(); + let c_package = Package::try_from( c_temp_absolute_path.clone() ).unwrap(); + let version = c_package.version().unwrap(); + + let root_manifest_path = temp.join( "Cargo.toml" ); + let mut cargo_toml = std::fs::File::create( &root_manifest_path ).unwrap(); + let root_manifest_absolute_path = AbsolutePath::try_from( root_manifest_path.as_path() ).unwrap(); + write!( cargo_toml, r#" +[workspace] +resolver = "2" +members = [ + "module/*", +] +[workspace.dependencies.test_experimental_c] +version = "{version}" +path = "module/c" +default-features = true +"# ).unwrap(); + let version = Version::try_from( &version ).unwrap(); + let bumped_version = version.clone().bump(); + + // Act + let options = BumpOptions + { + crate_dir : c_temp_crate_dir, + old_version : version.clone(), + new_version : bumped_version.clone(), + dependencies : vec![ CrateDir::try_from( root_manifest_absolute_path.parent().unwrap() ).unwrap() ], + dry : false, + }; + let bump_report = version_bump( options ).unwrap(); + version_revert( &bump_report ).unwrap(); + + // Assert + let c_package = Package::try_from( c_temp_absolute_path.clone() ).unwrap(); + let name = c_package.name().unwrap(); + assert_eq!( version.to_string(), c_package.version().unwrap() ); + let mut root_manifest = Manifest::try_from( root_manifest_absolute_path ).unwrap(); + root_manifest.load().unwrap(); + let data = root_manifest.data(); + let current_version_item = data.get( "workspace" ).and_then( | w | w.get( "dependencies" ) ).and_then( | d | d.get( &name ) ).and_then( | p | p.get( "version" ) ).unwrap(); + let current_version = current_version_item.as_str().unwrap(); + assert_eq!( &version.to_string(), current_version ); +} From 90bd73c972d363477d9d80a8afd7998ba501f750 Mon Sep 17 00:00:00 2001 From: Barsik Date: Mon, 6 May 2024 13:12:01 +0300 Subject: [PATCH 3/4] Fixing merge conflicts --- module/move/willbe/src/entity/package.rs | 4 ++-- module/move/willbe/src/tool/graph.rs | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/module/move/willbe/src/entity/package.rs b/module/move/willbe/src/entity/package.rs index c31acc6066..eae18b499b 100644 --- a/module/move/willbe/src/entity/package.rs +++ b/module/move/willbe/src/entity/package.rs @@ -454,7 +454,7 @@ mod private let bump_report = version::version_bump( version_bump ).map_err( | e | ( report.clone(), e ) )?; report.bump = Some( bump_report.clone() ); let git_root = git_things.git_root.clone(); - let git = match perform_git_commit( git_things ).map_err( | e | ( report.clone(), e ) ) + let git = match perform_git_commit( git_things ) { Ok( git ) => git, Err( e ) => @@ -466,7 +466,7 @@ mod private }; report.add = git.add; report.commit = git.commit; - report.publish = match cargo::publish( publish ).map_err( | e | ( report.clone(), e ) ) + report.publish = match cargo::publish( publish ) { Ok( publish ) => Some( publish ), Err( e ) => diff --git a/module/move/willbe/src/tool/graph.rs b/module/move/willbe/src/tool/graph.rs index 5fedda7688..db91c15afb 100644 --- a/module/move/willbe/src/tool/graph.rs +++ b/module/move/willbe/src/tool/graph.rs @@ -269,6 +269,7 @@ pub( crate ) mod private .path( package.crate_dir().absolute_path().as_ref().to_path_buf() ) .option_temp_path( temp_path.clone() ) .dry( false ) + .allow_dirty( true ) .form() )?; if publish_need( package, temp_path.clone() ).unwrap() From 21104b99d066909a81d2f6dc00b0715242bdc7ce Mon Sep 17 00:00:00 2001 From: Barsik Date: Mon, 6 May 2024 13:18:34 +0300 Subject: [PATCH 4/4] Use base error and the error that happens while reverting changes --- module/move/willbe/src/entity/package.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/module/move/willbe/src/entity/package.rs b/module/move/willbe/src/entity/package.rs index eae18b499b..5464dfe858 100644 --- a/module/move/willbe/src/entity/package.rs +++ b/module/move/willbe/src/entity/package.rs @@ -459,8 +459,12 @@ mod private Ok( git ) => git, Err( e ) => { - // use both errors - version_revert( &bump_report ).map_err( | le | ( report.clone(), le ) )?; + version_revert( &bump_report ) + .map_err( | le | + ( + report.clone(), + format_err!( "Base error:\n{}\nRevert error:\n{}", e.to_string().replace( '\n', "\n\t" ), le.to_string().replace( '\n', "\n\t" ) ) + ))?; return Err(( report, e )); } }; @@ -471,8 +475,12 @@ mod private Ok( publish ) => Some( publish ), Err( e ) => { - // use both errors - git::reset( git_root.as_ref(), true, 1, false ).map_err( | le | ( report.clone(), le ) )?; + git::reset( git_root.as_ref(), true, 1, false ) + .map_err( | le | + ( + report.clone(), + format_err!( "Base error:\n{}\nRevert error:\n{}", e.to_string().replace( '\n', "\n\t" ), le.to_string().replace( '\n', "\n\t" ) ) + ))?; return Err(( report, e )); } };