From f334f254836310bf983e952ec105dad33c9fca8f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Loipf=C3=BChrer?= Date: Mon, 10 Aug 2026 22:25:09 +0200 Subject: [PATCH] refactor(cli): improve architectural structure - better seperate out config resolution from main function - introduce proper distro version selection with error handling from changelog --- docs/usage/build.md | 3 + packages/debmagic-common/src/distro.rs | 24 ++- packages/debmagic/src/build/mod.rs | 243 +++++---------------- packages/debmagic/src/build/source.rs | 6 +- packages/debmagic/src/build_intent.rs | 273 +++++++++++++++++++++++ packages/debmagic/src/config.rs | 3 +- packages/debmagic/src/main.rs | 186 +++++----------- packages/debmagic/src/package.rs | 286 ++++++++++++++++++++----- 8 files changed, 647 insertions(+), 377 deletions(-) create mode 100644 packages/debmagic/src/build_intent.rs diff --git a/docs/usage/build.md b/docs/usage/build.md index e25ddae..488f745 100644 --- a/docs/usage/build.md +++ b/docs/usage/build.md @@ -114,6 +114,9 @@ This flag implies `--persistent`, and cannot be combined with `--clean yes`. Only needed when `debian/changelog`'s top entry doesn't unambiguously determine the target: pass `--distro ` (e.g. `--distro noble`, `--distro trixie`). If the changelog has a single unambiguous entry, omit it. +Suite aliases in the changelog (or via `--distro`) resolve to a concrete release: Debian `stable` / `oldstable` / `sid` (→ `unstable`), and Ubuntu `devel`. +Alias targets are updated manually when Debian/Ubuntu roll. + ## Proposed dependencies If needed, build dependencies can be used from `-proposed`. diff --git a/packages/debmagic-common/src/distro.rs b/packages/debmagic-common/src/distro.rs index e4f18f6..ee39916 100644 --- a/packages/debmagic-common/src/distro.rs +++ b/packages/debmagic-common/src/distro.rs @@ -59,12 +59,17 @@ static DISTRO_INFO_MAP: LazyLock> = LazyLoc DistroVersion::new(Debian, "experimental", ""), ), ("unstable", DistroVersion::new(Debian, "unstable", "")), - ("sid", DistroVersion::new(Debian, "sid", "")), + // Suite alias: sid → unstable (concrete release identity). + ("sid", DistroVersion::new(Debian, "unstable", "")), ("testing", DistroVersion::new(Debian, "testing", "")), ("duke", DistroVersion::new(Debian, "duke", "15")), ("forky", DistroVersion::new(Debian, "forky", "14")), ("trixie", DistroVersion::new(Debian, "trixie", "13")), + // Suite alias: stable → current stable release (update when Debian rolls). + ("stable", DistroVersion::new(Debian, "trixie", "13")), ("bookworm", DistroVersion::new(Debian, "bookworm", "12")), + // Suite alias: oldstable → current oldstable release. + ("oldstable", DistroVersion::new(Debian, "bookworm", "12")), ("bullseye", DistroVersion::new(Debian, "bullseye", "11")), ("buster", DistroVersion::new(Debian, "buster", "10")), ("stretch", DistroVersion::new(Debian, "stretch", "9")), @@ -73,6 +78,11 @@ static DISTRO_INFO_MAP: LazyLock> = LazyLoc "stonking", DistroVersion::new(Ubuntu, "stonking", "26.10").devel(), ), + // Suite alias: devel → current Ubuntu development release. + ( + "devel", + DistroVersion::new(Ubuntu, "stonking", "26.10").devel(), + ), ("resolute", DistroVersion::new(Ubuntu, "resolute", "26.04")), ("noble", DistroVersion::new(Ubuntu, "noble", "24.04")), ("jammy", DistroVersion::new(Ubuntu, "jammy", "22.04")), @@ -83,6 +93,14 @@ static DISTRO_INFO_MAP: LazyLock> = LazyLoc ]) }); -pub fn get_distro_version(codename: &str) -> Option { - DISTRO_INFO_MAP.get(codename).cloned() +/// Look up a distribution by codename or suite alias. +/// +/// Suite aliases are map keys that resolve to a concrete release [`DistroVersion`]: +/// - Debian: `stable` → current stable release, `oldstable` → current oldstable, +/// `sid` → `unstable` +/// - Ubuntu: `devel` → current development release +/// +/// Alias targets are maintained manually when Debian/Ubuntu roll. +pub fn get_distro_version(name: &str) -> Option { + DISTRO_INFO_MAP.get(name).cloned() } diff --git a/packages/debmagic/src/build/mod.rs b/packages/debmagic/src/build/mod.rs index f7f92ae..8dbf94c 100644 --- a/packages/debmagic/src/build/mod.rs +++ b/packages/debmagic/src/build/mod.rs @@ -9,6 +9,7 @@ use std::{ use crate::build::attach::{send_socket_command, start_socket_server}; use crate::build::config::DriverOverrides; use crate::build::source::{source_manifest_path, stage_source_tree}; +use crate::build_intent::BuildIntent; use crate::{ build::{ common::{BuildConfig, BuildDriver, BuildDriverType, BuildMetadata}, @@ -18,10 +19,9 @@ use crate::{ driver_lxd::{DriverLxd, LxdVariant}, }, config::Config, - package::PackageDescription, + package::{PackageIdentity, PackageTarget}, }; use anyhow::{Context, anyhow}; -use debmagic_common::distro::DistroVersion; pub mod artifacts; pub mod attach; @@ -252,95 +252,46 @@ impl Build { } fn get_build_root_and_identifier( - config: &Config, - package: &PackageDescription, + temp_build_dir: &Path, + identity: &PackageIdentity, ) -> (String, PathBuf) { - let package_identifier = format!("{}-{}", package.name, package.version); - let build_root = config.temp_build_dir.join(&package_identifier); + let package_identifier = format!("{}-{}", identity.name, identity.version); + let build_root = temp_build_dir.join(&package_identifier); (package_identifier, build_root) } -/// Determine which distro version to use for the build. -/// -/// If only one distro version is specified in the changelog, it's used automatically. -/// If multiple distro versions are specified, an explicit --distro is required. -/// If --distro is provided, it's validated against the changelog versions. -fn resolve_distro_version( - changelog_distros: &[String], - explicit_distro: Option<&str>, -) -> anyhow::Result { - let resolved_codename = match (changelog_distros.len(), explicit_distro) { - (0, _) => Err(anyhow!("changelog contains no distributions")), - (1, None) => Ok(changelog_distros[0].clone()), - (1, Some(explicit)) => { - if explicit == changelog_distros[0] { - Ok(explicit.to_string()) - } else { - Err(anyhow!( - "explicit distro version '{}' conflicts with distribution specified in changelog '{}'", - explicit, - changelog_distros[0] - )) - } - } - (_, None) => Err(anyhow!( - "changelog contains multiple distributions ({}), please specify which one to build for with --distro", - changelog_distros.join(", ") - )), - (_, Some(explicit)) => { - if changelog_distros.contains(&explicit.to_string()) { - Ok(explicit.to_string()) - } else { - Err(anyhow!( - "explicit distro version '{}' not found in changelog distributions: {}", - explicit, - changelog_distros.join(", ") - )) - } - } - }?; - let resolved = debmagic_common::distro::get_distro_version(&resolved_codename) - .ok_or_else(|| anyhow!("unknown distro codename '{}'", resolved_codename))?; - Ok(resolved) -} - -fn prepare_build_env( - config: &Config, - driver_overrides: &DriverOverrides, - package: &PackageDescription, - driver_type: BuildDriverType, - output_dir: &Path, - explicit_distro_version: Option<&str>, -) -> anyhow::Result { - let (package_identifier, build_root) = get_build_root_and_identifier(config, package); - - let distro_version = resolve_distro_version(&package.distro_versions, explicit_distro_version) - .context("failed to determine distro version")?; +fn prepare_build_env(intent: &BuildIntent, target: &PackageTarget) -> anyhow::Result { + let (package_identifier, build_root) = + get_build_root_and_identifier(&intent.config.temp_build_dir, &target.identity); let build_config = BuildConfig { - driver: driver_type, - package_name: package.name.clone(), + driver: intent.driver, + package_name: target.identity.name.clone(), package_identifier, - source_dir: package.source_dir.clone(), - output_dir: output_dir.to_path_buf(), + source_dir: target.identity.source_dir.clone(), + output_dir: intent.output_dir.clone(), build_root_dir: build_root.clone(), - distro: distro_version.clone(), - sign_package: config.sign_package, - sign_with: config.sign_with, - sign_key: config.sign_key.clone(), - build_debug_symbols: config.build_debug_symbols, - clean: config.clean, - persistent: config.driver.persistent, - incremental: config.incremental, - source_sync_mode: config.source_sync_mode, + distro: target.distro.clone(), + sign_package: intent.config.sign_package, + sign_with: intent.config.sign_with, + sign_key: intent.config.sign_key.clone(), + build_debug_symbols: intent.config.build_debug_symbols, + clean: intent.config.clean, + persistent: intent.config.driver.persistent, + incremental: intent.config.incremental, + source_sync_mode: intent.config.source_sync_mode, }; - if config.driver.persistent && build_root.exists() { + if intent.config.driver.persistent && build_root.exists() { // For persistent containers, starting first lets root inside delete // container-owned files the host user can't remove. - let build = Build::create(&build_config, &config.driver, driver_overrides) - .context(format!("failed to create {:?} build driver", driver_type))?; - if !config.incremental + let build = Build::create( + &build_config, + &intent.config.driver, + &intent.driver_overrides, + ) + .context(format!("failed to create {:?} build driver", intent.driver))?; + if !intent.config.incremental || !source_manifest_path(&build_config).is_file() || !build.driver.reused_environment() { @@ -352,7 +303,7 @@ fn prepare_build_env( build_config .create_dirs() .context("failed to create build directories")?; - stage_source_tree(&build_config, package)?; + stage_source_tree(&build_config, &target.identity)?; return Ok(build); } @@ -368,7 +319,7 @@ fn prepare_build_env( && let Ok(file) = fs::OpenOptions::new().read(true).open(&metadata_path) && let Ok(metadata) = serde_json::from_reader::<_, BuildMetadata>(BufReader::new(&file)) - && let Ok(driver) = create_driver_from_metadata(&config.driver, &metadata) + && let Ok(driver) = create_driver_from_metadata(&intent.config.driver, &metadata) { let _ = driver.reset_build_root(); } @@ -388,14 +339,19 @@ fn prepare_build_env( .create_dirs() .context("failed to create build directories")?; - stage_source_tree(&build_config, package)?; + stage_source_tree(&build_config, &target.identity)?; - let build = Build::create(&build_config, &config.driver, driver_overrides)?; + let build = Build::create( + &build_config, + &intent.config.driver, + &intent.driver_overrides, + )?; Ok(build) } -pub fn get_shell_in_build(config: &Config, package: &PackageDescription) -> anyhow::Result<()> { - let (_package_identifier, build_root) = get_build_root_and_identifier(config, package); +pub fn get_shell_in_build(config: &Config, identity: &PackageIdentity) -> anyhow::Result<()> { + let (_package_identifier, build_root) = + get_build_root_and_identifier(&config.temp_build_dir, identity); let build = Build::from_build_root(&build_root, &config.driver)?; let result = build .driver @@ -421,13 +377,9 @@ fn deb_build_options(existing: Option<&str>, build_debug_symbols: bool) -> Strin /// Everything needed to run one package build, independent of whether the /// build produces binary or source packages. -pub struct BuildRequest<'a> { - pub config: &'a Config, - pub package: &'a PackageDescription, - pub driver_type: BuildDriverType, - pub driver_overrides: &'a DriverOverrides, - pub output_dir: &'a Path, - pub explicit_distro_version: Option<&'a str>, +struct BuildRequest<'a> { + intent: &'a BuildIntent, + target: &'a PackageTarget, } /// Shared build orchestration: prepare the environment, run `build_commands` @@ -440,15 +392,8 @@ fn run_build( shell_on_failure: bool, build_commands: impl FnOnce(&Build) -> anyhow::Result<()>, ) -> anyhow::Result<()> { - let build = prepare_build_env( - request.config, - request.driver_overrides, - request.package, - request.driver_type, - request.output_dir, - request.explicit_distro_version, - ) - .context("failed to prepare build environment")?; + let build = prepare_build_env(request.intent, request.target) + .context("failed to prepare build environment")?; build .write_metadata() .context("failed to write build metadata")?; @@ -505,8 +450,9 @@ fn run_build( Ok(()) } -pub fn build_package(request: &BuildRequest) -> anyhow::Result<()> { - run_build(request, true, |build| { +pub fn build_package(intent: &BuildIntent, target: &PackageTarget) -> anyhow::Result<()> { + let request = BuildRequest { intent, target }; + run_build(&request, true, |build| { build.driver.run_command( &["apt-get", "-y", "build-dep", "."], &build.config.build_source_dir(), @@ -563,12 +509,13 @@ fn check_dpkg_buildpackage_available() -> anyhow::Result<()> { /// /// If `config.clean` is set, build-dependencies are installed before /// `dpkg-buildpackage` runs `debian/rules clean` once. -pub fn build_source_package(request: &BuildRequest) -> anyhow::Result<()> { - if request.driver_type == BuildDriverType::Bare { +pub fn build_source_package(intent: &BuildIntent, target: &PackageTarget) -> anyhow::Result<()> { + if intent.driver == BuildDriverType::Bare { check_dpkg_buildpackage_available()?; } - run_build(request, false, |build| { + let request = BuildRequest { intent, target }; + run_build(&request, false, |build| { let build_source_dir = build.config.build_source_dir(); if build.config.clean { build.driver.run_command( @@ -589,8 +536,6 @@ pub fn build_source_package(request: &BuildRequest) -> anyhow::Result<()> { #[cfg(test)] mod tests { - use debmagic_common::distro::Distro; - use super::*; #[test] @@ -604,86 +549,4 @@ mod tests { "nocheck parallel=8" ); } - - #[test] - fn test_resolve_distro_version_single_distro_no_explicit() { - let distros = vec!["forky".to_string()]; - let result = resolve_distro_version(&distros, None); - assert!(result.is_ok()); - let distro_version = result.unwrap(); - assert_eq!(distro_version.codename, "forky"); - assert_eq!(distro_version.distro, Distro::Debian); - } - - #[test] - fn test_resolve_distro_version_single_distro_matching_explicit() { - let distros = vec!["forky".to_string()]; - let result = resolve_distro_version(&distros, Some("forky")); - assert!(result.is_ok()); - let distro_version = result.unwrap(); - assert_eq!(distro_version.codename, "forky"); - assert_eq!(distro_version.distro, Distro::Debian); - } - - #[test] - fn test_resolve_distro_version_single_distro_conflicting_explicit() { - let distros = vec!["forky".to_string()]; - let result = resolve_distro_version(&distros, Some("duke")); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("conflicts with distribution specified in changelog") - ); - } - - #[test] - fn test_resolve_distro_version_multiple_distros_no_explicit() { - let distros = vec!["forky".to_string(), "duke".to_string()]; - let result = resolve_distro_version(&distros, None); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("multiple distributions") - ); - } - - #[test] - fn test_resolve_distro_version_multiple_distros_explicit_valid() { - let distros = vec!["forky".to_string(), "duke".to_string()]; - let result = resolve_distro_version(&distros, Some("duke")); - assert!(result.is_ok()); - let distro_version = result.unwrap(); - assert_eq!(distro_version.codename, "duke"); - assert_eq!(distro_version.distro, Distro::Debian); - } - - #[test] - fn test_resolve_distro_version_multiple_distros_explicit_invalid() { - let distros = vec!["forky".to_string(), "duke".to_string()]; - let result = resolve_distro_version(&distros, Some("trixie")); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("not found in changelog distributions") - ); - } - - #[test] - fn test_resolve_distro_version_empty_distros() { - let distros: Vec = vec![]; - let result = resolve_distro_version(&distros, None); - assert!(result.is_err()); - assert!( - result - .unwrap_err() - .to_string() - .contains("changelog contains no distributions") - ); - } } diff --git a/packages/debmagic/src/build/source.rs b/packages/debmagic/src/build/source.rs index 92f6be8..c1e5eba 100644 --- a/packages/debmagic/src/build/source.rs +++ b/packages/debmagic/src/build/source.rs @@ -17,7 +17,7 @@ use anyhow::{Context, anyhow, bail}; use glob::glob; use crate::build::common::{BuildConfig, SourceSyncMode}; -use crate::package::PackageDescription; +use crate::package::PackageIdentity; /// Paths of files tracked by git in `src`, as reported by `git ls-files`. /// Returns `None` if `src` is not inside a git worktree. @@ -375,7 +375,7 @@ fn sync_source_tree(build_config: &BuildConfig) -> anyhow::Result<()> { pub fn stage_source_tree( build_config: &BuildConfig, - package: &PackageDescription, + identity: &PackageIdentity, ) -> anyhow::Result<()> { if build_config.source_sync_mode == SourceSyncMode::Tracked { let untracked = git_untracked_paths(&build_config.source_dir); @@ -410,7 +410,7 @@ pub fn stage_source_tree( .source_dir .parent() .ok_or_else(|| anyhow!("source directory has no parent"))?; - let prefix = format!("{}_{}", package.name, package.version.upstream_version()); + let prefix = format!("{}_{}", identity.name, identity.version.upstream_version()); copy_glob( source_parent, &format!("{prefix}.orig.tar.*"), diff --git a/packages/debmagic/src/build_intent.rs b/packages/debmagic/src/build_intent.rs new file mode 100644 index 0000000..b447abf --- /dev/null +++ b/packages/debmagic/src/build_intent.rs @@ -0,0 +1,273 @@ +use std::path::{Path, PathBuf}; + +use anyhow::Context; + +use crate::{ + build::{ + common::{BuildDriverType, SourceSyncMode}, + config::DriverOverrides, + signing::SignWith, + }, + config::Config, +}; + +/// Clap-free inputs for resolving a [`BuildIntent`]. +#[derive(Debug, Clone)] +pub struct BuildIntentInput { + /// Directory used when `source_dir` / `output_dir` are unset (typically cwd). + pub fallback_dir: PathBuf, + pub source_dir: Option, + pub output_dir: Option, + pub config_file: Option, + pub driver: BuildDriverType, + pub persistent: Option, + pub incremental: Option, + /// Force incremental off (e.g. source-only builds). + pub disable_incremental: bool, + pub debug_symbols: Option, + pub sign: Option, + pub no_sign: Option, + pub sign_with: Option, + pub sign_key: Option, + pub clean: Option, + pub no_clean: Option, + pub source_sync: Option, + pub driver_overrides: DriverOverrides, +} + +/// Fully resolved description of *how* a package build should run. +/// +/// Does not include *what* is being built (package identity or target distro). +#[derive(Debug, Clone)] +pub struct BuildIntent { + pub source_dir: PathBuf, + pub output_dir: PathBuf, + pub driver: BuildDriverType, + pub config: Config, + pub driver_overrides: DriverOverrides, +} + +/// Precedence of config files is: +/// +/// 1. explicit config file passed on the command line +/// 2. `/debian/debmagic.toml` +/// 3. `/debmagic/config.toml` +pub fn load_config( + source_dir: Option<&Path>, + config_file: Option<&Path>, +) -> anyhow::Result { + let mut config_file_paths = vec![]; + let xdg_config_file = dirs::config_dir().map(|p| p.join("debmagic").join("config.toml")); + if let Some(xdg_config_file) = xdg_config_file + && xdg_config_file.is_file() + { + config_file_paths.push(xdg_config_file); + } + + if let Some(source_dir) = source_dir { + config_file_paths.push(source_dir.join("debian").join("debmagic.toml")); + } + + if let Some(config_file) = config_file { + config_file_paths.push(config_file.to_path_buf()); + } + + Config::new(&config_file_paths) +} + +pub fn resolve_build_intent(input: BuildIntentInput) -> anyhow::Result { + let source_dir = std::path::absolute(input.source_dir.unwrap_or(input.fallback_dir.clone())) + .context("resolving source dir failed")?; + let output_dir = std::path::absolute(input.output_dir.unwrap_or(input.fallback_dir)) + .context("resolving output dir failed")?; + + let mut config = load_config(Some(&source_dir), input.config_file.as_deref())?; + + if let Some(persistent) = input.persistent { + config.driver.persistent = persistent; + } + + if input.disable_incremental { + config.incremental = false; + } else if let Some(incremental) = input.incremental { + config.incremental = incremental; + } + + if let Some(debug_symbols) = input.debug_symbols { + config.build_debug_symbols = debug_symbols; + } + if let Some(sign) = input.sign { + config.sign_package = sign; + } + if let Some(no_sign) = input.no_sign { + config.sign_package = !no_sign; + } + if let Some(sign_with) = input.sign_with { + config.sign_with = sign_with; + } + if let Some(sign_key) = input.sign_key { + config.sign_key = Some(sign_key); + } + if let Some(clean) = input.clean { + config.clean = clean; + } + if let Some(no_clean) = input.no_clean { + config.clean = !no_clean; + } + if let Some(source_sync) = input.source_sync { + config.source_sync_mode = source_sync; + } + if config.incremental { + if config.clean { + anyhow::bail!("incremental builds are incompatible with clean builds"); + } + config.driver.persistent = true; + } + + Ok(BuildIntent { + source_dir, + output_dir, + driver: input.driver, + config, + driver_overrides: input.driver_overrides, + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::build::{ + driver_bare::DriverBareConfigOverrides, driver_docker::DriverDockerConfigOverrides, + driver_lxd::DriverLxdConfigOverrides, + }; + + fn asset_config() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests") + .join("assets") + .join("config1.toml") + } + + fn base_input(fallback: PathBuf) -> BuildIntentInput { + BuildIntentInput { + fallback_dir: fallback, + source_dir: None, + output_dir: None, + config_file: Some(asset_config()), + driver: BuildDriverType::Docker, + persistent: None, + incremental: None, + disable_incremental: false, + debug_symbols: None, + sign: None, + no_sign: None, + sign_with: None, + sign_key: None, + clean: None, + no_clean: None, + source_sync: None, + driver_overrides: DriverOverrides { + apt_mirror: None, + proposed: None, + docker: DriverDockerConfigOverrides { base_image: None }, + bare: DriverBareConfigOverrides {}, + lxd: DriverLxdConfigOverrides { + base_image: None, + project: None, + }, + }, + } + } + + #[test] + fn load_config_reads_explicit_file() -> anyhow::Result<()> { + let cfg = load_config(None, Some(&asset_config()))?; + assert!(cfg.driver.persistent); + assert_eq!( + cfg.driver.docker.base_images.get("debian:trixie"), + Some(&"some-debian-trixie-image:latest".to_string()) + ); + Ok(()) + } + + #[test] + fn resolve_applies_incremental_implies_persistent() -> anyhow::Result<()> { + let dir = std::env::temp_dir(); + let mut input = base_input(dir); + input.persistent = Some(false); + input.incremental = Some(true); + + let intent = resolve_build_intent(input)?; + assert!(intent.config.incremental); + assert!(intent.config.driver.persistent); + Ok(()) + } + + #[test] + fn resolve_honours_persistent_without_incremental() -> anyhow::Result<()> { + let dir = std::env::temp_dir(); + let mut input = base_input(dir); + // config1.toml has persistent = true; CLI can turn it off + input.persistent = Some(false); + input.incremental = Some(false); + + let intent = resolve_build_intent(input)?; + assert!(!intent.config.incremental); + assert!(!intent.config.driver.persistent); + Ok(()) + } + + #[test] + fn resolve_keeps_docker_base_image_override() -> anyhow::Result<()> { + let dir = std::env::temp_dir(); + let mut input = base_input(dir); + input.driver_overrides.docker.base_image = Some("custom:image".to_string()); + + let intent = resolve_build_intent(input)?; + assert_eq!( + intent.driver_overrides.docker.base_image.as_deref(), + Some("custom:image") + ); + Ok(()) + } + + #[test] + fn resolve_absolutizes_paths() -> anyhow::Result<()> { + let dir = std::env::temp_dir(); + let intent = resolve_build_intent(base_input(dir.clone()))?; + assert!(intent.source_dir.is_absolute()); + assert!(intent.output_dir.is_absolute()); + assert_eq!(intent.source_dir, std::path::absolute(&dir)?); + assert_eq!(intent.output_dir, std::path::absolute(&dir)?); + Ok(()) + } + + #[test] + fn resolve_rejects_incremental_with_clean() { + let dir = std::env::temp_dir(); + let mut input = base_input(dir); + input.incremental = Some(true); + input.clean = Some(true); + + let result = resolve_build_intent(input); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("incremental builds are incompatible with clean builds") + ); + } + + #[test] + fn resolve_disable_incremental_for_source_builds() -> anyhow::Result<()> { + let dir = std::env::temp_dir(); + let mut input = base_input(dir); + input.incremental = Some(true); + input.disable_incremental = true; + + let intent = resolve_build_intent(input)?; + assert!(!intent.config.incremental); + Ok(()) + } +} diff --git a/packages/debmagic/src/config.rs b/packages/debmagic/src/config.rs index 3c5c8b7..21ba279 100644 --- a/packages/debmagic/src/config.rs +++ b/packages/debmagic/src/config.rs @@ -8,7 +8,7 @@ use config::{Config as ConfigBuilder, File}; use serde::Deserialize; /// documented in docs/usage/config.md -#[derive(Deserialize, Debug)] +#[derive(Deserialize, Debug, Clone)] #[serde(default)] pub struct Config { pub driver: DriverConfig, @@ -60,7 +60,6 @@ impl Config { } } - // TODO: reimplement cli arg overwrites let build = builder .build() .context("Failed to initialize config reader")?; diff --git a/packages/debmagic/src/main.rs b/packages/debmagic/src/main.rs index 99a4e03..5efad7d 100644 --- a/packages/debmagic/src/main.rs +++ b/packages/debmagic/src/main.rs @@ -1,67 +1,32 @@ -use std::{ - env, - path::{self, PathBuf}, -}; +use std::env; use anyhow::Context; use clap::{CommandFactory, Parser}; use crate::{ build::{ - BuildRequest, build_package, build_source_package, common::BuildDriverType, - config::DriverOverrides, driver_bare::DriverBareConfigOverrides, - driver_docker::DriverDockerConfigOverrides, driver_lxd::DriverLxdConfigOverrides, - get_shell_in_build, + build_package, build_source_package, common::BuildDriverType, config::DriverOverrides, + driver_bare::DriverBareConfigOverrides, driver_docker::DriverDockerConfigOverrides, + driver_lxd::DriverLxdConfigOverrides, get_shell_in_build, }, - cli::{BuildTarget, Cli, Commands, CommonBuildArgs}, - config::Config, - package::PackageDescription, + build_intent::{BuildIntentInput, load_config, resolve_build_intent}, + cli::{BuildTarget, Cli, Commands}, + package::{load_package_identity, resolve_package_target}, }; pub mod build; +pub mod build_intent; pub mod cli; pub mod config; pub mod package; -/// Precedence of config files is: -/// -/// 1. explicit config file passed on the command line -/// 2. `/debian/debmagic.toml` -/// 3. `/debmagic/config.toml` -/// -fn get_config(cli: &Cli, source_dir: &Option) -> anyhow::Result { - let mut config_file_paths = vec![]; - let xdg_config_file = dirs::config_dir().map(|p| p.join("debmagic").join("config.toml")); - if let Some(xdg_config_file) = xdg_config_file - && xdg_config_file.is_file() - { - config_file_paths.push(xdg_config_file); - } - - if let Some(source_dir) = &source_dir { - config_file_paths.push(source_dir.join("debian").join("debmagic.toml")); - } - - if let Some(config_file_override) = &cli.config { - config_file_paths.push(config_file_override.clone()); - } - - let config = Config::new(&config_file_paths)?; - Ok(config) -} - fn main() -> anyhow::Result<()> { let cli = Cli::parse(); let current_dir = env::current_dir()?; match &cli.command { Commands::Build(args) => { - let (build_args, debug_symbols, incremental, is_source): ( - &CommonBuildArgs, - Option, - Option, - bool, - ) = match &args.target { + let (build_args, debug_symbols, incremental, is_source) = match &args.target { BuildTarget::Binary(binary_args) => ( &binary_args.build, binary_args.debug_symbols, @@ -71,103 +36,62 @@ fn main() -> anyhow::Result<()> { BuildTarget::Source(source_args) => (&source_args.build, None, None, true), }; - let source_dir = build_args - .common - .source_dir - .as_deref() - .unwrap_or(¤t_dir); - let mut config = get_config(&cli, &Some(source_dir.to_path_buf()))?; - - // TODO: figure out a better way to override config from CLI args - maybe more generic, if that is even possible since - // we want a nice cli which somewhat matches the config structure - // but some config options only make sense in some cli subcommands -> these flags don't make sense in all commands - // and should only be used in some - if let Some(persistent) = build_args.persistent { - config.driver.persistent = persistent; - } - - if is_source { - config.incremental = false; - } else if let Some(incremental) = incremental { - config.incremental = incremental; - } + let driver = if is_source { + build_args.driver.unwrap_or(BuildDriverType::Bare) + } else { + build_args.driver.context( + "--driver is required for binary builds (docker, bare, lxd or incus)", + )? + }; - if let Some(debug_symbols) = debug_symbols { - config.build_debug_symbols = debug_symbols; - } - if let Some(sign) = build_args.sign { - config.sign_package = sign; - } - if let Some(no_sign) = build_args.no_sign { - config.sign_package = !no_sign; - } - if let Some(sign_with) = build_args.sign_with { - config.sign_with = sign_with; - } - if let Some(sign_key) = build_args.sign_key.clone() { - config.sign_key = Some(sign_key); - } - if let Some(clean) = build_args.clean { - config.clean = clean; - } - if let Some(no_clean) = build_args.no_clean { - config.clean = !no_clean; - } - if let Some(source_sync) = build_args.source_sync { - config.source_sync_mode = source_sync; - } - if config.incremental { - if config.clean { - anyhow::bail!("incremental builds are incompatible with clean builds"); - } - config.driver.persistent = true; - } - let driver_overrides = DriverOverrides { - apt_mirror: build_args.apt_mirror.clone(), - proposed: build_args.proposed, - docker: DriverDockerConfigOverrides { - base_image: build_args.docker.base_image.clone(), - }, - bare: DriverBareConfigOverrides {}, - lxd: DriverLxdConfigOverrides { - base_image: build_args.lxd.base_image.clone(), - project: build_args.lxd.project.clone(), + let intent = resolve_build_intent(BuildIntentInput { + fallback_dir: current_dir.clone(), + source_dir: build_args.common.source_dir.clone(), + output_dir: build_args.output_dir.clone(), + config_file: cli.config.clone(), + driver, + persistent: build_args.persistent, + incremental, + disable_incremental: is_source, + debug_symbols, + sign: build_args.sign, + no_sign: build_args.no_sign, + sign_with: build_args.sign_with, + sign_key: build_args.sign_key.clone(), + clean: build_args.clean, + no_clean: build_args.no_clean, + source_sync: build_args.source_sync, + driver_overrides: DriverOverrides { + apt_mirror: build_args.apt_mirror.clone(), + proposed: build_args.proposed, + docker: DriverDockerConfigOverrides { + base_image: build_args.docker.base_image.clone(), + }, + bare: DriverBareConfigOverrides {}, + lxd: DriverLxdConfigOverrides { + base_image: build_args.lxd.base_image.clone(), + project: build_args.lxd.project.clone(), + }, }, - }; + })?; - let package = PackageDescription::from_dir( - &path::absolute(source_dir).context("resolving source dir failed")?, - )?; - let output_dir = build_args.output_dir.as_deref().unwrap_or(¤t_dir); - let output_dir = path::absolute(output_dir).context("resolving output dir failed")?; + let target = resolve_package_target(&intent.source_dir, build_args.distro.as_deref()) + .context("failed to determine package target")?; - let request = BuildRequest { - config: &config, - package: &package, - driver_type: if is_source { - build_args.driver.unwrap_or(BuildDriverType::Bare) - } else { - build_args.driver.context( - "--driver is required for binary builds (docker, bare, lxd or incus)", - )? - }, - driver_overrides: &driver_overrides, - output_dir: &output_dir, - explicit_distro_version: build_args.distro.as_deref(), - }; if is_source { - build_source_package(&request).context("Building the source package failed")?; + build_source_package(&intent, &target) + .context("Building the source package failed")?; } else { - build_package(&request).context("Building the package failed")?; + build_package(&intent, &target).context("Building the package failed")?; } } Commands::Shell(args) => { let source_dir = args.common.source_dir.as_deref().unwrap_or(¤t_dir); - let config = get_config(&cli, &Some(source_dir.to_path_buf()))?; - let package = PackageDescription::from_dir( - &path::absolute(source_dir).context("resolving source dir failed")?, - )?; - get_shell_in_build(&config, &package)?; + let source_dir = + std::path::absolute(source_dir).context("resolving source dir failed")?; + let config = load_config(Some(&source_dir), cli.config.as_deref())?; + let identity = load_package_identity(&source_dir)?; + get_shell_in_build(&config, &identity)?; } Commands::Test(_args) => { println!("Test subcommand! - not implemented"); diff --git a/packages/debmagic/src/package.rs b/packages/debmagic/src/package.rs index 66e3d5f..7da6a31 100644 --- a/packages/debmagic/src/package.rs +++ b/packages/debmagic/src/package.rs @@ -1,84 +1,274 @@ use std::path::{Path, PathBuf}; use anyhow::anyhow; - use debmagic_common::debian::version::PackageVersion; +use debmagic_common::distro::DistroVersion; +/// Who/what is being built, as read from the source tree changelog. #[derive(Debug, Clone)] -pub struct PackageDescription { +pub struct PackageIdentity { pub name: String, pub version: PackageVersion, pub source_dir: PathBuf, - pub distro_versions: Vec, } -impl PackageDescription { - pub fn from_dir(dir: &Path) -> anyhow::Result { - let changelog_file = dir.join("debian").join("changelog"); - let changelog_contents = std::fs::read_to_string(changelog_file)?; - let changelog: debian_changelog::ChangeLog = changelog_contents.parse()?; - - let first_entry = changelog - .into_iter() - .next() - .ok_or(anyhow!("changelog is empty"))?; - - let name = first_entry - .package() - .ok_or(anyhow!("empty package name in changelog entry"))?; - let version = first_entry - .version() - .ok_or(anyhow!("no package version in changelog entry")) - .map(|v| PackageVersion::new(v.epoch, v.upstream_version, v.debian_revision))?; - - let distro_versions = first_entry - .distributions() - .ok_or(anyhow!("no distribution specified in changelog entry"))?; - - Ok(Self { +/// A [`PackageIdentity`] plus the chosen [`DistroVersion`] for a build run. +#[derive(Debug, Clone)] +pub struct PackageTarget { + pub identity: PackageIdentity, + pub distro: DistroVersion, +} + +struct ChangelogPackage { + identity: PackageIdentity, + /// Raw distribution names from the changelog entry (not looked up yet). + changelog_distros: Vec, +} + +fn read_changelog_package(dir: &Path) -> anyhow::Result { + let changelog_file = dir.join("debian").join("changelog"); + let changelog_contents = std::fs::read_to_string(changelog_file)?; + let changelog: debian_changelog::ChangeLog = changelog_contents.parse()?; + + let first_entry = changelog + .into_iter() + .next() + .ok_or(anyhow!("changelog is empty"))?; + + let name = first_entry + .package() + .ok_or(anyhow!("empty package name in changelog entry"))?; + let version = first_entry + .version() + .ok_or(anyhow!("no package version in changelog entry")) + .map(|v| PackageVersion::new(v.epoch, v.upstream_version, v.debian_revision))?; + + let changelog_distros = first_entry + .distributions() + .ok_or(anyhow!("no distribution specified in changelog entry"))?; + + Ok(ChangelogPackage { + identity: PackageIdentity { name, version, source_dir: dir.to_path_buf(), - distro_versions, - }) + }, + changelog_distros, + }) +} + +pub fn load_package_identity(dir: &Path) -> anyhow::Result { + Ok(read_changelog_package(dir)?.identity) +} + +/// Resolve package identity and target distro from a source tree. +/// +/// If only one distribution is listed in the changelog, it is used automatically. +/// If multiple are listed, an explicit `--distro` is required and must match one of them. +/// Suite aliases (`stable`, `sid`, `devel`, …) resolve to the same concrete +/// [`DistroVersion`] as their canonical codename. +pub fn resolve_package_target( + dir: &Path, + explicit_distro: Option<&str>, +) -> anyhow::Result { + let parsed = read_changelog_package(dir)?; + let distro = select_distro_version(&parsed.changelog_distros, explicit_distro)?; + Ok(PackageTarget { + identity: parsed.identity, + distro, + }) +} + +fn lookup_distro(name: &str) -> anyhow::Result { + debmagic_common::distro::get_distro_version(name) + .ok_or_else(|| anyhow!("unknown distro codename '{}'", name)) +} + +fn select_distro_version( + changelog_distros: &[String], + explicit_distro: Option<&str>, +) -> anyhow::Result { + match (changelog_distros.len(), explicit_distro) { + (0, _) => Err(anyhow!("changelog contains no distributions")), + (1, None) => lookup_distro(&changelog_distros[0]), + (1, Some(explicit)) => { + let from_changelog = lookup_distro(&changelog_distros[0])?; + let from_explicit = lookup_distro(explicit)?; + if from_changelog == from_explicit { + Ok(from_explicit) + } else { + Err(anyhow!( + "explicit distro version '{}' conflicts with distribution specified in changelog '{}'", + explicit, + changelog_distros[0] + )) + } + } + (_, None) => Err(anyhow!( + "changelog contains multiple distributions ({}), please specify which one to build for with --distro", + changelog_distros.join(", ") + )), + (_, Some(explicit)) => { + let from_explicit = lookup_distro(explicit)?; + let matched = changelog_distros.iter().any(|name| { + lookup_distro(name).is_ok_and(|from_changelog| from_changelog == from_explicit) + }); + if matched { + Ok(from_explicit) + } else { + Err(anyhow!( + "explicit distro version '{}' not found in changelog distributions: {}", + explicit, + changelog_distros.join(", ") + )) + } + } } } #[cfg(test)] mod tests { + use debmagic_common::distro::Distro; + use super::*; - #[test] - fn test_package_description_from_changelog() -> Result<(), anyhow::Error> { - let test_asset_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + fn test_package_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("tests") .join("assets") - .join("test_package"); + .join("test_package") + } - let package = PackageDescription::from_dir(&test_asset_dir)?; + fn test_package_multi_distro_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("tests") + .join("assets") + .join("test_package_multi_distro") + } - assert_eq!(package.name, "test-package"); - assert_eq!(package.version.version(), "1.2.4-1"); - assert_eq!(package.distro_versions, vec!["stable"]); - assert_eq!(package.source_dir, test_asset_dir); + #[test] + fn load_package_identity_from_changelog() -> anyhow::Result<()> { + let dir = test_package_dir(); + let identity = load_package_identity(&dir)?; + assert_eq!(identity.name, "test-package"); + assert_eq!(identity.version.version(), "1.2.4-1"); + assert_eq!(identity.source_dir, dir); Ok(()) } #[test] - fn test_package_description_with_multiple_distros() -> Result<(), anyhow::Error> { - let test_asset_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")) - .join("tests") - .join("assets") - .join("test_package_multi_distro"); + fn resolve_package_target_stable_aliases_to_trixie() -> anyhow::Result<()> { + let target = resolve_package_target(&test_package_dir(), None)?; + assert_eq!(target.distro.codename, "trixie"); + assert_eq!(target.distro.distro, Distro::Debian); + Ok(()) + } - let package = PackageDescription::from_dir(&test_asset_dir)?; + #[test] + fn select_distro_version_alias_matches_canonical_explicit() -> anyhow::Result<()> { + let distro = select_distro_version(&["stable".to_string()], Some("trixie"))?; + assert_eq!(distro.codename, "trixie"); + Ok(()) + } - assert_eq!(package.name, "test-package"); - assert_eq!(package.version.version(), "1.2.4-1"); - assert_eq!(package.distro_versions, vec!["unstable", "testing"]); - assert_eq!(package.source_dir, test_asset_dir); + #[test] + fn select_distro_version_sid_matches_unstable_explicit() -> anyhow::Result<()> { + let distro = select_distro_version(&["sid".to_string()], Some("unstable"))?; + assert_eq!(distro.codename, "unstable"); + Ok(()) + } + + #[test] + fn resolve_package_target_multiple_distros_requires_explicit() { + let result = resolve_package_target(&test_package_multi_distro_dir(), None); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("multiple distributions") + ); + } + + #[test] + fn resolve_package_target_multiple_distros_with_explicit() -> anyhow::Result<()> { + let target = resolve_package_target(&test_package_multi_distro_dir(), Some("unstable"))?; + assert_eq!(target.identity.name, "test-package"); + assert_eq!(target.distro.codename, "unstable"); + assert_eq!(target.distro.distro, Distro::Debian); + Ok(()) + } + + #[test] + fn select_distro_version_single_no_explicit() -> anyhow::Result<()> { + let distro = select_distro_version(&["forky".to_string()], None)?; + assert_eq!(distro.codename, "forky"); + assert_eq!(distro.distro, Distro::Debian); + Ok(()) + } + #[test] + fn select_distro_version_single_matching_explicit() -> anyhow::Result<()> { + let distro = select_distro_version(&["forky".to_string()], Some("forky"))?; + assert_eq!(distro.codename, "forky"); Ok(()) } + + #[test] + fn select_distro_version_single_conflicting_explicit() { + let result = select_distro_version(&["forky".to_string()], Some("duke")); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("conflicts with distribution specified in changelog") + ); + } + + #[test] + fn select_distro_version_multiple_no_explicit() { + let result = select_distro_version(&["forky".to_string(), "duke".to_string()], None); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("multiple distributions") + ); + } + + #[test] + fn select_distro_version_multiple_explicit_valid() -> anyhow::Result<()> { + let distro = + select_distro_version(&["forky".to_string(), "duke".to_string()], Some("duke"))?; + assert_eq!(distro.codename, "duke"); + Ok(()) + } + + #[test] + fn select_distro_version_multiple_explicit_invalid() { + let result = + select_distro_version(&["forky".to_string(), "duke".to_string()], Some("trixie")); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("not found in changelog distributions") + ); + } + + #[test] + fn select_distro_version_empty_distros() { + let result = select_distro_version(&[], None); + assert!(result.is_err()); + assert!( + result + .unwrap_err() + .to_string() + .contains("changelog contains no distributions") + ); + } }