From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id 2A118ACB4 for ; Wed, 28 Jun 2023 12:15:16 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0A81E21DD0 for ; Wed, 28 Jun 2023 12:14:46 +0200 (CEST) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Wed, 28 Jun 2023 12:14:44 +0200 (CEST) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id D4CC740AB4 for ; Wed, 28 Jun 2023 12:14:43 +0200 (CEST) Message-ID: <5723714f-673f-e0d5-a7d8-ee12a793117d@proxmox.com> Date: Wed, 28 Jun 2023 12:14:41 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.12.0 Content-Language: en-US From: Lukas Wagner To: Proxmox Backup Server development discussion , Christian Ebner Reply-To: Proxmox Backup Server development discussion References: <20230628081005.48122-1-c.ebner@proxmox.com> <3b490122-c2a8-aade-b09c-bf83f74926c8@proxmox.com> In-Reply-To: <3b490122-c2a8-aade-b09c-bf83f74926c8@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL -0.087 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment NICE_REPLY_A -0.103 Looks like a legit reply (A) SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH backup stable-2] pbs2to3: add upgrade checker binary X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jun 2023 10:15:16 -0000 Oh, seems that I have missed v2 - but I think many comments still apply. On 6/28/23 12:13, Lukas Wagner wrote: > Hi, > > tested on current `stable-2` branch. Seems to work just fine, at least on a clean PBS installation. > > Some comments regarding the code are inline (nothing major though). > > Tested-by: Lukas Wagner > Reviewed-by: Lukas Wagner > > On 6/28/23 10:10, Christian Ebner wrote: >> Adds the pbs2to3 upgrade checker with some basic checks. >> >> Signed-off-by: Christian Ebner >> --- >>   Makefile                             |   1 + >>   debian/proxmox-backup-server.install |   2 + >>   src/bin/pbs2to3.rs                   | 602 +++++++++++++++++++++++++++ >>   zsh-completions/_pbs2to3             |  13 + >>   4 files changed, 618 insertions(+) >>   create mode 100644 src/bin/pbs2to3.rs >>   create mode 100644 zsh-completions/_pbs2to3 >> >> diff --git a/Makefile b/Makefile >> index b307009d..7477935d 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -19,6 +19,7 @@ USR_BIN := \ >>   USR_SBIN := \ >>       proxmox-backup-manager \ >>       proxmox-backup-debug \ >> +    pbs2to3 >>   # Binaries for services: >>   SERVICE_BIN := \ >> diff --git a/debian/proxmox-backup-server.install b/debian/proxmox-backup-server.install >> index 76f50cd0..ebe51aae 100644 >> --- a/debian/proxmox-backup-server.install >> +++ b/debian/proxmox-backup-server.install >> @@ -11,6 +11,7 @@ usr/lib/x86_64-linux-gnu/proxmox-backup/proxmox-daily-update >>   usr/lib/x86_64-linux-gnu/proxmox-backup/sg-tape-cmd >>   usr/sbin/proxmox-backup-debug >>   usr/sbin/proxmox-backup-manager >> +usr/sbin/pbs2to3 >>   usr/bin/pmtx >>   usr/bin/pmt >>   usr/bin/proxmox-tape >> @@ -39,3 +40,4 @@ usr/share/zsh/vendor-completions/_proxmox-backup-manager >>   usr/share/zsh/vendor-completions/_proxmox-tape >>   usr/share/zsh/vendor-completions/_pmtx >>   usr/share/zsh/vendor-completions/_pmt >> +usr/share/zsh/vendor-completions/_pbs2to3 >> diff --git a/src/bin/pbs2to3.rs b/src/bin/pbs2to3.rs >> new file mode 100644 >> index 00000000..97d064cd >> --- /dev/null >> +++ b/src/bin/pbs2to3.rs >> @@ -0,0 +1,602 @@ >> +use std::io::Write; >> +use std::path::Path; >> + >> +use anyhow::{format_err, Error}; >> +use regex::Regex; >> +use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor}; >> + >> +use proxmox_apt::repositories::{self, APTRepositoryFile}; >> +use proxmox_backup::api2::node::apt; >> + >> +const OLD_SUITE: &str = "bullseye"; >> +const NEW_SUITE: &str = "bookworm"; >> +const PROXMOX_BACKUP_META: &str = "proxmox-backup"; >> +const MIN_PBS_MAJOR: u8 = 2; >> +const MIN_PBS_MINOR: u8 = 4; >> +const MIN_PBS_PKGREL: u8 = 1; >> + >> +fn main() -> Result<(), Error> { >> +    let mut checker = Checker::new(); >> +    checker.check_pbs_packages()?; >> +    checker.check_misc()?; >> +    checker.summary()?; >> +    Ok(()) >> +} >> + >> +struct Checker { >> +    output: ConsoleOutput, >> +    upgraded: bool, >> +} >> + >> +impl Checker { >> +    pub fn new() -> Self { >> +        Self { >> +            output: ConsoleOutput::new(), >> +            upgraded: false, >> +        } >> +    } >> + > > The `check_pbs_packages` function is a bit long and hard to read, maybe break this > into multiple subroutines? Generally I prefer to have functions that fit entirely on screen without > scrolling, unless there is a good reason against it. >> +    pub fn check_pbs_packages(&mut self) -> Result<(), Error> { >> +        self.output.print_header("CHECKING VERSION INFORMATION FOR PBS PACKAGES")?; >> +        self.output.log_info("Checking for package updates..")?; >> + >> +        let result = Self::get_upgradable_packages(); >> +        match result { >> +            Err(err) => { >> +                self.output.log_warn(format!("{}", err).as_str())?; >                                                          ^ > Nit: You could inline the variable here: `format!("{err}")` > Also applies to many other locations in the code, I'll refrain from highlighting the other occurences ;). > > >> +                self.output.log_fail("unable to retrieve list of package updates!")?; >> +            } >> +            Ok(cache) => { >> +                if cache.package_status.is_empty() { >> +                    self.output.log_pass("all packages up-to-date")?; >> +                } else { >> +                    let pkgs = cache.package_status.iter() >> +                        .map(|pkg| format!("{}, ", pkg.package.clone())) >> +                        .collect::()> +                    self.output.log_warn( >> +                        format!( >> +                            "updates for the following packages are available:\ >> +                            \n      {}", >> +                            &pkgs[..pkgs.len()-2] >> +                        ).as_str() > You could change the signatures for the `log_*` functions to > >     pub fn log_pass>(&mut self, message: T) -> Result<(), Error> { >         self.log_line(LogLevel::Pass, message.as_ref()) >     } > > -> then the function can take a `String` as a well as `&str`, allowing you to omit the > `as_str()` calls in various places. > >> +                    )?; >> +                } >> +            } >> +        } >> + >> +        self.output.log_info("Checking proxmox backup server package version..")?; >> +        let pkg_versions = apt::get_versions()?; >> + >> +        let pbs_meta_pkg = pkg_versions.iter().find(|pkg| { >> +            pkg.package.as_str() == PROXMOX_BACKUP_META >> +        }); >> + >> +        if let Some(pbs_meta_pkg) = pbs_meta_pkg { >> +            let pkg_version = Regex::new(r"^(\d+)\.(\d+)[.-](\d+)")?; >> +            let captures = pkg_version.captures(&pbs_meta_pkg.old_version); >> +            if let Some(captures) = captures { >> +                let maj = Self::extract_version_from_captures(1, &captures)?; >> +                let min = Self::extract_version_from_captures(2, &captures)?; >> +                let pkgrel = Self::extract_version_from_captures(3, &captures)?; >> + >> +                if maj > MIN_PBS_MAJOR { >> +                    self.output.log_pass( >> +                        format!("Already upgraded to Proxmox Backup Server {}", maj).as_str() >> +                    )?; >> +                    self.upgraded = true; >> +                } else if maj >= MIN_PBS_MAJOR && min >= MIN_PBS_MINOR && pkgrel >= MIN_PBS_PKGREL { >> +                    self.output.log_pass( >> +                        format!( >> +                            "'{}' has version >= {}.{}-{}", >> +                            PROXMOX_BACKUP_META, >> +                            MIN_PBS_MAJOR, >> +                            MIN_PBS_MINOR, >> +                            MIN_PBS_PKGREL, >> +                        ).as_str() >> +                    )?; >> +                } else { >> +                    self.output.log_fail( >> +                        format!( >> +                            "'{}' package is too old, please upgrade to >= {}.{}-{}", >> +                            PROXMOX_BACKUP_META, >> +                            MIN_PBS_MAJOR, >> +                            MIN_PBS_MINOR, >> +                            MIN_PBS_PKGREL, >> +                        ).as_str() >> +                    )?; >> +                } >> +            } else { >> +                self.output.log_fail( >> +                    format!( >> +                        "could not match the '{}' package version, is it installed?", >> +                        PROXMOX_BACKUP_META, >> +                    ).as_str() >> +                )?; >> +            } >> + >> +            self.output.log_info("Check running kernel version..")?; >> +            let mut krunning = Regex::new(r"^6\.(?:2\.(?:[2-9]\d+|1[6-8]|1\d\d+)|5)[^~]*$")?; >> +            let mut kinstalled = "pve-kernel-6.2"; >> +            if !self.upgraded { >> +                krunning = Regex::new(r"^(?:5\.(?:13|15)|6\.2)")?; >> +                kinstalled = "pve-kernel-5.15"; >> +            } > > Nit: I'd probably write something like: > > let (krunning, kinstalled) = if self.upgraded { >     ( >       Regex::new(...), >       "pve-kernel-..." >     ) > } else { >    ... > } > > That way you can avoid the `mut` and also avoid compiling a regex that is not actually used. > >> + >> +            let mut command = std::process::Command::new("uname"); >> +            command.arg("-r"); >> +            match command.output() { > > Nit: Also here you could avoid the `mut` and use the `Command` in a chained fashion: > > let output = Command::new(..) >     .arg(...) >     .output(); > > match output { >     ... > } > >> +                Err(_err) => self.output.log_fail("unable to determine running kernel version.")?, >> +                Ok(ret) => { >> +                    let running_version = std::str::from_utf8(&ret.stdout[..ret.stdout.len()-1])?; >> +                    if krunning.is_match(running_version) { >> +                        if self.upgraded { >> +                            self.output.log_pass( >> +                                format!( >> +                                    "running new kernel '{}' after upgrade.", >> +                                    running_version, >> +                                ).as_str() >> +                            )?; >> +                        } else { >> +                            self.output.log_pass( >> +                                format!( >> +                                    "running kernel '{}' is considered suitable for upgrade.", >> +                                    running_version, >> +                                ).as_str() >> +                            )?; >> +                        } >> +                    } else { >> +                        let installed_kernel = pkg_versions.iter().find(|pkg| { >> +                            pkg.package.as_str() == kinstalled >> +                        }); >> +                        if installed_kernel.is_some() { >> +                            self.output.log_warn( >> +                                format!( >> +                                    "a suitable kernel '{}' is installed, but an unsuitable '{}' \ >> +                                    is booted, missing reboot?!", >> +                                    kinstalled, >> +                                    running_version, >> +                                ).as_str() >> +                            )?; >> +                        } else { >> +                            self.output.log_warn( >> +                                format!( >> +                                    "unexpected running and installed kernel '{}'.", >> +                                    running_version, >> +                                ).as_str() >> +                            )?; >> +                        } >> +                    } >> +                } >> +            } >> + >> +        } else { >> +            self.output.log_fail(format!("'{}' package not found!", PROXMOX_BACKUP_META).as_str())?; >> +        } >> + >> +        Ok(()) >> +    } >> + >> +    fn extract_version_from_captures(index: usize, captures: ®ex::Captures) -> Result { >> +        if let Some(capture) = captures.get(index) { >> +            let val = capture.as_str().parse::()?; >> +            Ok(val) >> +        } else { >> +            Ok(0) >> +        } >> +    } >> + >> +    fn check_bootloader(&mut self) -> Result<(), Error> { >> +        self.output.log_info("Checking bootloader configuration...")?; >> + >> +        // PBS packages version check needs to be run before >> +        if !self.upgraded { >> +            self.output.log_skip("not yet upgraded, no need to check the presence of systemd-boot")?; >> +        } >> + >> +        if !Path::is_file(Path::new("/etc/kernel/proxmox-boot-uuids")) { >                      ^ > `Path::is_file(&self)` takes `&self` as parameter, so I'd just write: > > `if !Path::new("...").is_file() {` > >> +            self.output.log_skip("proxmox-boot-tool not used for bootloader configuration")?; >> +            return Ok(()); >> +        } >> + >> +        if !Path::is_dir(Path::new("/sys/firmware/efi")) { > Here as well. >> +            self.output.log_skip("System booted in legacy-mode - no need for systemd-boot")?; >> +            return Ok(()); >> +        }/ >> + >> +        if Path::is_file(Path::new("/usr/share/doc/systemd-boot/changelog.Debian.gz")) { > Here as well. >> +            self.output.log_pass("systemd-boot is installed")?; >> +        } else { >> +            self.output.log_warn( >> +                "proxmox-boot-tool is used for bootloader configuration in uefi mode \ >> +                 but the separate systemd-boot package, existing in Debian Bookworm \ >> +                 is not installed.\n\ >> +                 initializing new ESPs will not work unitl the package is installed." >> +            )?; >> +        } >> +        Ok(()) >> +    } >> + >> +    fn check_apt_repos(&mut self) -> Result<(), Error> { >> +        self.output.log_info("Checking for package repository suite mismatches..")?; >> + >> +        let mut strange = false; >> +        let mut mismatches = Vec::new(); >> + >> +        let (repo_files, _repo_errors, _digest) = repositories::repositories()?; >> +        for repo_file in repo_files { >> +            let (found_strange, mut found_mismatches) = self.check_repo_file(repo_file)?; >> +            if found_strange { >> +                strange = true; >> +            } >> +            mismatches.append(&mut found_mismatches); >> +        } >> + >> +        match (mismatches.is_empty(), strange) { >> +            (true, false) => self.output.log_pass("found no suite mismatch")?, >> +            (true, true) => self.output.log_notice( >> +                "found no suite mismatches, but found at least one strange suite" >> +            )?, >> +            (false, _) => { >> +                let mut message = String::from( >> +                    "Found mixed old and new packages repository suites, fix before upgrading!\ >> +                    \n      Mismatches:" >> +                ); >> +                for (suite, location) in mismatches.iter() { >> +                    message.push_str( >> +                        format!("\n      found suite '{}' at '{}'", suite, location).as_str() >> +                    ); >> +                } >> +                message.push('\n'); >> +                self.output.log_fail(message.as_str())? >> +            } >> +        } >> + >> +        Ok(()) >> +    } >> + >> +    pub fn check_misc(&mut self) -> Result<(), Error> { >> +        self.output.print_header("MISCELLANEOUS CHECKS")?; >> +        self.check_pbs_services()?; >> +        self.check_time_sync()?; >> +        self.check_apt_repos()?; >> +        self.check_bootloader()?; >> +        Ok(()) >> +    } >> + >> +    pub fn summary(&mut self) -> Result<(), Error> { >> +        self.output.print_summary() >> +    } >> + > > The function below is deeply nested (up to 7 levels, if I counted correctly), maybe break > stuff out into smaller functions? >> +    fn check_repo_file( >> +        &mut self, >> +        repo_file: APTRepositoryFile >> +    ) -> Result<(bool, Vec<(String, String)>), Error> { >> +        let deb_suite = Regex::new(r"(\w+)(-updates|-backports|-security)?$")?; >> +        let mut strange_suite = false; >> +        let mut found_suite: Option<(String, String)> = None; >> +        let mut mismatches = Vec::new(); >> + >> +        for repo in repo_file.repositories { >> +            for suite in &repo.suites { >> +                if let Some(suite) = deb_suite.captures(suite) { >> +                    if let Some(suite_match) = suite.get(1) { >> +                        let suite = suite_match.as_str(); >> +                        let location = repo_file.path.clone().unwrap_or(String::new()); >                                                                               ^ > clippy: You can use `unwrap_or_default` here. > >> + >> +                        if suite != OLD_SUITE && suite != NEW_SUITE { >> +                            self.output.log_notice(format!( >> +                                "found unusual suite '{}', neighter old '{}' nor new '{}'..\ >> +                                 \n        Affected file {}\ >> +                                 \n        Please assure this is shipping compatible packages for \ >> +                                 the upgrade!", >> +                                 suite, >> +                                 OLD_SUITE, >> +                                 NEW_SUITE, >> +                                 location, >> +                            ).as_str())?; >> +                            strange_suite = true; >> +                            continue; >> +                        } >> + >> +                        if let Some((ref current_suite, ref current_location)) = found_suite { >> +                            if suite != current_suite { >> +                                if mismatches.is_empty() { >> +                                    mismatches.push( >> +                                        (current_suite.clone(), current_location.clone()) >> +                                    ); >> +                                    mismatches.push((suite.to_string(), location)); >> +                                } else { >> +                                    mismatches.push((suite.to_string(), location)); >> +                                } >> +                            } >> +                        } else { >> +                            found_suite = Some((suite.to_string(), location)); >> +                        } >> +                    } >> +                } >> +            } >> +        } >> + >> +        Ok((strange_suite, mismatches)) >> +    } >> + >> +    fn get_systemd_unit_state( >> +        &mut self, >> +        unit: &str, >> +    ) -> Result<(SystemdUnitState, SystemdUnitState), Error> { >> +        let mut command = std::process::Command::new("systemctl"); >> +        command.arg("is-enabled"); >> +        command.arg(unit); >> +        let output = command.output().map_err(|err| { >> +            format_err!("failed to execute {:?} - {}", command, err) >> +        })?; >> + >> +        let is_enabled_state = match output.stdout.as_slice() { >> +            b"enabled\n" => SystemdUnitState::Enabled, >> +            b"disabled\n" => SystemdUnitState::Disabled, >> +            _ => SystemdUnitState::Unknown, >> +        }; >> + >> +        let mut command = std::process::Command::new("systemctl"); >> +        command.arg("is-active"); >> +        command.arg(unit); >> +        let output = command.output().map_err(|err| { >> +            format_err!("failed to execute {:?} - {}", command, err) >> +        })?; >> + >> +        let is_active_state = match output.stdout.as_slice() { >> +            b"active\n" => SystemdUnitState::Active, >> +            b"inactive\n" => SystemdUnitState::Inactive, >> +            b"failed\n" => SystemdUnitState::Failed, >> +            _ => SystemdUnitState::Unknown, >> +        }; >> +        Ok((is_enabled_state, is_active_state)) >> +    } >> + >> +    fn check_pbs_services(&mut self) -> Result<(), Error> { >> +        self.output.log_info("Checking pbs daemon services..")?; >> + >> +        for service in ["proxmox-backup.service", "proxmox-backup-proxy.service"] { >> +            match self.get_systemd_unit_state(service)? { >> +                (_, SystemdUnitState::Active) => { >> +                    self.output.log_pass( >> +                        format!("systemd unit '{}' is in state 'active'", service).as_str() >> +                    )?; >> +                } >> +                (_, SystemdUnitState::Inactive) => { >> +                    self.output.log_fail( >> +                        format!( >> +                            "systemd unit '{}' is in state 'inactive'\ >> +                            \n    Please check the service for errors and start it.", >> +                            service, >> +                        ).as_str() >> +                    )?; >> +                } >> +                (_, SystemdUnitState::Failed) => { >> +                    self.output.log_fail( >> +                        format!( >> +                            "systemd unit '{}' is in state 'failed'\ >> +                            \n    Please check the service for errors and start it.", >> +                            service, >> +                        ).as_str() >> +                    )?; >> +                } >> +                (_, _) => { >> +                    self.output.log_fail( >> +                        format!( >> +                            "systemd unit '{}' is not in state 'active'\ >> +                            \n    Please check the service for errors and start it.", >> +                            service, >> +                        ).as_str() >> +                    )?; >> +                } >> +            } >> +        } >> +        Ok(()) >> +    } >> + >> +    fn check_time_sync(&mut self) -> Result<(), Error> { >> +        self.output.log_info("Checking for supported & active NTP service..")?; >> +        if self.get_systemd_unit_state("systemd-timesyncd.service")?.1 == SystemdUnitState::Active { >> +            self.output.log_warn( >> +                "systemd-timesyncd is not the best choice for time-keeping on servers, due to only \ >> +                applying updates on boot.\ >> +                \n       While not necessary for the upgrade it's recommended to use one of:\ >> +                \n        * chrony (Default in new Proxmox VE installations)\ >> +                \n        * ntpsec\ >> +                \n        * openntpd" >> +            )?; >> +        } else if self.get_systemd_unit_state("ntp.service")?.1 == SystemdUnitState::Active { >> +            self.output.log_info( >> +                "Debian deprecated and removed the ntp package for Bookworm, but the system \ >> +                will automatically migrate to the 'ntpsec' replacement package on upgrade." >> +            )?; >> +        } else if self.get_systemd_unit_state("chrony.service")?.1 == SystemdUnitState::Active || >> +            self.get_systemd_unit_state("openntpd.service")?.1 == SystemdUnitState::Active || >> +            self.get_systemd_unit_state("ntpsec.service")?.1 == SystemdUnitState::Active >> +        { >> +            self.output.log_pass("Detected active time synchronisation unit")?; >> +        } else { >> +            self.output.log_warn( >> +                "No (active) time synchronisation daemon (NTP) detected, but synchronized systems \ >> +                are important!" >> +            )?; >> +        } >> +        Ok(()) >> +    } >> + >> +    fn get_upgradable_packages() -> Result { >> +        let cache = if let Ok(false) = proxmox_backup::tools::apt::pkg_cache_expired() { >> +            if let Ok(Some(cache)) = proxmox_backup::tools::apt::read_pkg_state() { >> +                cache >> +            } else { >> +                proxmox_backup::tools::apt::update_cache()? >> +            } >> +        } else { >> +            proxmox_backup::tools::apt::update_cache()? >> +        }; >> + >> +        Ok(cache) >> +    } >> +} >> + >> +#[derive(PartialEq)] >> +enum SystemdUnitState { >> +    Active, >> +    Enabled, >> +    Disabled, >> +    Failed, >> +    Inactive, >> +    Unknown, >> +} >> + >> +#[derive(Default)] >> +struct Counters { >> +    pass: u64, >> +    skip: u64, >> +    notice: u64, >> +    warn: u64, >> +    fail: u64, >> +} >> + >> +enum LogLevel { >> +    Pass, >> +    Info, >> +    Skip, >> +    Notice, >> +    Warn, >> +    Fail, >> +} >> + >> +struct ConsoleOutput { >> +    stream: StandardStream, >> +    first_header: bool, >> +    counters: Counters, >> +} >> + >> +impl ConsoleOutput { >> +    pub fn new() -> Self { >> +        Self { >> +            stream: StandardStream::stdout(ColorChoice::Always), >> +            first_header: true, >> +            counters: Counters::default(), >> +        } >> +    } >> + >> +    pub fn print_header(&mut self, message: &str) -> Result<(), Error> { >> +        if !self.first_header { >> +            writeln!(&mut self.stream, "")?; >                                                ^ > clippy: You can remove the empty string if you just want to write a new line. > >> +        } >> +        self.first_header = false; >> +        writeln!(&mut self.stream, "= {message} =\n")?; >> +        Ok(()) >> +    } >> + >> +    pub fn set_color(&mut self, color: Color) -> Result<(), Error> { >> +        self.stream.set_color(ColorSpec::new().set_fg(Some(color)))?; >> +        Ok(()) >> +    } >> + >> +    pub fn log_line(&mut self, level: LogLevel, message: &str) -> Result<(), Error> { >> +        match level { >> +            LogLevel::Pass => { >> +                self.counters.pass += 1; >> +                self.set_color(Color::Green)?; >> +                writeln!(&mut self.stream, "PASS: {}", message)?; >> +                self.set_color(Color::White)?; >> +            } >> +            LogLevel::Info => { >> +                writeln!(&mut self.stream, "INFO: {}", message)?; >> +            } >> +            LogLevel::Skip => { >> +                self.counters.skip += 1; >> +                writeln!(&mut self.stream, "SKIP: {}", message)?; >> +            } >> +            LogLevel::Notice => { >> +                self.counters.notice += 1; >> +                writeln!(&mut self.stream, "NOTICE: {}", message)?; >> +            } >> +            LogLevel::Warn=> { >> +                self.counters.warn += 1; >> +                self.set_color(Color::Yellow)?; >> +                writeln!(&mut self.stream, "WARN: {}", message)?; >> +                self.set_color(Color::White)?; >> +            } >> +            LogLevel::Fail => { >> +                self.counters.fail += 1; >> +                self.set_color(Color::Red)?; >> +                writeln!(&mut self.stream, "FAIL: {}", message)?; >> +                self.set_color(Color::White)?; >> +            } >> +        } >> +        Ok(()) >> +    } >> + >> +    pub fn log_pass(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Pass, message) >> +    } >> + >> +    pub fn log_info(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Info, message) >> +    } >> + >> +    pub fn log_skip(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Skip, message) >> +    } >> + >> +    pub fn log_notice(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Notice, message) >> +    } >> + >> +    pub fn log_warn(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Warn, message) >> +    } >> + >> +    pub fn log_fail(&mut self, message: &str) -> Result<(), Error> { >> +        self.log_line(LogLevel::Fail, message) >> +    } >> + >> +    pub fn print_summary(&mut self) -> Result<(), Error> { >> +        self.print_header("SUMMARY")?; >> + >> +        let total = self.counters.fail + self.counters.pass + self.counters.notice + >> +            self.counters.skip + self.counters.warn; >> + >> +        self.set_color(Color::White)?; >> +        writeln!(&mut self.stream, "TOTAL:     {total}")?; >> +        self.set_color(Color::Green)?; >> +        writeln!(&mut self.stream, "PASSED:    {}", self.counters.pass)?; >> +        self.set_color(Color::White)?; >> +        writeln!(&mut self.stream, "SKIPPED:   {}", self.counters.skip)?; >> +        writeln!(&mut self.stream, "NOTICE:    {}", self.counters.notice)?; >> +        if self.counters.warn > 0 { >> +            self.set_color(Color::Yellow)?; >> +            writeln!(&mut self.stream, "WARNINGS:  {}", self.counters.warn)?; >> +        } >> +        if self.counters.fail > 0 { >> +            self.set_color(Color::Red)?; >> +            writeln!(&mut self.stream, "FAILURES:  {}", self.counters.fail)?; >> +        } >> +        if self.counters.warn > 0 || self.counters.fail > 0 { >> +            let mut color = Color::Yellow; >> +            if self.counters.fail > 0 { >> +                color = Color::Red; >> +            } >> + >> +            self.set_color(color)?; >> +            writeln!( >> +                &mut self.stream, >> +                "\nATTENTION: Please check the output for detailed information!", >> +            )?; >> +            if self.counters.fail > 0 { >> +                writeln!( >> +                    &mut self.stream, >> +                    "Try to solve the problems one at a time and rerun this checklist tool again.", >> +                )?; >> +            } >> +        } >> +        self.set_color(Color::White)?; >> +        Ok(()) >> +    } >> +} >> + >> diff --git a/zsh-completions/_pbs2to3 b/zsh-completions/_pbs2to3 >> new file mode 100644 >> index 00000000..f6dc3d67 >> --- /dev/null >> +++ b/zsh-completions/_pbs2to3 >> @@ -0,0 +1,13 @@ >> +#compdef _pbs2to3() pbs2to3 >> + >> +function _pbs2to3() { >> +    local cwords line point cmd curr prev >> +    cwords=${#words[@]} >> +    line=$words >> +    point=${#line} >> +    cmd=${words[1]} >> +    curr=${words[cwords]} >> +    prev=${words[cwords-1]} >> +    compadd -- $(COMP_CWORD="$cwords" COMP_LINE="$line" COMP_POINT="$point" \ >> +        pbs2to3 bashcomplete "$cmd" "$curr" "$prev") >> +} > -- - Lukas