From c2dae4f3f953ff00af714ad7a6ece326171d557f Mon Sep 17 00:00:00 2001 From: Valentin Haudiquet Date: Fri, 18 Sep 2026 20:51:36 +0200 Subject: [PATCH] lift main.rs business logic into the library MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chlog target-series resolution becomes changelog::series_candidates (UNRELEASED pinning, development-series default and fallbacks modeled by SeriesCandidates), PPA references get package_info::split_ppa (shared by pull and deb, now also rejecting empty parts), and SSH endpoints get context::ContextConfig::from_endpoint — so a library consumer can resolve series, validate PPAs and build context configurations without reimplementing the CLI's rules. All three carry unit tests; main.rs keeps only parsing of flags and error handling. --- src/changelog.rs | 148 ++++++++++++++++++++++++++++++++++++++++++++ src/context/api.rs | 76 +++++++++++++++++++++++ src/deb/local.rs | 77 +++++++++++------------ src/main.rs | 109 ++++++++++---------------------- src/package_info.rs | 35 +++++++++++ 5 files changed, 325 insertions(+), 120 deletions(-) diff --git a/src/changelog.rs b/src/changelog.rs index c631d13..ee17b58 100644 --- a/src/changelog.rs +++ b/src/changelog.rs @@ -139,6 +139,78 @@ pub fn parse_changelog_header( Ok((entry.source, entry.version.full(), entry.distribution)) } +/// What a new changelog entry may target as series, derived from the +/// current changelog ([`series_candidates`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum SeriesCandidates { + /// Offer `options` with `default` preselected; when the selection + /// cannot be made (cancelled, no interactive user) `fallback` — the + /// changelog's current series — is used instead. + Choose { + /// Series names to offer. + options: Vec, + /// Preselected series. + default: String, + /// Series to fall back to when nothing can be selected. + fallback: String, + }, + /// Nothing to choose: the series list was unavailable, keep the + /// current series. + Keep(String), +} + +/// Derive the candidate series for a new changelog entry from the changelog +/// at `changelog_path`. +/// +/// An UNRELEASED entry offers itself as a pinned first option (selecting it +/// keeps the changelog unreleased) on top of the current vendor's series +/// list, defaulting to the development series; any other series resolves +/// through the series list of its own distribution. `None` when the +/// changelog cannot be parsed (no default to derive at all). +pub async fn series_candidates(changelog_path: &Path) -> Option { + let (_package, _version, current) = parse_changelog_header(changelog_path).ok()?; + + if crate::distro_info::is_unreleased(¤t) { + // Vendors keep original casing ("Ubuntu"), while the series data + // keys are lowercase + let dist = crate::build::env::current_vendor().to_lowercase(); + let mut options = vec![crate::distro_info::UNRELEASED.to_string()]; + match crate::distro_info::get_ordered_series_name(&dist).await { + Ok(series_list) => { + options.extend(series_list); + // Default to the development series (the first real entry), + // not to the pinned UNRELEASED entry itself + let default = if options.len() > 1 { + options[1].clone() + } else { + current.clone() + }; + Some(SeriesCandidates::Choose { + options, + default, + fallback: current, + }) + } + Err(_) => Some(SeriesCandidates::Keep(current)), + } + } else { + match crate::distro_info::get_dist_from_series(¤t).await { + Ok(dist) => { + match crate::distro_info::get_ordered_series_name(&dist).await { + Ok(options) if !options.is_empty() => Some(SeriesCandidates::Choose { + options, + default: current.clone(), + fallback: current, + }), + // An empty list offers nothing to choose from + _ => Some(SeriesCandidates::Keep(current)), + } + } + Err(_) => Some(SeriesCandidates::Keep(current)), + } + } +} + /// Parse a changelog file footer to extract maintainer information /// Returns (name, email) tuple from the last modification entry pub fn parse_changelog_footer(path: &Path) -> Result<(String, String), Box> { @@ -306,6 +378,82 @@ mod tests { use std::process::Command; use tempfile::TempDir; + /// An UNRELEASED changelog offers UNRELEASED pinned first, the + /// development series as the default, and itself as the fallback. + #[tokio::test] + async fn series_candidates_unreleased_pins_entry_and_defaults_to_dev() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("changelog"); + std::fs::write( + &path, + "hello (1.0-1) UNRELEASED; urgency=medium\n\n * Something.\n\n \ + -- A B Mon, 01 Jan 2024 00:00:00 +0000\n", + ) + .unwrap(); + + match series_candidates(&path).await { + Some(SeriesCandidates::Choose { + options, + default, + fallback, + }) => { + assert_eq!(options[0], "UNRELEASED"); + assert!(options.len() > 1, "the vendor series list is offered"); + assert_eq!(default, options[1]); + assert_eq!(fallback, "UNRELEASED"); + } + other => panic!("expected Choose, got {other:?}"), + } + } + + /// A released changelog offers its distribution's series with the + /// current one preselected. Uses a series of the host vendor so the + /// test only relies on the local distro-info data. + #[tokio::test] + async fn series_candidates_released_defaults_to_current_series() { + let dist = crate::build::env::current_vendor().to_lowercase(); + let vendor_series = crate::distro_info::get_ordered_series_name(&dist) + .await + .expect("the host vendor's series data resolves"); + // Any released series of the vendor works; the changelog names it. + let current = vendor_series.last().expect("non-empty series list"); + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("changelog"); + std::fs::write( + &path, + format!( + "hello (1.0-1) {current}; urgency=medium\n\n * Something.\n\n \ + -- A B Mon, 01 Jan 2024 00:00:00 +0000\n" + ), + ) + .unwrap(); + + match series_candidates(&path).await { + Some(SeriesCandidates::Choose { + options, + default, + fallback, + }) => { + assert_eq!(options, vendor_series); + assert_eq!(default, *current); + assert_eq!(fallback, *current); + } + other => panic!("expected Choose, got {other:?}"), + } + } + + /// Without a parsable changelog there is no candidate at all. + #[tokio::test] + async fn series_candidates_none_without_changelog() { + let dir = tempfile::tempdir().unwrap(); + assert!( + series_candidates(&dir.path().join("changelog")) + .await + .is_none() + ); + } + fn setup_repo(dir: &Path) { Command::new("git") .arg("init") diff --git a/src/context/api.rs b/src/context/api.rs index f2e5970..3e10f71 100644 --- a/src/context/api.rs +++ b/src/context/api.rs @@ -117,6 +117,28 @@ pub enum ContextConfig { }, } +impl ContextConfig { + /// Build an SSH context configuration from an endpoint of the form + /// `[ssh://][user@]host[:port]`. + pub fn from_endpoint(endpoint: &str) -> Result { + let re = regex::Regex::new( + r"^(?:ssh://)?(?:(?P[^@]+)@)?(?P[^:/]+)(?::(?P\d+))?$", + ) + .expect("valid endpoint regex"); + let cap = re.captures(endpoint).ok_or_else(|| { + format!("Invalid endpoint format: '{endpoint}'. Expected [ssh://][user@]host[:port]") + })?; + let host = cap.name("host").unwrap().as_str().to_string(); + let user = cap.name("user").map(|m| m.as_str().to_string()); + let port = cap + .name("port") + .map(|m| m.as_str().parse::()) + .transpose() + .map_err(|_| "Invalid port number".to_string())?; + Ok(ContextConfig::Ssh { host, user, port }) + } +} + /// A context, allowing to run commands, read and write files, etc pub struct Context { /// Configuration for the context @@ -455,3 +477,57 @@ fn contextualize_spawn_error(program: &str, e: io::Error) -> io::Error { io::Error::new(e.kind(), format!("Could not run '{program}': {e}")) } } + +#[cfg(test)] +mod endpoint_tests { + use super::*; + + /// Every accepted endpoint spelling maps to the expected config. + #[test] + fn from_endpoint_parses_all_spellings() { + assert_eq!( + ContextConfig::from_endpoint("myhost"), + Ok(ContextConfig::Ssh { + host: "myhost".into(), + user: None, + port: None, + }) + ); + assert_eq!( + ContextConfig::from_endpoint("admin@myhost"), + Ok(ContextConfig::Ssh { + host: "myhost".into(), + user: Some("admin".into()), + port: None, + }) + ); + assert_eq!( + ContextConfig::from_endpoint("myhost:2222"), + Ok(ContextConfig::Ssh { + host: "myhost".into(), + user: None, + port: Some(2222), + }) + ); + assert_eq!( + ContextConfig::from_endpoint("ssh://admin@myhost:22"), + Ok(ContextConfig::Ssh { + host: "myhost".into(), + user: Some("admin".into()), + port: Some(22), + }) + ); + } + + /// Non-numeric ports and extra segments are format errors; a + /// non-u16 numeric port is a port error. + #[test] + fn from_endpoint_rejects_malformed_endpoints() { + for bad in ["", "a/b/c", "host:notaport"] { + let err = ContextConfig::from_endpoint(bad).unwrap_err(); + assert!(err.contains("Invalid endpoint format"), "{err}"); + } + let err = ContextConfig::from_endpoint("host:99999").unwrap_err(); + assert_eq!(err, "Invalid port number"); + } +} diff --git a/src/deb/local.rs b/src/deb/local.rs index e81f52b..296b9a3 100644 --- a/src/deb/local.rs +++ b/src/deb/local.rs @@ -84,51 +84,44 @@ pub async fn build( // Add PPA repositories if specified for ppa_str in ppa { - // PPA format: user/ppa_name - let parts: Vec<&str> = ppa_str.split('/').collect(); - if parts.len() == 2 { - let base_url = crate::package_info::ppa_to_base_url(parts[0], parts[1]); + let (ppa_user, ppa_name) = crate::package_info::split_ppa(ppa_str)?; + let base_url = crate::package_info::ppa_to_base_url(ppa_user, ppa_name); - // Add new PPA source if not found - if !sources.iter().any(|s| s.uri.contains(&base_url)) { - // Get host and target architectures - let host_arch = crate::get_current_arch(); - let target_arch = arch; + // Add new PPA source if not found + if !sources.iter().any(|s| s.uri.contains(&base_url)) { + // Get host and target architectures + let host_arch = crate::get_current_arch(); + let target_arch = arch; - // Create architectures list with both host and target if different - let mut architectures = vec![host_arch.clone()]; - if host_arch != *target_arch { - architectures.push(target_arch.to_string()); - } - - // Create suite list with all Ubuntu series - let suites = vec![series.to_string()]; - - let new_source = crate::apt::sources::SourceEntry { - enabled: true, - kind: crate::apt::sources::SourceKind::Deb, - components: vec!["main".to_string()], - architectures: architectures.clone(), - signed_by: None, - trusted: None, - suite: suites, - uri: base_url, - // No origin: saved to the pkh-owned added-sources file - origin: None, - }; - sources.push(new_source); - modified = true; - added_ppas.push((parts[0], parts[1])); - log::info!( - "Added PPA: {} for series {} with architectures {:?}", - ppa_str, - series, - architectures - ); + // Create architectures list with both host and target if different + let mut architectures = vec![host_arch.clone()]; + if host_arch != *target_arch { + architectures.push(target_arch.to_string()); } - } else { - return Err( - format!("Invalid PPA format: '{}'. Expected: user/ppa_name", ppa_str).into(), + + // Create suite list with all Ubuntu series + let suites = vec![series.to_string()]; + + let new_source = crate::apt::sources::SourceEntry { + enabled: true, + kind: crate::apt::sources::SourceKind::Deb, + components: vec!["main".to_string()], + architectures: architectures.clone(), + signed_by: None, + trusted: None, + suite: suites, + uri: base_url, + // No origin: saved to the pkh-owned added-sources file + origin: None, + }; + sources.push(new_source); + modified = true; + added_ppas.push((ppa_user, ppa_name)); + log::info!( + "Added PPA: {} for series {} with architectures {:?}", + ppa_str, + series, + architectures ); } } diff --git a/src/main.rs b/src/main.rs index e67ecd6..594625f 100644 --- a/src/main.rs +++ b/src/main.rs @@ -353,15 +353,14 @@ fn main() { let (pb, progress_callback) = pkh::ui::create_progress_bar(&multi); // Convert PPA to base URL if provided - let base_url = ppa.map(|ppa_str| { - // PPA format: user/ppa_name - let parts: Vec<&str> = ppa_str.split('/').collect(); - if parts.len() != 2 || parts[0].is_empty() || parts[1].is_empty() { - error!("Invalid PPA format: '{}'. Expected: user/ppa_name", ppa_str); + let base_url = match ppa.map(pkh::package_info::split_ppa) { + Some(Ok((user, name))) => Some(pkh::package_info::ppa_to_base_url(user, name)), + Some(Err(e)) => { + error!("{e}"); std::process::exit(1); } - pkh::package_info::ppa_to_base_url(parts[0], parts[1]) - }); + None => None, + }; // Since pull is async, we need to block on it if let Err(e) = rt.block_on(async { @@ -395,62 +394,27 @@ fn main() { let target_series = if let Some(s) = cli_series { Some(s.to_string()) } else { - // Parse current changelog to determine the default series let changelog_path = cwd.join("debian/changelog"); - match pkh::changelog::parse_changelog_header(&changelog_path) { - Ok((_pkg, _ver, current_series)) => { - // UNRELEASED is not a real series: offer it as a - // pinned first entry (selecting it keeps the changelog - // unreleased) on top of the current vendor's series - // list, defaulting to the development series. Any - // other series resolves through the series list of - // its own distribution. - match rt.block_on(async { - if pkh::distro_info::is_unreleased(¤t_series) { - // Vendors keep original casing ("Ubuntu"), - // while the series data keys are lowercase - let dist = pkh::build::env::current_vendor().to_lowercase(); - let mut series_list = - vec![pkh::distro_info::UNRELEASED.to_string()]; - series_list.extend( - pkh::distro_info::get_ordered_series_name(&dist).await?, - ); - Ok(series_list) - } else { - let dist = - pkh::distro_info::get_dist_from_series(¤t_series).await?; - pkh::distro_info::get_ordered_series_name(&dist).await - } - }) { - Ok(series_list) => { - // Default to the development series (the - // first real entry) when the changelog is - // UNRELEASED, not to the pinned entry itself - let default = if pkh::distro_info::is_unreleased(¤t_series) - && series_list.len() > 1 - { - series_list[1].clone() - } else { - current_series.clone() - }; - match pkh::ui::select_series(&series_list, &default) { - Ok(selected) => Some(selected), - Err(e) => { - error!( - "Series selection failed: {}. Using current series '{}' instead.", - e, current_series - ); - Some(current_series) - } - } - } - Err(_) => { - // Could not fetch series list, use current series as default - Some(current_series) - } + match rt.block_on(pkh::changelog::series_candidates(&changelog_path)) { + Some(pkh::changelog::SeriesCandidates::Choose { + options, + default, + fallback, + }) => match pkh::ui::select_series(&options, &default) { + Ok(selected) => Some(selected), + Err(e) => { + error!( + "Series selection failed: {}. Using current series '{}' instead.", + e, fallback + ); + Some(fallback) } - } - Err(_) => None, + }, + // Could not fetch the series list: use the current series + Some(pkh::changelog::SeriesCandidates::Keep(current)) => Some(current), + // No parsable changelog: leave the series decision to + // generate_entry + None => None, } }; @@ -668,24 +632,13 @@ fn main() { std::process::exit(1); }); - // Parse host, user, port from endpoint - // Formats: [ssh://][user@]host[:port] - let endpoint_re = regex::Regex::new(r"^(?:ssh://)?(?:(?P[^@]+)@)?(?P[^:/]+)(?::(?P\d+))?$").unwrap(); - let endpoint_cap = endpoint_re.captures(endpoint).unwrap_or_else(|| { - error!("Invalid endpoint format: '{}'. Expected [ssh://][user@]host[:port]", endpoint); - std::process::exit(1); - }); - - let host = endpoint_cap.name("host").unwrap().as_str().to_string(); - let user = endpoint_cap.name("user").map(|m| m.as_str().to_string()); - let port = endpoint_cap.name("port").map(|m| { - m.as_str().parse::().unwrap_or_else(|_| { - error!("Invalid port number"); + match pkh::context::ContextConfig::from_endpoint(endpoint) { + Ok(config) => config, + Err(e) => { + error!("{e}"); std::process::exit(1); - }) - }); - - ContextConfig::Ssh { host, user, port } + } + } } _ => { error!("Unknown context type: {}", type_str); diff --git a/src/package_info.rs b/src/package_info.rs index 205d05e..acf0e7d 100644 --- a/src/package_info.rs +++ b/src/package_info.rs @@ -9,6 +9,22 @@ use crate::apt::release::{self, VerifiedRelease}; use crossterm::style::Stylize; use log::{debug, warn}; +/// Split a PPA reference into its `(user, name)` parts +/// +/// A PPA is written `user/ppa_name` (e.g. `user/my-ppa`); anything else — +/// more segments, empty parts — is a format error carrying the canonical +/// message. +pub fn split_ppa(ppa: &str) -> Result<(&str, &str), String> { + let parts: Vec<&str> = ppa.split('/').collect(); + if parts.len() == 2 && !parts[0].is_empty() && !parts[1].is_empty() { + Ok((parts[0], parts[1])) + } else { + Err(format!( + "Invalid PPA format: '{ppa}'. Expected: user/ppa_name" + )) + } +} + /// Convert a PPA specification to a base URL /// /// # Arguments @@ -943,6 +959,25 @@ pub async fn lookup( mod tests { use super::*; + /// `user/ppa_name` splits into its two parts. + #[test] + fn split_ppa_parses_the_canonical_form() { + assert_eq!(split_ppa("user/my-ppa"), Ok(("user", "my-ppa"))); + } + + /// Anything but exactly two non-empty segments is rejected, with the + /// canonical message. + #[test] + fn split_ppa_rejects_malformed_references() { + for bad in ["", "user", "user/", "/ppa", "a/b/c"] { + let err = split_ppa(bad).unwrap_err(); + assert!(err.contains("Invalid PPA format"), "{err}"); + assert!(err.contains(bad), "{err}"); + } + } + + use super::*; + /// Serve canned byte responses on a local port, one per connection (the /// last response repeats), and return the base URL ///