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

[Fabian Ebner <f.ebner@proxmox.com>]

Am 17.06.21 um 16:16 schrieb Fabian Grünbichler:
> 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)
> 

Thanks for the info. The Debian wiki[0] only mentions 
security.debian.org, so I wasn't aware of that. And somebody might have 
a FQDN here too...

[0]: https://wiki.debian.org/NewInBullseye

>> +                };
>> +
>> +                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
>>
>>
>>
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>