compression: Refactor handling of compression formats

In preparation for adding zstd support to rustup, this rework
means that we treat compression formats we have available (and know)
generically without breaking if unexpected formats are present,
or if no format we know is present.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>

Author: kinnison

src/dist/manifest.rs

@@ -47,16 +47,34 @@ pub enum PackageTargets {
 
 #[derive(Clone, Debug, PartialEq)]
 pub struct TargetedPackage {
-    pub bins: Option<PackageBins>,
+    pub bins: Vec<(CompressionKind, HashedBinary)>,
     pub components: Vec<Component>,
 }
 
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub enum CompressionKind {
+    GZip,
+    XZ,
+}
+
+/// Each compression kind, in order of preference for use, from most desirable
+/// to least desirable.
+static COMPRESSION_KIND_PREFERENCE_ORDER: &[CompressionKind] =
+    &[CompressionKind::XZ, CompressionKind::GZip];
+
+impl CompressionKind {
+    const fn key_prefix(self) -> &'static str {
+        match self {
+            Self::GZip => "",
+            Self::XZ => "xz_",
+        }
+    }
+}
+
 #[derive(Clone, Debug, PartialEq)]
-pub struct PackageBins {
+pub struct HashedBinary {
     pub url: String,
     pub hash: String,
-    pub xz_url: Option<String>,
-    pub xz_hash: Option<String>,
 }
 
 #[derive(Clone, Debug, Eq, Ord, PartialOrd)]
@@ -417,19 +435,21 @@ impl TargetedPackage {
         )?);
 
         if get_bool(&mut table, "available", path)? {
-            Ok(Self {
-                bins: Some(PackageBins {
-                    url: get_string(&mut table, "url", path)?,
-                    hash: get_string(&mut table, "hash", path)?,
-                    xz_url: get_string(&mut table, "xz_url", path).ok(),
-                    xz_hash: get_string(&mut table, "xz_hash", path).ok(),
-                }),
-                components,
-            })
+            let mut bins = Vec::new();
+            for kind in COMPRESSION_KIND_PREFERENCE_ORDER.iter().copied() {
+                let url_key = format!("{}url", kind.key_prefix());
+                let hash_key = format!("{}hash", kind.key_prefix());
+                let url = get_string(&mut table, &url_key, path).ok();
+                let hash = get_string(&mut table, &hash_key, path).ok();
+                if let (Some(url), Some(hash)) = (url, hash) {
+                    bins.push((kind, HashedBinary { url, hash }));
+                }
+            }
+            Ok(Self { bins, components })
         } else {
             Ok(Self {
-                bins: None,
-                components: vec![],
+                bins: Vec::new(),
+                components: Vec::new(),
             })
         }
     }
@@ -442,22 +462,22 @@ impl TargetedPackage {
         if !extensions.is_empty() {
             result.insert("extensions".to_owned(), toml::Value::Array(extensions));
         }
-        if let Some(bins) = self.bins {
-            result.insert("hash".to_owned(), toml::Value::String(bins.hash));
-            result.insert("url".to_owned(), toml::Value::String(bins.url));
-            if let (Some(xz_hash), Some(xz_url)) = (bins.xz_hash, bins.xz_url) {
-                result.insert("xz_hash".to_owned(), toml::Value::String(xz_hash));
-                result.insert("xz_url".to_owned(), toml::Value::String(xz_url));
+        if self.bins.is_empty() {
+            result.insert("available".to_owned(), toml::Value::Boolean(false));
+        } else {
+            for (kind, bin) in self.bins {
+                let url_key = format!("{}url", kind.key_prefix());
+                let hash_key = format!("{}hash", kind.key_prefix());
+                result.insert(url_key, toml::Value::String(bin.url));
+                result.insert(hash_key, toml::Value::String(bin.hash));
             }
             result.insert("available".to_owned(), toml::Value::Boolean(true));
-        } else {
-            result.insert("available".to_owned(), toml::Value::Boolean(false));
         }
         result
     }
 
     pub fn available(&self) -> bool {
-        self.bins.is_some()
+        !self.bins.is_empty()
     }
 
     fn toml_to_components(

src/dist/manifestation.rs

@@ -11,7 +11,7 @@ use crate::dist::component::{Components, Package, TarGzPackage, TarXzPackage, Tr
 use crate::dist::config::Config;
 use crate::dist::dist::{Profile, TargetTriple, DEFAULT_DIST_SERVER};
 use crate::dist::download::{DownloadCfg, File};
-use crate::dist::manifest::{Component, Manifest, TargetedPackage};
+use crate::dist::manifest::{Component, CompressionKind, Manifest, TargetedPackage};
 use crate::dist::notifications::*;
 use crate::dist::prefix::InstallPrefix;
 use crate::dist::temp;
@@ -22,11 +22,6 @@ use crate::utils::utils;
 pub const DIST_MANIFEST: &str = "multirust-channel-manifest.toml";
 pub const CONFIG_FILE: &str = "multirust-config.toml";
 
-enum Format {
-    Gz,
-    Xz,
-}
-
 #[derive(Debug)]
 pub struct Manifestation {
     installation: Components,
@@ -153,7 +148,7 @@ impl Manifestation {
         let altered = temp_cfg.dist_server != DEFAULT_DIST_SERVER;
 
         // Download component packages and validate hashes
-        let mut things_to_install: Vec<(Component, Format, File)> = Vec::new();
+        let mut things_to_install: Vec<(Component, CompressionKind, File)> = Vec::new();
         let mut things_downloaded: Vec<String> = Vec::new();
         let components = update.components_urls_and_hashes(new_manifest)?;
 
@@ -246,11 +241,11 @@ impl Manifestation {
             let reader =
                 utils::FileReaderWithProgress::new_file(&installer_file, &notification_converter)?;
             let package: &dyn Package = match format {
-                Format::Gz => {
+                CompressionKind::GZip => {
                     gz = TarGzPackage::new(reader, temp_cfg, Some(&notification_converter))?;
                     &gz
                 }
-                Format::Xz => {
+                CompressionKind::XZ => {
                     xz = TarXzPackage::new(reader, temp_cfg, Some(&notification_converter))?;
                     &xz
                 }
@@ -683,28 +678,24 @@ impl Update {
     fn components_urls_and_hashes(
         &self,
         new_manifest: &Manifest,
-    ) -> Result<Vec<(Component, Format, String, String)>> {
+    ) -> Result<Vec<(Component, CompressionKind, String, String)>> {
         let mut components_urls_and_hashes = Vec::new();
         for component in &self.components_to_install {
             let package = new_manifest.get_package(&component.short_name_in_manifest())?;
             let target_package = package.get_target(component.target.as_ref())?;
 
-            let bins = match target_package.bins {
-                None => continue,
-                Some(ref bins) => bins,
-            };
-            let c_u_h = if let (Some(url), Some(hash)) = (bins.xz_url.clone(), bins.xz_hash.clone())
-            {
-                (component.clone(), Format::Xz, url, hash)
-            } else {
-                (
-                    component.clone(),
-                    Format::Gz,
-                    bins.url.clone(),
-                    bins.hash.clone(),
-                )
-            };
-            components_urls_and_hashes.push(c_u_h);
+            if target_package.bins.is_empty() {
+                // This package is not available, no files to download.
+                continue;
+            }
+            // We prefer the first format in the list, since the parsing of the
+            // manifest leaves us with the files/hash pairs in preference order.
+            components_urls_and_hashes.push((
+                component.clone(),
+                target_package.bins[0].0,
+                target_package.bins[0].1.url.clone(),
+                target_package.bins[0].1.hash.clone(),
+            ));
         }
 
         Ok(components_urls_and_hashes)

tests/cli-v2.rs

@@ -1051,7 +1051,7 @@ fn make_component_unavailable(config: &Config, name: &str, target: &str) {
         let std_pkg = manifest.packages.get_mut(name).unwrap();
         let target = TargetTriple::new(target);
         let target_pkg = std_pkg.targets.get_mut(&target).unwrap();
-        target_pkg.bins = None;
+        target_pkg.bins = Vec::new();
     }
     let manifest_str = manifest.stringify();
     rustup::utils::raw::write_file(&manifest_path, &manifest_str).unwrap();

tests/dist_manifest.rs

@@ -27,8 +27,8 @@ fn parse_smoke_test() {
         .get_target(Some(&x86_64_unknown_linux_gnu))
         .unwrap();
     assert_eq!(rust_target_pkg.available(), true);
-    assert_eq!(rust_target_pkg.bins.clone().unwrap().url, "example.com");
-    assert_eq!(rust_target_pkg.bins.clone().unwrap().hash, "...");
+    assert_eq!(rust_target_pkg.bins[0].1.url, "example.com");
+    assert_eq!(rust_target_pkg.bins[0].1.hash, "...");
 
     let component = &rust_target_pkg.components[0];
     assert_eq!(component.short_name_in_manifest(), "rustc");
@@ -42,7 +42,7 @@ fn parse_smoke_test() {
     let docs_target_pkg = docs_pkg
         .get_target(Some(&x86_64_unknown_linux_gnu))
         .unwrap();
-    assert_eq!(docs_target_pkg.bins.clone().unwrap().url, "example.com");
+    assert_eq!(docs_target_pkg.bins[0].1.url, "example.com");
 }
 
 #[test]