diff --git a/module/move/willbe/Cargo.toml b/module/move/willbe/Cargo.toml index 8086d5372b..e193fe5553 100644 --- a/module/move/willbe/Cargo.toml +++ b/module/move/willbe/Cargo.toml @@ -43,7 +43,6 @@ enabled = [ "wca/enabled", "pth/enabled", "process_tools/enabled", - "derive_tools/enabled", "data_type/enabled", "collection_tools/enabled", "macro_tools/enabled", @@ -81,6 +80,7 @@ serde = "1.0" # for CargoMetadata::Package parse-display = "0.9" # need because derive_tools don't reexport this correctly walkdir = "2.3" rustdoc-md = "0.1.0" +assert_fs = "1.1.3" ## internal # qqq : optimize features @@ -93,7 +93,7 @@ mod_interface = { workspace = true, features = [ "default" ] } wca = { workspace = true, features = [ "default" ] } pth = { workspace = true, features = [ "default", "path_utf8" ] } process_tools = { workspace = true, features = [ "default" ] } -derive_tools = { workspace = true, features = [ "derive_display", "derive_from_str", "derive_deref", "derive_from", "derive_as_ref" ] } +derive_tools = { workspace = true, features = [ "default" ] } # derive_tools is a basic dependency required for compilation. data_type = { workspace = true, features = [ "either" ] } collection_tools = { workspace = true, features = [ "collection_constructors", "collection_into_constructors" ] } macro_tools = { workspace = true, features = [ "default" ] } diff --git a/module/move/willbe/src/action/list.rs b/module/move/willbe/src/action/list.rs index 3ed25a206d..26d908d46f 100644 --- a/module/move/willbe/src/action/list.rs +++ b/module/move/willbe/src/action/list.rs @@ -438,6 +438,27 @@ mod private /// /// - `Result` - A result containing the list report if successful, /// or a tuple containing the list report and error if not successful. + /// # Errors + /// + /// Returns an error if it fails to read the workspace manifest, parse dependencies, + /// or if a dependency cycle is detected in topological sort mode. + /// + /// # Panics + /// + /// The function may panic if it encounters a package version that cannot be parsed + /// into a valid `semver::VersionReq`. This can happen with malformed `Cargo.toml` files. + /// + /// # Errors + /// + /// Returns an error if it fails to read the workspace manifest, parse dependencies, + /// or if a dependency cycle is detected in topological sort mode. + /// + /// # Panics + /// + /// The function may panic if it encounters a package version that cannot be parsed + /// into a valid `semver::VersionReq`. This can happen with malformed `Cargo.toml` files. + /// + #[ allow( clippy::too_many_lines ) ] #[ cfg_attr( feature = "tracing", tracing::instrument ) ] pub fn list_all( args : ListOptions ) -> ResultWithReport< ListReport, error::untyped::Error > // qqq : should be specific error diff --git a/module/move/willbe/src/action/publish.rs b/module/move/willbe/src/action/publish.rs index 99d9dd4223..a181f741fc 100644 --- a/module/move/willbe/src/action/publish.rs +++ b/module/move/willbe/src/action/publish.rs @@ -115,10 +115,19 @@ mod private /// /// # Returns /// A Result containing a `PublishPlan` if successful, or an `Error` otherwise. + /// + /// # Errors + /// + /// Returns an error if it fails to find packages, read the workspace, or create a temporary directory. + /// + /// # Panics + /// + /// Panics if `patterns` is not empty but resolving the first path to a workspace fails, + /// or if toposort fails on the dependency graph. #[ cfg_attr( feature = "tracing", tracing::instrument ) ] pub fn publish_plan ( - patterns : Vec< String >, + patterns : &[ String ], channel : channel::Channel, dry : bool, temp : bool @@ -128,7 +137,7 @@ mod private { let mut paths = collection::HashSet::new(); // find all packages by specified folders - for pattern in &patterns + for pattern in patterns { let current_path = AbsolutePath::try_from ( @@ -175,7 +184,7 @@ mod private &graph, &packages_to_publish[ .. ] ); - let tmp = subgraph_wanted + let tmp_subgraph = subgraph_wanted .map ( | _, n | @@ -212,8 +221,9 @@ mod private let subgraph = graph::remove_not_required_to_publish ( + &workspace, &package_map, - &tmp, + &tmp_subgraph, &packages_to_publish, dir.clone(), )?; @@ -246,7 +256,14 @@ mod private /// /// Publish packages. /// - + /// # Errors + /// + /// Returns an error if any of the publishing steps fail or if cleanup of temporary directories fails. + /// + /// # Panics + /// + /// Panics if the report for a successfully published package is missing expected information. + #[ allow( clippy::result_large_err ) ] #[ cfg_attr( feature = "tracing", tracing::instrument ) ] pub fn publish( plan : publish::PublishPlan ) -> diff --git a/module/move/willbe/src/action/publish_diff.rs b/module/move/willbe/src/action/publish_diff.rs index c204ac5bce..b71747eb7d 100644 --- a/module/move/willbe/src/action/publish_diff.rs +++ b/module/move/willbe/src/action/publish_diff.rs @@ -99,6 +99,16 @@ mod private } /// Return the differences between a local and remote package versions. + /// + /// # Errors + /// + /// Returns an error if there's an issue with path conversion, packing the local crate, + /// or if the internal `list` action returns an unexpected format. + /// + /// # Panics + /// + /// This function may panic if the internal `list_all` action fails, if it's unable to download + /// the package from crates.io, or if a dependency tree walk encounters an unexpected structure. #[ cfg_attr( feature = "tracing", tracing::instrument ) ] pub fn publish_diff( o : PublishDiffOptions ) -> Result< PublishDiffReport > // qqq : don't use 1-prameter Result @@ -106,6 +116,8 @@ mod private let path = AbsolutePath::try_from( o.path )?; let dir = CrateDir::try_from( path.clone() )?; + let workspace = Workspace::try_from( dir.clone() )?; + let list = action::list_all ( action::list::ListOptions::former() @@ -150,7 +162,7 @@ mod private .checking_consistency( false ) .dry( false ).form() )?; - let l = CrateArchive::read( packed_crate::local_path( name, version, dir )? )?; + let l = CrateArchive::read( packed_crate::local_path( name, version, workspace.target_directory() )? )?; let r = CrateArchive::download_crates_io( name, version ).unwrap(); diff --git a/module/move/willbe/src/command/publish.rs b/module/move/willbe/src/command/publish.rs index 004bb3dec8..fb24c6f0ef 100644 --- a/module/move/willbe/src/command/publish.rs +++ b/module/move/willbe/src/command/publish.rs @@ -55,7 +55,7 @@ mod private dry, temp } = o.props.try_into()?; - let plan = action::publish_plan( patterns, channel, dry, temp ) + let plan = action::publish_plan( &patterns, channel, dry, temp ) .context( "Failed to plan the publication process" )?; let mut formatted_plan = String::new(); diff --git a/module/move/willbe/src/entity/package.rs b/module/move/willbe/src/entity/package.rs index b5de716391..93cf72a933 100644 --- a/module/move/willbe/src/entity/package.rs +++ b/module/move/willbe/src/entity/package.rs @@ -208,24 +208,44 @@ mod private // - /// Determines whether a package needs to be published by comparing `.crate` files from the local and remote package. + /// Determines if a package needs to be published by comparing its local `.crate` file against the version on crates.io. /// - /// This function requires the local package to be previously packed. + /// This function first locates the local, pre-packaged `.crate` file and then attempts to download + /// the corresponding version from the remote registry. It returns `true` if there are differences + /// or if the remote version does not exist (implying a new version to be published). /// - /// # Returns : - /// - `true` if the package needs to be published. - /// - `false` if there is no need to publish the package. + /// **Prerequisite**: The local package must have been packaged beforehand (e.g., using `cargo package`). + /// + /// # Arguments + /// + /// * `package` - A reference to the `Package` struct for which the check is being performed. + /// * `path` - An optional path to a directory that contains the packaged `.crate` file. + /// If `Some`, this path is used directly. If `None`, the path is constructed using `target_dir`. + /// * `target_dir` - The path to the workspace's `target` directory, used to find the + /// local `.crate` file if a specific `path` is not provided. + /// + /// # Returns + /// + /// - `Ok(true)` if the local and remote `.crate` files have differences, or if the package + /// version does not exist on crates.io (e.g., a 403 Forbidden error is received). + /// - `Ok(false)` if the local and remote packages are identical. /// - /// Panics if the package is not loaded or local package is not packed. /// # Errors - /// qqq: doc - pub fn publish_need( package : &Package< '_ >, path : Option< path::PathBuf > ) -> Result< bool, PackageError > + /// + /// This function will return an error in the following cases: + /// + /// - `PackageError::LocalPath`: If the path to the local `.crate` file cannot be determined. + /// - `PackageError::ReadArchive`: If the local `.crate` file exists but cannot be read. + /// - `PackageError::LoadRemotePackage`: If downloading the remote package fails for reasons + /// other than a non-existent version (e.g., network issues). + /// - Any error that occurs while trying to read the package's name or version. + pub fn publish_need( package : &Package< '_ >, path : Option< path::PathBuf >, target_dir : &std::path::Path ) -> Result< bool, PackageError > { let name = package.name()?; let version = package.version()?; let local_package_path = path .map( | p | p.join( format!( "package/{name}-{version}.crate" ) ) ) - .unwrap_or( packed_crate::local_path( name, &version, package.crate_dir() ).map_err( | _ | PackageError::LocalPath )? ); + .unwrap_or( packed_crate::local_path( name, &version, target_dir ).map_err( | _ | PackageError::LocalPath )? ); let local_package = CrateArchive::read( local_package_path ).map_err( | _ | PackageError::ReadArchive )?; let remote_package = match CrateArchive::download_crates_io( name, version ) diff --git a/module/move/willbe/src/entity/packed_crate.rs b/module/move/willbe/src/entity/packed_crate.rs index 1cb55a3ac1..1a98f7af61 100644 --- a/module/move/willbe/src/entity/packed_crate.rs +++ b/module/move/willbe/src/entity/packed_crate.rs @@ -13,29 +13,34 @@ mod private use error::{ untyped::Context }; use ureq::Agent; - /// Returns the local path of a packed `.crate` file based on its name, version, and manifest path. + /// Constructs the expected local path for a packed `.crate` file within a target directory. /// - /// # Args : - /// - `name` - the name of the package. - /// - `version` - the version of the package. - /// - `manifest_file` - path to the package `Cargo.toml` file. + /// This is a utility function that builds a predictable path without verifying + /// if the file actually exists. It follows the standard Cargo packaging structure. /// - /// # Returns : - /// The local packed `.crate` file of the package + /// # Arguments + /// + /// - `name` - The name of the package. + /// - `version` - The version of the package. + /// - `target_dir` - The path to the workspace's `target` directory, inside which + /// the `package/` subdirectory is expected. + /// + /// # Returns + /// + /// Returns a `Result` containing a `PathBuf` that points to the expected location of the `.crate` file, + /// for example: `/package/my_package-0.1.0.crate`. /// /// # Errors - /// qqq: doc + /// + /// This function is currently infallible as it only performs path joining and string formatting. + /// The `Result` is kept for API consistency. // qqq : typed error - pub fn local_path< 'a >( name : &'a str, version : &'a str, crate_dir : CrateDir ) -> error::untyped::Result< PathBuf > + pub fn local_path< 'a >( name : &'a str, version : &'a str, target_dir : &std::path::Path ) -> error::untyped::Result< PathBuf > { let buf = format!( "package/{name}-{version}.crate" ); - let workspace = Workspace::try_from( crate_dir )?; - - let mut local_package_path = PathBuf::new(); - local_package_path.push( workspace.target_directory() ); - local_package_path.push( buf ); - + let local_package_path = target_dir.join( buf ); Ok( local_package_path ) + } /// diff --git a/module/move/willbe/src/entity/publish.rs b/module/move/willbe/src/entity/publish.rs index a34d8e0195..c21a6cbdcb 100644 --- a/module/move/willbe/src/entity/publish.rs +++ b/module/move/willbe/src/entity/publish.rs @@ -390,7 +390,7 @@ mod private }; report.add = git.add; report.commit = git.commit; - report.publish = match cargo::publish( publish ) + report.publish = match cargo::publish( &publish ) { Ok( publish ) => Some( publish ), Err( e ) => diff --git a/module/move/willbe/src/tool/cargo.rs b/module/move/willbe/src/tool/cargo.rs index 6d0f687c40..5cfe81fef1 100644 --- a/module/move/willbe/src/tool/cargo.rs +++ b/module/move/willbe/src/tool/cargo.rs @@ -75,8 +75,14 @@ mod private #[ allow( clippy::if_not_else ) ] fn to_pack_args( &self ) -> Vec< String > { + // Building the full path to Cargo.toml + let manifest_path = self.path.join( "Cargo.toml" ); + let normalized_manifest_path = manifest_path.to_string_lossy().replace( '\\', "/" ); [ "run".to_string(), self.channel.to_string(), "cargo".into(), "package".into() ] .into_iter() + // clearly show the way to the manifesto + .chain( Some( "--manifest-path".to_string() ) ) + .chain( Some( normalized_manifest_path ) ) .chain( if self.allow_dirty { Some( "--allow-dirty".to_string() ) } else { None } ) .chain( if !self.checking_consistency { Some( "--no-verify".to_string() ) } else { None } ) .chain( self.temp_path.clone().map( | p | vec![ "--target-dir".to_string(), p.to_string_lossy().into() ] ).into_iter().flatten() ) @@ -92,6 +98,11 @@ mod private /// - `path` - path to the package directory /// - `dry` - a flag that indicates whether to execute the command or not /// + // FIX: Added # Errors section for `pack` function + /// # Errors + /// + /// Returns an error if the `rustup ... cargo package` command fails. + /// #[ cfg_attr ( feature = "tracing", @@ -159,14 +170,19 @@ mod private } } - /// Upload a package to the registry + /// Upload a package to the registry + // FIX: Added # Errors section for `publish` function + /// # Errors + /// + /// Returns an error if the `cargo publish` command fails after all retry attempts. + /// #[ cfg_attr ( feature = "tracing", track_caller, tracing::instrument( fields( caller = ?{ let x = std::panic::Location::caller(); ( x.file(), x.line() ) } ) ) )] - pub fn publish( args : PublishOptions ) -> error::untyped::Result< process::Report > + pub fn publish( args : &PublishOptions ) -> error::untyped::Result< process::Report > // qqq : use typed error { diff --git a/module/move/willbe/src/tool/git.rs b/module/move/willbe/src/tool/git.rs index b9f2761a58..acd0dfda8c 100644 --- a/module/move/willbe/src/tool/git.rs +++ b/module/move/willbe/src/tool/git.rs @@ -24,6 +24,9 @@ mod private /// /// # Returns : /// Returns a result containing a report indicating the result of the operation. + /// # Errors + /// + /// Returns an error if the `git add` command fails. // qqq : should be typed error, apply err_with #[ cfg_attr ( @@ -79,6 +82,9 @@ mod private /// /// # Returns : /// Returns a result containing a report indicating the result of the operation. + /// # Errors + /// + /// Returns an error if the `git commit` command fails. // qqq : should be typed error, apply err_with #[ cfg_attr ( @@ -132,7 +138,9 @@ mod private /// /// # Returns : /// Returns a result containing a report indicating the result of the operation. - + /// # Errors + /// + /// Returns an error if the `git push` command fails. // qqq : should be typed error, apply err_with #[ cfg_attr( feature = "tracing", tracing::instrument( skip( path ), fields( path = %path.as_ref().display() ) ) ) ] diff --git a/module/move/willbe/src/tool/graph.rs b/module/move/willbe/src/tool/graph.rs index e0ba9bc109..cac8812f77 100644 --- a/module/move/willbe/src/tool/graph.rs +++ b/module/move/willbe/src/tool/graph.rs @@ -248,27 +248,49 @@ mod private subgraph } - /// Removes nodes that are not required to be published from the graph. + /// Filters a dependency graph to retain only the packages that require publishing. + /// + /// This function traverses the dependency graph starting from the specified `roots`. + /// For each package, it determines if a new version needs to be published by + /// packaging it locally (`cargo pack`) and comparing it with the latest version on + /// crates.io using the `publish_need` function. + /// + /// A package is retained in the final graph if: + /// 1. It has changed since its last publication. + /// 2. One of its dependencies requires publishing (thus forcing a version bump). + /// + /// This helps in creating a minimal publish plan, avoiding unnecessary publications + /// of packages that have not changed. /// /// # Arguments /// - /// * `package_map` - A reference to a `HashMap` mapping `String` keys to `Package` values. - /// * `graph` - A reference to a `Graph` of nodes and edges, where nodes are of type `String` and edges are of type `String`. - /// * `roots` - A slice of `String` representing the root nodes of the graph. + /// * `workspace` - The workspace context, used to locate the `target` directory for packaging. + /// * `package_map` - A map from package names to `Package` details, used for quick lookups. + /// * `graph` - The complete dependency graph of the workspace packages. + /// * `roots` - A slice of package names that serve as the starting points for the analysis. + /// * `temp_path` - An optional path to a temporary directory for `cargo pack` to use, + /// preventing interference between parallel runs. /// /// # Returns /// - /// A new `Graph` with the nodes that are not required to be published removed. + /// A `Result` containing a new, filtered `Graph` with only the packages that need + /// to be published and their inter-dependencies. /// /// # Errors - /// qqq: doc + /// + /// Returns an `Err` if the `cargo::pack` command fails for any of the packages during the check. /// /// # Panics - /// qqq: doc + /// + /// This function will panic if: + /// - A package name from the graph cannot be found in the `package_map`. + /// - The graph is inconsistent and a node index is invalid. + /// - The `publish_need` check panics (e.g., due to network issues). // qqq : for Bohdan : typed error #[ allow( clippy::single_match, clippy::needless_pass_by_value, clippy::implicit_hasher ) ] pub fn remove_not_required_to_publish ( + workspace : &Workspace, package_map : &HashMap< String, Package< '_ > >, graph : &Graph< String, String >, roots : &[ String ], @@ -299,13 +321,13 @@ mod private _ = cargo::pack ( cargo::PackOptions::former() - .path( package.crate_dir().absolute_path() ) + .path( package.crate_dir().absolute_path().inner() ) .option_temp_path( temp_path.clone() ) .dry( false ) .allow_dirty( true ) .form() )?; - if publish_need( package, temp_path.clone() ).unwrap() + if publish_need( package, temp_path.clone(), workspace.target_directory() ).unwrap() { nodes.insert( n ); } diff --git a/module/move/willbe/tests/inc/package.rs b/module/move/willbe/tests/inc/package.rs index 0a2a07dd05..5de21d8aac 100644 --- a/module/move/willbe/tests/inc/package.rs +++ b/module/move/willbe/tests/inc/package.rs @@ -1,6 +1,6 @@ use std::*; use std::io::Write; - +use assert_fs::TempDir; use crate::the_module::{ action, channel, package }; enum Dependency @@ -235,10 +235,9 @@ impl Drop for TestWorkspace #[ test ] fn kos_plan() { - let tmp_folder = env::temp_dir().join( "publish_plan_kos_plan" ); - _ = fs::remove_dir_all( &tmp_folder ).ok(); + let temp = TempDir::new().unwrap(); - let workspace = TestWorkspace::new( tmp_folder ).unwrap() + let workspace = TestWorkspace::new( temp.path() ).unwrap() .with_packages( [ TestPackage::new( "a" ), @@ -256,7 +255,7 @@ fn kos_plan() let plan = action::publish_plan ( - the_patterns, + &the_patterns, channel::Channel::Stable, false, false,