Re: [pbs-devel] [PATCH v6 proxmox-apt 06/11] add release_upgrade function and constants for the current and upgrade suite

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On June 11, 2021 1:43 pm, Fabian Ebner wrote:
> useful for major upgrades. The stable branch can enable the upgrade, and bump
> the minor version, while the master branch will adapt to the new release and
> bump the major version. Each product can depend on the the new major version
> after branching off the stable branch, and once the release is out, its stable
> branch can depend on the new minor version.
> 
> Signed-off-by: Fabian Ebner <f.ebner@proxmox.com>
> ---
> 
> Changes from v5:
>     * Make function less general (only care about the current release upgrade)
>       and handle special case for security repository.
>     * Make list of suite available as constants.
>     * Get the current release from /etc/os-release and abort if it is not the
>       same as STABLE_SUITE.
>     * Add a constant UPGRADE_SUITE which can be set for the library's last
>       release in the stable-X branch to enable the release_upgrade() function.
> 
>  .gitignore                |  1 +
>  src/repositories/check.rs | 57 +++++++++++++-----------
>  src/repositories/mod.rs   | 92 +++++++++++++++++++++++++++++++++++++++
>  tests/repositories.rs     | 79 ++++++++++++++++++++++++++++++++-
>  4 files changed, 202 insertions(+), 27 deletions(-)
> 
> diff --git a/.gitignore b/.gitignore
> index db6f13e..de68da9 100644
> --- a/.gitignore
> +++ b/.gitignore
> @@ -1,6 +1,7 @@
>  Cargo.lock
>  target/
>  tests/sources.list.d.actual
> +tests/sources.list.d.upgraded.actual
>  tests/sources.list.d.digest
>  proxmox-apt-*/
>  *proxmox-apt*.buildinfo
> diff --git a/src/repositories/check.rs b/src/repositories/check.rs
> index 585c28d..e0ec93e 100644
> --- a/src/repositories/check.rs
> +++ b/src/repositories/check.rs
> @@ -5,8 +5,34 @@ use crate::types::{
>      APTRepositoryPackageType,
>  };
>  
> +/// The (code)names of old Debian releases.
> +pub const OLD_SUITES: [&str; 7] = [
> +    "lenny",
> +    "squeeze",
> +    "wheezy",
> +    "jessie",
> +    "stretch",
> +    "oldoldstable",
> +    "oldstable",
> +];
> +
> +/// The codename of the current stable Debian release.
> +pub const STABLE_SUITE: &str = "buster";
> +/// The codename of the next stable Debian release.
> +pub const NEXT_STABLE_SUITE: &str = "bullseye";
> +
> +/// The (code)names of new/testing Debian releases.
> +pub const NEW_SUITES: [&str; 6] = [
> +    "bookworm",
> +    "trixie",
> +    "testing",
> +    "unstable",
> +    "sid",
> +    "experimental",
> +];
> +
>  /// Splits the suite into its base part and variant.
> -fn suite_variant(suite: &str) -> (&str, &str) {
> +pub fn suite_variant(suite: &str) -> (&str, &str) {
>      let variants = ["-backports-sloppy", "-backports", "-updates", "/updates"];
>  
>      for variant in variants.iter() {
> @@ -19,7 +45,7 @@ fn suite_variant(suite: &str) -> (&str, &str) {
>  }
>  
>  /// Get the host part from a given URI.
> -fn host_from_uri(uri: &str) -> Option<&str> {
> +pub fn host_from_uri(uri: &str) -> Option<&str> {
>      if let Some(begin) = uri.find("://") {
>          let mut host = uri.split_at(begin + 3).1;
>  
> @@ -145,34 +171,13 @@ impl APTRepository {
>      /// Checks if old or unstable suites are configured and also that the
>      /// `stable` keyword is not used.
>      fn check_suites(&self, add_info: &mut dyn FnMut(String, String)) {
> -        let old_suites = [
> -            "lenny",
> -            "squeeze",
> -            "wheezy",
> -            "jessie",
> -            "stretch",
> -            "oldoldstable",
> -            "oldstable",
> -        ];
> -
> -        let next_suite = "bullseye";
> -
> -        let new_suites = [
> -            "bookworm",
> -            "trixie",
> -            "testing",
> -            "unstable",
> -            "sid",
> -            "experimental",
> -        ];
> -
>          if self
>              .types
>              .iter()
>              .any(|package_type| *package_type == APTRepositoryPackageType::Deb)
>          {
>              for suite in self.suites.iter() {
> -                if old_suites
> +                if OLD_SUITES
>                      .iter()
>                      .any(|base_suite| suite_variant(suite).0 == *base_suite)
>                  {
> @@ -182,14 +187,14 @@ impl APTRepository {
>                      );
>                  }
>  
> -                if suite_variant(suite).0 == next_suite {
> +                if suite_variant(suite).0 == NEXT_STABLE_SUITE {
>                      add_info(
>                          "ignore-pre-upgrade-warning".to_string(),
>                          format!("suite '{}' should not be used in production!", suite),
>                      );
>                  }
>  
> -                if new_suites
> +                if NEW_SUITES
>                      .iter()
>                      .any(|base_suite| suite_variant(suite).0 == *base_suite)
>                  {
> diff --git a/src/repositories/mod.rs b/src/repositories/mod.rs
> index 2c01011..eceede3 100644
> --- a/src/repositories/mod.rs
> +++ b/src/repositories/mod.rs
> @@ -1,4 +1,5 @@
>  use std::collections::BTreeMap;
> +use std::io::{BufRead, BufReader};
>  use std::path::PathBuf;
>  
>  use anyhow::{bail, format_err, Error};
> @@ -21,6 +22,11 @@ mod writer;
>  const APT_SOURCES_LIST_FILENAME: &str = "/etc/apt/sources.list";
>  const APT_SOURCES_LIST_DIRECTORY: &str = "/etc/apt/sources.list.d/";
>  
> +/// The codename of the current stable Debian release.
> +pub const STABLE_SUITE: &str = check::STABLE_SUITE;
> +/// The codename of the next stable Debian release or `None` if an upgrade is not yet possible.
> +pub const UPGRADE_SUITE: Option<&str> = None;
> +
>  impl APTRepository {
>      /// Crates an empty repository.
>      fn new(file_type: APTRepositoryFileType) -> Self {
> @@ -265,6 +271,92 @@ pub fn repositories() -> Result<(Vec<APTRepositoryFile>, Vec<APTRepositoryFileEr
>      Ok((files, errors))
>  }
>  
> +/// Read the `VERSION_CODENAME` from `/etc/os-release`.
> +fn get_release_codename() -> Result<String, Error> {
> +    let raw = std::fs::read("/etc/os-release")
> +        .map_err(|err| format_err!("unable to read '/etc/os-release' - {}", err))?;
> +
> +    let reader = BufReader::new(&*raw);
> +
> +    for line in reader.lines() {
> +        let line = line.map_err(|err| format_err!("unable to read '/etc/os-release' - {}", err))?;
> +
> +        if let Some(codename) = line.strip_prefix("VERSION_CODENAME=") {
> +            let codename = codename.trim_matches(&['"', '\''][..]);
> +            return Ok(codename.to_string());
> +        }
> +    }
> +
> +    bail!("unable to parse codename from '/etc/os-release'");
> +}
> +
> +/// For enabled repositories, replaces each occurence of the `STABLE_SUITE` with the
> +/// `UPGRADE_SUITE` suite, including variants (e.g. `-updates`).
> +///
> +/// Returns an error if the `UPGRADE_SUITE` is currently `None`, i.e. upgrade not yet possible.
> +///
> +/// Returns an error if the `VERSION_CODENAME` from `/etc/os-release` is not `STABLE_SUITE`.
> +///
> +/// Also handles the special case `buster/updates` -> `bullseye-security` when the URI is
> +/// security.debian.org, but fails if there's additional URIs.
> +pub fn release_upgrade(files: &mut [APTRepositoryFile]) -> Result<(), Error> {
> +    let upgrade_suite = match UPGRADE_SUITE {
> +        Some(suite) => suite,
> +        None => bail!("release upgrade is not yet possible"),
> +    };
> +
> +    let current = get_release_codename()?;
> +
> +    if current == upgrade_suite {
> +        bail!("already installed '{}'", current);
> +    }
> +
> +    if current != STABLE_SUITE {
> +        bail!(
> +            "unexpected release '{}' - cannot prepare repositories for upgrade",
> +            current
> +        );
> +    }
> +
> +    for file in files.iter_mut() {
> +        for repo in file.repositories.iter_mut() {
> +            if !repo.enabled {
> +                continue;
> +            }
> +
> +            for i in 0..repo.suites.len() {
> +                let suite = &repo.suites[i];
> +
> +                // FIXME special case for security repository can be removed for Debian Bookworm
> +
> +                let is_security_uri = |uri| {
> +                    check::host_from_uri(uri).map_or(false, |host| host == "security.debian.org")

this should probably also check for the not uncommon case of

  https://deb.debian.org/debian-security (or http)

> +                };
> +
> +                let has_security_uri = repo.uris.iter().any(|uri| is_security_uri(uri));
> +                let has_only_security_uri = repo.uris.iter().all(|uri| is_security_uri(uri));
> +
> +                if suite == "buster/updates" && has_security_uri {
> +                    if !has_only_security_uri {
> +                        bail!("cannot replace 'buster/updates' suite - multiple URIs");
> +                    }
> +
> +                    repo.suites[i] = "bullseye-security".to_string();
> +
> +                    continue;
> +                }
> +
> +                let (base, variant) = check::suite_variant(suite);
> +                if base == STABLE_SUITE {
> +                    repo.suites[i] = format!("{}{}", upgrade_suite, variant);
> +                }
> +            }
> +        }
> +    }
> +
> +    Ok(())
> +}
> +
>  /// Write the repositories for each file.
>  ///
>  /// Returns an error for each file that could not be written successfully.
> diff --git a/tests/repositories.rs b/tests/repositories.rs
> index 3919077..ee7f1a8 100644
> --- a/tests/repositories.rs
> +++ b/tests/repositories.rs
> @@ -4,7 +4,7 @@ use anyhow::{bail, format_err, Error};
>  
>  use proxmox_apt::repositories::{
>      check_repositories, common_digest, enterprise_repository_enabled,
> -    no_subscription_repository_enabled, write_repositories,
> +    no_subscription_repository_enabled, release_upgrade, write_repositories,
>  };
>  use proxmox_apt::types::{APTRepositoryFile, APTRepositoryInfo};
>  
> @@ -292,3 +292,80 @@ fn test_common_digest() -> Result<(), Error> {
>  
>      Ok(())
>  }
> +
> +#[test]
> +fn test_release_upgrade() -> Result<(), Error> {
> +    let test_dir = std::env::current_dir()?.join("tests");
> +    let read_dir = test_dir.join("sources.list.d");
> +    let write_dir = test_dir.join("sources.list.d.upgraded.actual");
> +    let expected_dir = test_dir.join("sources.list.d.upgraded.expected");
> +
> +    if write_dir.is_dir() {
> +        std::fs::remove_dir_all(&write_dir)
> +            .map_err(|err| format_err!("unable to remove dir {:?} - {}", write_dir, err))?;
> +    }
> +
> +    std::fs::create_dir_all(&write_dir)
> +        .map_err(|err| format_err!("unable to create dir {:?} - {}", write_dir, err))?;
> +
> +    let mut files = vec![];
> +    let mut errors = vec![];
> +
> +    for entry in std::fs::read_dir(read_dir)? {
> +        let path = entry?.path();
> +
> +        match APTRepositoryFile::new(&path)? {
> +            Some(mut file) => match file.parse() {
> +                Ok(()) => files.push(file),
> +                Err(err) => errors.push(err),
> +            },
> +            None => bail!("unexpected None for '{:?}'", path),
> +        }
> +    }
> +
> +    assert!(errors.is_empty());
> +
> +    for file in files.iter_mut() {
> +        let path = PathBuf::from(&file.path);
> +        let new_path = write_dir.join(path.file_name().unwrap());
> +        file.path = new_path.into_os_string().into_string().unwrap();
> +        file.digest = None;
> +    }
> +
> +    let res = release_upgrade(&mut files);
> +
> +    // FIXME adapt test after branching off the stable-X branch!
> +    assert!(res.is_err());
> +    if res.is_err() {
> +        return Ok(());
> +    }
> +
> +    write_repositories(&files).map_err(|err| format_err!("{:?}", err))?;
> +
> +    let mut expected_count = 0;
> +
> +    for entry in std::fs::read_dir(expected_dir)? {
> +        expected_count += 1;
> +
> +        let expected_path = entry?.path();
> +        let actual_path = write_dir.join(expected_path.file_name().unwrap());
> +
> +        let expected_contents = std::fs::read(&expected_path)
> +            .map_err(|err| format_err!("unable to read {:?} - {}", expected_path, err))?;
> +
> +        let actual_contents = std::fs::read(&actual_path)
> +            .map_err(|err| format_err!("unable to read {:?} - {}", actual_path, err))?;
> +
> +        assert_eq!(
> +            expected_contents, actual_contents,
> +            "Use\n\ndiff {:?} {:?}\n\nif you're not fluent in byte decimals",
> +            expected_path, actual_path
> +        );
> +    }
> +
> +    let actual_count = std::fs::read_dir(write_dir)?.count();
> +
> +    assert_eq!(expected_count, actual_count);
> +
> +    Ok(())
> +}
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>