Merge pull request #2675 from kinnison/tarball-compression-flexibility

compression: Refactor handling of compression formats

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]