deb: consider all kinds of package dirs

Co-authored-by: Valentin Haudiquet <valentin.haudiquet@canonical.com>
Co-committed-by: Valentin Haudiquet <valentin.haudiquet@canonical.com>

.github/workflows/ci.yml

@@ -2,7 +2,7 @@ name: CI
 
 on:
   push:
-    branches: [ "main" ]
+    branches: [ "main", "ci-test" ]
   pull_request:
     branches: [ "main" ]
 
@@ -23,7 +23,7 @@ jobs:
     - uses: actions/checkout@v6
     - uses: dtolnay/rust-toolchain@stable
       with:
-        components: rustfmt
+        components: rustfmt, clippy
     - name: Check format
       run: cargo fmt --check
     - name: Install build dependencies
@@ -32,6 +32,12 @@ jobs:
         sudo apt-get install -y pkg-config libssl-dev libgpg-error-dev libgpgme-dev
     - name: Build
       run: cargo build
+      env:
+        RUSTFLAGS: -Dwarnings
+    - name: Lint
+      run: cargo clippy --all-targets --all-features
+      env:
+        RUSTFLAGS: -Dwarnings
     - name: Install runtime system dependencies
       run: |
         sudo apt-get update

Cargo.toml

@@ -27,8 +27,9 @@ xz2 = "0.1"
 serde_json = "1.0.145"
 directories = "6.0.0"
 ssh2 = "0.9.5"
-tempfile = "3.10.1"
 gpgme = "0.11"
 
 [dev-dependencies]
 test-log = "0.2.19"
+serial_test = "3.3.1"
+tempfile = "3.10.1"

README.md

@@ -24,8 +24,10 @@ Options:
 Commands and workflows include:
 ```
 Commands:
-  pull    Get a source package from the archive or git
+  pull   Pull a source package from the archive or git
   chlog  Auto-generate changelog entry, editing it, committing it afterwards
+  build  Build the source package (into a .dsc)
+  deb    Build the source package into binary package (.deb)
   help   Print this message or the help of the given subcommand(s)
 ```
 
@@ -96,7 +98,7 @@ Missing features:
   - [ ] Three build modes:
     - [ ] Build locally (discouraged)
     - [x] Build using sbuild+unshare, with binary emulation (default)
-    - [ ] Cross-compilation
+    - [x] Cross-compilation
   - [ ] Async build
 - [ ] `pkh status`
   - [ ] Show build status

src/context/local.rs

@@ -4,6 +4,7 @@ use super::api::ContextDriver;
 use std::io;
 use std::path::{Path, PathBuf};
 use std::process::Command;
+use std::time::SystemTime;
 
 pub struct LocalDriver;
 
@@ -20,8 +21,34 @@ impl ContextDriver for LocalDriver {
     }
 
     fn create_temp_dir(&self) -> io::Result<String> {
-        let temp_dir = tempfile::Builder::new().prefix("pkh-").tempdir()?;
+        // Generate a unique temporary directory name with random string
-        Ok(temp_dir.keep().to_string_lossy().to_string())
+        let base_timestamp = SystemTime::now()
+            .duration_since(SystemTime::UNIX_EPOCH)
+            .unwrap()
+            .as_secs();
+
+        let mut attempt = 0;
+        loop {
+            let work_dir_name = if attempt == 0 {
+                format!("pkh-{}", base_timestamp)
+            } else {
+                format!("pkh-{}-{}", base_timestamp, attempt)
+            };
+
+            let temp_dir_path = std::env::temp_dir().join(&work_dir_name);
+
+            // Check if directory already exists
+            if temp_dir_path.exists() {
+                attempt += 1;
+                continue;
+            }
+
+            // Create the directory
+            std::fs::create_dir_all(&temp_dir_path)?;
+
+            // Return the path as a string
+            return Ok(temp_dir_path.to_string_lossy().to_string());
+        }
     }
 
     fn retrieve_path(&self, src: &Path, dest: &Path) -> io::Result<()> {

src/context/unshare.rs

@@ -112,27 +112,41 @@ impl ContextDriver for UnshareDriver {
     }
 
     fn create_temp_dir(&self) -> io::Result<String> {
-        // Create a temporary directory inside the chroot
+        // Create a temporary directory inside the chroot with unique naming
-        let timestamp = std::time::SystemTime::now()
+        let base_timestamp = std::time::SystemTime::now()
             .duration_since(std::time::UNIX_EPOCH)
             .unwrap()
             .as_secs();
 
-        let work_dir_name = format!("pkh-build-{}", timestamp);
+        let mut attempt = 0;
-        let work_dir_inside_chroot = format!("/tmp/{}", work_dir_name);
+        loop {
+            let work_dir_name = if attempt == 0 {
+                format!("pkh-build-{}", base_timestamp)
+            } else {
+                format!("pkh-build-{}-{}", base_timestamp, attempt)
+            };
 
-        // Create the directory on the host filesystem
+            let work_dir_inside_chroot = format!("/tmp/{}", work_dir_name);
-        let host_path = Path::new(&self.path).join("tmp").join(&work_dir_name);
+            let host_path = Path::new(&self.path).join("tmp").join(&work_dir_name);
-        std::fs::create_dir_all(&host_path)?;
 
-        debug!(
+            // Check if directory already exists
-            "Created work directory: {} (host: {})",
+            if host_path.exists() {
-            work_dir_inside_chroot,
+                attempt += 1;
-            host_path.display()
+                continue;
-        );
+            }
 
-        // Return the path as it appears inside the chroot
+            // Create the directory on the host filesystem
-        Ok(work_dir_inside_chroot)
+            std::fs::create_dir_all(&host_path)?;
+
+            debug!(
+                "Created work directory: {} (host: {})",
+                work_dir_inside_chroot,
+                host_path.display()
+            );
+
+            // Return the path as it appears inside the chroot
+            return Ok(work_dir_inside_chroot);
+        }
     }
 
     fn copy_path(&self, src: &Path, dest: &Path) -> io::Result<()> {

src/deb/local.rs

@@ -80,10 +80,16 @@ pub fn build(
         return Err("Could not install essential packages for the build".into());
     }
 
+    // Find the actual package directory
+    let package_dir = crate::deb::find_package_directory(Path::new(build_root), package, version)?;
+    let package_dir_str = package_dir
+        .to_str()
+        .ok_or("Invalid package directory path")?;
+
     // Install build dependencies
     log::debug!("Installing build dependencies...");
     let mut cmd = ctx.command("apt-get");
-    cmd.current_dir(format!("{build_root}/{package}"))
+    cmd.current_dir(package_dir_str)
         .envs(env.clone())
         .arg("-y")
         .arg("build-dep");
@@ -102,7 +108,7 @@ pub fn build(
     log::debug!("Building (debian/rules build) package...");
     let status = ctx
         .command("debian/rules")
-        .current_dir(format!("{build_root}/{package}"))
+        .current_dir(package_dir_str)
         .envs(env.clone())
         .arg("build")
         .status()?;
@@ -113,7 +119,7 @@ pub fn build(
     // Run the 'binary' step to produce deb
     let status = ctx
         .command("fakeroot")
-        .current_dir(format!("{build_root}/{package}"))
+        .current_dir(package_dir_str)
         .envs(env.clone())
         .arg("debian/rules")
         .arg("binary")

src/deb/mod.rs

@@ -85,6 +85,87 @@ pub fn build_binary_package(
     Ok(())
 }
 
+/// Find the current package directory by trying both patterns:
+/// - package/package
+/// - package/package-origversion
+pub(crate) fn find_package_directory(
+    parent_dir: &Path,
+    package: &str,
+    version: &str,
+) -> Result<PathBuf, Box<dyn Error>> {
+    let ctx = context::current();
+
+    // Try package/package pattern first
+    let package_dir = parent_dir.join(package).join(package);
+    if ctx.exists(&package_dir)? {
+        return Ok(package_dir);
+    }
+
+    // Compute origversion from version: remove everything after first '-', after stripping epoch
+    let version_without_epoch = version.split_once(':').map(|(_, v)| v).unwrap_or(version);
+    let origversion = version_without_epoch
+        .split_once('-')
+        .map(|(v, _)| v)
+        .unwrap_or(version);
+
+    // Try package/package-origversion pattern
+    let package_dir = parent_dir
+        .join(package)
+        .join(format!("{}-{}", package, origversion));
+    if ctx.exists(&package_dir)? {
+        return Ok(package_dir);
+    }
+
+    // Try 'package' only
+    let package_dir = parent_dir.join(package);
+    if ctx.exists(&package_dir)? {
+        return Ok(package_dir);
+    }
+
+    // Try package-origversion only
+    let package_dir = parent_dir.join(format!("{}-{}", package, origversion));
+    if ctx.exists(&package_dir)? {
+        return Ok(package_dir);
+    }
+
+    // List all directories under 'package/' and log them
+    let package_parent = parent_dir;
+    if ctx.exists(package_parent)? {
+        log::debug!(
+            "Listing all directories under '{}':",
+            package_parent.display()
+        );
+        let entries = ctx.list_files(package_parent)?;
+        let mut found_dirs = Vec::new();
+        for entry in entries {
+            if entry.is_dir() {
+                if let Some(file_name) = entry.file_name() {
+                    found_dirs.push(file_name.to_string_lossy().into_owned());
+                }
+                log::debug!("  - {}", entry.display());
+            }
+        }
+
+        // If we found directories but none matched our patterns, provide helpful error
+        if !found_dirs.is_empty() {
+            return Err(format!(
+                "Could not find package directory for {} in {}. Found directories: {}",
+                package,
+                parent_dir.display(),
+                found_dirs.join(", ")
+            )
+            .into());
+        }
+    }
+
+    Err(format!(
+        "Could not find package directory for {} in {}",
+        package,
+        parent_dir.display()
+    )
+    .into())
+}
+
 fn find_dsc_file(
     build_root: &str,
     package: &str,
@@ -105,6 +186,7 @@ fn find_dsc_file(
 
 #[cfg(test)]
 mod tests {
+    use serial_test::serial;
     async fn test_build_end_to_end(
         package: &str,
         series: &str,
@@ -124,14 +206,18 @@ mod tests {
         let cwd = temp_dir.path();
         log::debug!("Created temporary directory: {}", cwd.display());
 
-        log::info!("Pulling package {} from Ubuntu {}...", package, series);
+        log::info!("Pulling package {} from {}...", package, series);
-        crate::pull::pull(package, "", Some(series), "", "", dist, Some(cwd), None)
+        let package_info = crate::package_info::lookup(package, None, Some(series), "", dist, None)
+            .await
+            .expect("Cannot lookup package information");
+        crate::pull::pull(&package_info, Some(cwd), None, true)
             .await
             .expect("Cannot pull package");
         log::info!("Successfully pulled package {}", package);
 
         // Change directory to the package directory
-        let cwd = cwd.join(package).join(package);
+        let cwd = crate::deb::find_package_directory(cwd, package, &package_info.stanza.version)
+            .expect("Cannot find package directory");
         log::debug!("Package directory: {}", cwd.display());
 
         log::info!("Starting binary package build...");
@@ -159,8 +245,16 @@ mod tests {
         );
     }
 
+    // Tests below will be marked 'serial'
+    // As builds are using ephemeral contexts, tests running on the same
+    // process could use the ephemeral context of another thread and
+    // interfere with each other.
+    // FIXME: This is not ideal. In the future, we might want to
+    // either explicitely pass context (instead of shared state) or
+    // fork for building?
     #[tokio::test]
     #[test_log::test]
+    #[serial]
     async fn test_deb_hello_ubuntu_end_to_end() {
         test_build_end_to_end("hello", "noble", None, None, false).await;
     }
@@ -168,6 +262,7 @@ mod tests {
     #[tokio::test]
     #[test_log::test]
     #[cfg(target_arch = "x86_64")]
+    #[serial]
     async fn test_deb_hello_ubuntu_cross_end_to_end() {
         test_build_end_to_end("hello", "noble", None, Some("riscv64"), true).await;
     }

src/deb/sbuild.rs

@@ -2,18 +2,26 @@
 /// Call 'sbuild' with the dsc file to build the package with unshare
 use crate::context;
 use std::error::Error;
+use std::path::Path;
 
 pub fn build(
     package: &str,
-    _version: &str,
+    version: &str,
     arch: &str,
     series: &str,
     build_root: &str,
     cross: bool,
 ) -> Result<(), Box<dyn Error>> {
     let ctx = context::current();
+
+    // Find the actual package directory
+    let package_dir = crate::deb::find_package_directory(Path::new(build_root), package, version)?;
+    let package_dir_str = package_dir
+        .to_str()
+        .ok_or("Invalid package directory path")?;
+
     let mut cmd = ctx.command("sbuild");
-    cmd.current_dir(format!("{}/{}", build_root, package));
+    cmd.current_dir(package_dir_str);
     cmd.arg("--chroot-mode=unshare");
     cmd.arg("--no-clean-source");
 

src/main.rs

@@ -7,8 +7,6 @@ use pkh::context::ContextConfig;
 
 extern crate flate2;
 
-use pkh::pull::pull;
-
 use pkh::changelog::generate_entry;
 
 use indicatif_log_bridge::LogWrapper;
@@ -49,10 +47,10 @@ fn main() {
                 .arg(arg!(--backport "This changelog is for a backport entry").required(false))
                 .arg(arg!(-v --version <version> "Target version").required(false)),
         )
-        .subcommand(Command::new("build").about("Build the source package"))
+        .subcommand(Command::new("build").about("Build the source package (into a .dsc)"))
         .subcommand(
             Command::new("deb")
-                .about("Build the binary package")
+                .about("Build the source package into binary package (.deb)")
                 .arg(arg!(-s --series <series> "Target distribution series").required(false))
                 .arg(arg!(-a --arch <arch> "Target architecture").required(false))
                 .arg(arg!(--cross "Cross-compile for target architecture (instead of qemu-binfmt)")
@@ -94,11 +92,8 @@ fn main() {
             let package = sub_matches.get_one::<String>("package").expect("required");
             let series = sub_matches.get_one::<String>("series").map(|s| s.as_str());
             let dist = sub_matches.get_one::<String>("dist").map(|s| s.as_str());
-            let version = sub_matches
+            let version = sub_matches.get_one::<String>("version").map(|s| s.as_str());
-                .get_one::<String>("version")
+            let _ppa = sub_matches
-                .map(|s| s.as_str())
-                .unwrap_or("");
-            let ppa = sub_matches
                 .get_one::<String>("ppa")
                 .map(|s| s.as_str())
                 .unwrap_or("");
@@ -106,16 +101,18 @@ fn main() {
             let (pb, progress_callback) = ui::create_progress_bar(&multi);
 
             // Since pull is async, we need to block on it
-            if let Err(e) = rt.block_on(pull(
+            if let Err(e) = rt.block_on(async {
-                package,
+                let package_info = pkh::package_info::lookup(
-                version,
+                    package,
-                series,
+                    version,
-                "",
+                    series,
-                ppa,
+                    "",
-                dist,
+                    dist,
-                None,
+                    Some(&progress_callback),
-                Some(&progress_callback),
+                )
-            )) {
+                .await?;
+                pkh::pull::pull(&package_info, None, Some(&progress_callback), false).await
+            }) {
                 pb.finish_and_clear();
                 error!("{}", e);
                 std::process::exit(1);

src/package_info.rs

@@ -56,7 +56,8 @@ fn parse_series_csv(content: &str) -> Result<Vec<String>, Box<dyn Error>> {
     Ok(entries.into_iter().map(|(s, _)| s).collect())
 }
 
-async fn get_ordered_series(dist: &str) -> Result<Vec<String>, Box<dyn Error>> {
+/// Get time-ordered list of series for a distribution, development series first
+pub async fn get_ordered_series(dist: &str) -> Result<Vec<String>, Box<dyn Error>> {
     let content = if Path::new(format!("/usr/share/distro-info/{dist}.csv").as_str()).exists() {
         std::fs::read_to_string(format!("/usr/share/distro-info/{dist}.csv"))?
     } else {
@@ -71,9 +72,8 @@ async fn get_ordered_series(dist: &str) -> Result<Vec<String>, Box<dyn Error>> {
 
     let mut series = parse_series_csv(&content)?;
 
-    // For Debian, ensure 'sid' is first if it's not (it usually doesn't have a date or is very old/new depending on file)
+    // For Debian, ensure 'sid' is first if it's not
-    // Actually in the file sid has 1993 date.
+    // We want to try 'sid' (unstable) first for Debian.
-    // But we want to try 'sid' (unstable) first for Debian.
     if dist == "debian" {
         series.retain(|s| s != "sid");
         series.insert(0, "sid".to_string());
@@ -332,7 +332,7 @@ fn parse_sources(
 }
 
 /// Get package information from a package, distribution series, and pocket
-pub async fn get(
+async fn get(
     package_name: &str,
     series: &str,
     pocket: &str,
@@ -406,7 +406,7 @@ pub async fn get(
 }
 
 /// Try to find package information in a distribution, trying all series and pockets
-pub async fn find_package(
+async fn find_package(
     package_name: &str,
     dist: &str,
     pocket: &str,
@@ -452,6 +452,58 @@ pub async fn find_package(
     Err(format!("Package '{}' not found.", package_name).into())
 }
 
+/// Lookup package information for a source package
+///
+/// This function obtains package information either directly from a specific series
+/// or by searching across all series in a distribution.
+pub async fn lookup(
+    package: &str,
+    version: Option<&str>,
+    series: Option<&str>,
+    pocket: &str,
+    dist: Option<&str>,
+    progress: ProgressCallback<'_>,
+) -> Result<PackageInfo, Box<dyn Error>> {
+    // Obtain the package information, either directly in a series or with a search in all series
+    let package_info = if let Some(s) = series {
+        if let Some(cb) = progress {
+            cb(
+                &format!("Resolving package info for {}...", package),
+                "",
+                0,
+                0,
+            );
+        }
+
+        // Get the package information from that series and pocket
+        get(package, s, pocket, version).await?
+    } else {
+        let dist = dist.unwrap_or_else(||
+            // Use auto-detection to see if current distro is ubuntu, or fallback to debian by default
+            if std::process::Command::new("lsb_release").arg("-i").arg("-s").output()
+                .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_lowercase()).unwrap_or_default() == "ubuntu" {
+                "ubuntu"
+            } else {
+                "debian"
+            }
+        );
+
+        if let Some(cb) = progress {
+            cb(
+                &format!("Searching for package {} in {}...", package, dist),
+                "",
+                0,
+                0,
+            );
+        }
+
+        // Try to find the package in all series from that dist
+        find_package(package, dist, pocket, version, progress).await?
+    };
+
+    Ok(package_info)
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

src/pull.rs

@@ -2,7 +2,6 @@ use std::cmp::min;
 use std::error::Error;
 use std::path::Path;
 
-use crate::package_info;
 use crate::package_info::PackageInfo;
 
 use std::process::Command;
@@ -333,64 +332,19 @@ async fn fetch_archive_sources(
     Ok(())
 }
 
-/// Pull a source package locally
+/// Pull a source package locally using pre-retrieved package information
 ///
-/// Will try to find the package information, and use it to download it over prefered way
+/// This function takes a PackageInfo struct and downloads the package using the preferred method
-/// (either git or direct archive download), as well as orig tarball, inside 'package' directory
+/// (either git or direct archive download), as well as orig tarball, inside 'package' directory.
-/// The source will be extracted under 'package/package'
+/// The source will be extracted under 'package/package'.
 pub async fn pull(
-    package: &str,
+    package_info: &PackageInfo,
-    _version: &str,
-    series: Option<&str>,
-    pocket: &str,
-    _ppa: &str,
-    dist: Option<&str>,
     cwd: Option<&Path>,
     progress: ProgressCallback<'_>,
-) -> Result<PackageInfo, Box<dyn Error>> {
+    force_archive: bool,
-    let version_opt = if _version.is_empty() {
+) -> Result<(), Box<dyn Error>> {
-        None
+    let package = &package_info.stanza.package;
-    } else {
+    let series = &package_info.series;
-        Some(_version)
-    };
-
-    /* Obtain the package information, either directly in a series or with a search in all series */
-    let package_info = if let Some(s) = series {
-        if let Some(cb) = progress {
-            cb(
-                &format!("Resolving package info for {}...", package),
-                "",
-                0,
-                0,
-            );
-        }
-
-        // Get the package information from that series and pocket
-        package_info::get(package, s, pocket, version_opt).await?
-    } else {
-        let dist = dist.unwrap_or_else(||
-            // Use auto-detection to see if current distro is ubuntu, or fallback to debian by default
-            if std::process::Command::new("lsb_release").arg("-i").arg("-s").output()
-                .map(|o| String::from_utf8_lossy(&o.stdout).trim().to_lowercase()).unwrap_or_default() == "ubuntu" {
-                "ubuntu"
-            } else {
-                "debian"
-            }
-        );
-
-        if let Some(cb) = progress {
-            cb(
-                &format!("Searching for package {} in {}...", package, dist),
-                "",
-                0,
-                0,
-            );
-        }
-
-        // Try to find the package in all series from that dist
-        package_info::find_package(package, dist, pocket, version_opt, progress).await?
-    };
-
     let package_dir = if let Some(path) = cwd {
         path.join(package)
     } else {
@@ -398,15 +352,20 @@ pub async fn pull(
     };
 
     /* Fetch the package: either via git (preferred VCS) or the archive */
-    if let Some(ref url) = package_info.preferred_vcs {
+    if let Some(ref url) = package_info.preferred_vcs
+        && !force_archive
+    {
         // We have found a preferred VCS (git repository) for the package, so
         // we fetch the package from that repo.
 
-        // Depending on target series, we pick target branch; if no series is specified,
+        // Depending on target series, we pick target branch; if latest series is specified,
         // we target the development branch, i.e. the default branch
-        let branch_name = if let Some(s) = series {
+        let branch_name = if crate::package_info::get_ordered_series(package_info.dist.as_str())
+            .await?[0]
+            != *series
+        {
             if package_info.dist == "ubuntu" {
-                Some(format!("{}/{}", package_info.dist, s))
+                Some(format!("{}/{}", package_info.dist, series))
             } else {
                 // Debian does not have reliable branch naming...
                 // For now, we skip that part and clone default
@@ -446,7 +405,7 @@ pub async fn pull(
             if let Some(cb) = progress {
                 cb("Fetching orig tarball...", "", 0, 0);
             }
-            fetch_orig_tarball(&package_info, Some(&package_dir), progress).await?;
+            fetch_orig_tarball(package_info, Some(&package_dir), progress).await?;
         } else {
             debug!("Native package, skipping orig tarball fetch.");
         }
@@ -454,16 +413,16 @@ pub async fn pull(
         if let Some(cb) = progress {
             cb("Fetching dsc file...", "", 0, 0);
         }
-        fetch_dsc_file(&package_info, Some(&package_dir), progress).await?;
+        fetch_dsc_file(package_info, Some(&package_dir), progress).await?;
     } else {
         // Fallback to archive fetching
         if let Some(cb) = progress {
             cb("Downloading from archive...", "", 0, 0);
         }
-        fetch_archive_sources(&package_info, Some(&package_dir), progress).await?;
+        fetch_archive_sources(package_info, Some(&package_dir), progress).await?;
     }
 
-    Ok(package_info)
+    Ok(())
 }
 
 #[cfg(test)]
@@ -475,16 +434,17 @@ mod tests {
 
         // For determinism, we require for tests that either a distro or series is specified,
         // as no distribution would mean fallback to system distro
-        assert!(dist != None || series != None);
+        assert!(dist.is_some() || series.is_some());
 
         // Use a temp directory as working directory
         let temp_dir = tempfile::tempdir().unwrap();
         let cwd = temp_dir.path();
 
         // Main 'pull' command: the one we want to test
-        let info = pull(package, "", series, "", "", dist, Some(cwd), None)
+        let info = crate::package_info::lookup(package, None, series, "", dist, None)
             .await
             .unwrap();
+        pull(&info, Some(cwd), None, false).await.unwrap();
 
         let package_dir = cwd.join(package);
         assert!(package_dir.exists());