fix(app): make MC <1.12.2 downloadable again (#4494)

PR #4270 modified the internal `fetch` function used by the application
to download version artifacts in a way that 4xx HTTP errors also caused
an abnormal return, instead of just 5xx errors. That was a good change,
but it had the unintended side effect of exposing our faulty logic
elsewhere of trying to download non-native JAR library artifacts when
only native artifacts are appropriate, at least according to the
PrismLauncher source code I've read. Such a download always returned a
404 error, but because such error was considered successful, a dummy
library file was still created and things worked seemingly fine.

These changes bring the Modrinth App behavior in this regard more in
line with PrismLauncher's, avoiding downloading non-native artifacts for
dependencies that have native artifacts available. (Reference:
8b5e91920d/launcher/minecraft/Library.cpp (L163))

I've tested these changes to work successfully with a variety of old
vanilla and modded Minecraft versions.

Fixes #4464.

packages/app-lib/src/launcher/args.rs

@@ -15,7 +15,7 @@ use daedalus::{
 };
 use dunce::canonicalize;
 use itertools::Itertools;
-use std::io::{BufRead, BufReader};
+use std::io::{BufRead, BufReader, ErrorKind};
 use std::net::SocketAddr;
 use std::{collections::HashMap, path::Path};
 use uuid::Uuid;
@@ -60,7 +60,11 @@ pub fn get_class_paths(
                 return None;
             }
 
-            Some(get_lib_path(libraries_path, &library.name, false))
+            Some(get_lib_path(
+                libraries_path,
+                &library.name,
+                library.natives_os_key_and_classifiers(java_arch).is_some(),
+            ))
         }))
         .process_results(|iter| {
             iter.unique().join(classpath_separator(java_arch))
@@ -85,21 +89,21 @@ pub fn get_lib_path(
     lib: &str,
     allow_not_exist: bool,
 ) -> crate::Result<String> {
-    let path = libraries_path
+    let path = libraries_path.join(get_path_from_artifact(lib)?);
-        .to_path_buf()
-        .join(get_path_from_artifact(lib)?);
 
-    if !path.exists() && allow_not_exist {
+    let path = match canonicalize(&path) {
-        return Ok(path.to_string_lossy().to_string());
+        Ok(p) => p,
-    }
+        Err(err) if err.kind() == ErrorKind::NotFound && allow_not_exist => {
-
+            path
-    let path = &canonicalize(&path).map_err(|_| {
+        }
-        crate::ErrorKind::LauncherError(format!(
+        Err(err) => {
-            "Library file at path {} does not exist",
+            return Err(crate::ErrorKind::LauncherError(format!(
-            path.to_string_lossy()
+                "Could not canonicalize library path {}: {err}",
-        ))
+                path.display()
-        .as_error()
+            ))
-    })?;
+            .as_error());
+        }
+    };
 
     Ok(path.to_string_lossy().to_string())
 }

packages/app-lib/src/launcher/download.rs

@@ -8,13 +8,13 @@ use crate::{
         emit::{emit_loading, loading_try_for_each_concurrent},
     },
     state::State,
-    util::{fetch::*, io, platform::OsExt},
+    util::{fetch::*, io},
 };
 use daedalus::minecraft::{LoggingConfiguration, LoggingSide};
 use daedalus::{
     self as d,
     minecraft::{
-        Asset, AssetsIndex, Library, Os, Version as GameVersion,
+        Asset, AssetsIndex, Library, Version as GameVersion,
         VersionInfo as GameVersionInfo,
     },
     modded::LoaderVersion,
@@ -288,90 +288,132 @@ pub async fn download_libraries(
     }?;
     let num_files = libraries.len();
     loading_try_for_each_concurrent(
-        stream::iter(libraries.iter())
+        stream::iter(libraries.iter()).map(Ok::<&Library, crate::Error>),
-            .map(Ok::<&Library, crate::Error>), None, loading_bar,loading_amount,num_files, None,|library| async move {
+        None,
-                if let Some(rules) = &library.rules
+        loading_bar,
-                    && !parse_rules(rules, java_arch, &QuickPlayType::None, minecraft_updated) {
+        loading_amount,
-                        tracing::trace!("Skipped library {}", &library.name);
+        num_files,
-                        return Ok(());
+        None,
-                    }
+        |library| async move {
+            if let Some(rules) = &library.rules
+                && !parse_rules(
+                    rules,
+                    java_arch,
+                    &QuickPlayType::None,
+                    minecraft_updated,
+                )
+            {
+                tracing::trace!("Skipped library {}", &library.name);
+                return Ok(());
+            }
 
-                if !library.downloadable {
+            if !library.downloadable {
-                    tracing::trace!("Skipped non-downloadable library {}", &library.name);
+                tracing::trace!(
+                    "Skipped non-downloadable library {}",
+                    &library.name
+                );
+                return Ok(());
+            }
+
+            // When a library has natives, we only need to download such natives, as PrismLauncher does
+            if let Some((os_key, classifiers)) =
+                library.natives_os_key_and_classifiers(java_arch)
+            {
+                let parsed_key = os_key
+                    .replace("${arch}", crate::util::platform::ARCH_WIDTH);
+
+                if let Some(native) = classifiers.get(&parsed_key) {
+                    let data = fetch(
+                        &native.url,
+                        Some(&native.sha1),
+                        &st.fetch_semaphore,
+                        &st.pool,
+                    )
+                    .await?;
+
+                    if let Ok(mut archive) =
+                        zip::ZipArchive::new(std::io::Cursor::new(&data))
+                    {
+                        match archive.extract(
+                            st.directories.version_natives_dir(version),
+                        ) {
+                            Ok(_) => tracing::debug!(
+                                "Fetched native {}",
+                                &library.name
+                            ),
+                            Err(err) => tracing::error!(
+                                "Failed extracting native {}. err: {err}",
+                                &library.name
+                            ),
+                        }
+                    } else {
+                        tracing::error!(
+                            "Failed extracting native {}",
+                            &library.name
+                        );
+                    }
+                }
+            } else {
+                let artifact_path = d::get_path_from_artifact(&library.name)?;
+                let path = st.directories.libraries_dir().join(&artifact_path);
+
+                if path.exists() && !force {
                     return Ok(());
                 }
 
-                tokio::try_join! {
+                if let Some(d::minecraft::LibraryDownloads {
-                    async {
+                    artifact: Some(ref artifact),
-                        let artifact_path = d::get_path_from_artifact(&library.name)?;
+                    ..
-                        let path = st.directories.libraries_dir().join(&artifact_path);
+                }) = library.downloads
+                    && !artifact.url.is_empty()
+                {
+                    let bytes = fetch(
+                        &artifact.url,
+                        Some(&artifact.sha1),
+                        &st.fetch_semaphore,
+                        &st.pool,
+                    )
+                    .await?;
+                    write(&path, &bytes, &st.io_semaphore).await?;
 
-                        if path.exists() && !force {
+                    tracing::trace!(
-                            return Ok(());
+                        "Fetched library {} to path {:?}",
-                        }
+                        &library.name,
+                        &path
+                    );
+                } else {
+                    // We lack an artifact URL, so fall back to constructing one ourselves.
+                    // PrismLauncher just ignores the library if this is the case, so it's
+                    // probably not needed, but previous code revisions of the Modrinth App
+                    // intended to do this, so we keep that behavior for compatibility.
 
-                        if let Some(d::minecraft::LibraryDownloads { artifact: Some(ref artifact), ..}) = library.downloads
+                    let url = format!(
-                            && !artifact.url.is_empty(){
+                        "{}{artifact_path}",
-                                let bytes = fetch(&artifact.url, Some(&artifact.sha1), &st.fetch_semaphore, &st.pool)
+                        library
-                                    .await?;
+                            .url
-                                write(&path, &bytes, &st.io_semaphore).await?;
+                            .as_deref()
-                                tracing::trace!("Fetched library {} to path {:?}", &library.name, &path);
+                            .unwrap_or("https://libraries.minecraft.net/")
-                                return Ok::<_, crate::Error>(());
+                    );
-                            }
 
-                        let url = [
+                    let bytes =
-                            library
+                        fetch(&url, None, &st.fetch_semaphore, &st.pool)
-                                .url
+                            .await?;
-                                .as_deref()
-                                .unwrap_or("https://libraries.minecraft.net/"),
-                            &artifact_path
-                        ].concat();
 
-                        let bytes = fetch(&url, None, &st.fetch_semaphore, &st.pool).await?;
+                    write(&path, &bytes, &st.io_semaphore).await?;
-                        write(&path, &bytes, &st.io_semaphore).await?;
-                        tracing::trace!("Fetched library {} to path {:?}", &library.name, &path);
-                        Ok::<_, crate::Error>(())
-                    },
-                    async {
-                        // HACK: pseudo try block using or else
-                        if let Some((os_key, classifiers)) = None.or_else(|| Some((
-                            library
-                                .natives
-                                .as_ref()?
-                                .get(&Os::native_arch(java_arch))?,
-                            library
-                                .downloads
-                                .as_ref()?
-                                .classifiers
-                                .as_ref()?
-                        ))) {
-                            let parsed_key = os_key.replace(
-                                "${arch}",
-                                crate::util::platform::ARCH_WIDTH,
-                            );
 
-                            if let Some(native) = classifiers.get(&parsed_key) {
+                    tracing::trace!(
-                                let data = fetch(&native.url, Some(&native.sha1), &st.fetch_semaphore, &st.pool).await?;
+                        "Fetched library {} to path {:?}",
-                                let reader = std::io::Cursor::new(&data);
+                        &library.name,
-                                if let Ok(mut archive) = zip::ZipArchive::new(reader) {
+                        &path
-                                    match archive.extract(st.directories.version_natives_dir(version)) {
+                    );
-                                        Ok(_) => tracing::debug!("Fetched native {}", &library.name),
+                }
-                                        Err(err) => tracing::error!("Failed extracting native {}. err: {}", &library.name, err)
-                                    }
-                                } else {
-                                    tracing::error!("Failed extracting native {}", &library.name)
-                                }
-                            }
-                        }
-
-                        Ok(())
-                    }
-                }?;
-
-                tracing::debug!("Loaded library {}", library.name);
-                Ok(())
             }
-        ).await?;
+
+            tracing::debug!("Loaded library {}", library.name);
+            Ok(())
+        },
+    )
+    .await?;
 
     tracing::debug!("Done loading libraries!");
     Ok(())

packages/app-lib/src/util/platform.rs

@@ -1,65 +1,6 @@
 //! Platform-related code
 use daedalus::minecraft::{Os, OsRule};
 
-// OS detection
-pub trait OsExt {
-    /// Get the OS of the current system
-    fn native() -> Self;
-
-    /// Gets the OS + Arch of the current system
-    fn native_arch(java_arch: &str) -> Self;
-
-    /// Gets the OS from an OS + Arch
-    fn get_os(&self) -> Self;
-}
-
-impl OsExt for Os {
-    fn native() -> Self {
-        match std::env::consts::OS {
-            "windows" => Self::Windows,
-            "macos" => Self::Osx,
-            "linux" => Self::Linux,
-            _ => Self::Unknown,
-        }
-    }
-
-    fn native_arch(java_arch: &str) -> Self {
-        if std::env::consts::OS == "windows" {
-            if java_arch == "aarch64" {
-                Os::WindowsArm64
-            } else {
-                Os::Windows
-            }
-        } else if std::env::consts::OS == "linux" {
-            if java_arch == "aarch64" {
-                Os::LinuxArm64
-            } else if java_arch == "arm" {
-                Os::LinuxArm32
-            } else {
-                Os::Linux
-            }
-        } else if std::env::consts::OS == "macos" {
-            if java_arch == "aarch64" {
-                Os::OsxArm64
-            } else {
-                Os::Osx
-            }
-        } else {
-            Os::Unknown
-        }
-    }
-
-    fn get_os(&self) -> Self {
-        match self {
-            Os::OsxArm64 => Os::Osx,
-            Os::LinuxArm32 => Os::Linux,
-            Os::LinuxArm64 => Os::Linux,
-            Os::WindowsArm64 => Os::Windows,
-            _ => self.clone(),
-        }
-    }
-}
-
 // Bit width
 #[cfg(target_pointer_width = "64")]
 pub const ARCH_WIDTH: &str = "64";

packages/daedalus/src/minecraft.rs

@@ -179,6 +179,56 @@ pub enum Os {
     Unknown,
 }
 
+impl Os {
+    /// Returns the native OS of the build
+    pub fn native() -> Self {
+        match std::env::consts::OS {
+            "windows" => Self::Windows,
+            "macos" => Self::Osx,
+            "linux" => Self::Linux,
+            _ => Self::Unknown,
+        }
+    }
+
+    /// Returns the native OS variant of the build, taking into account the architecture of its Java runtime
+    pub fn native_arch(java_arch: &str) -> Self {
+        if std::env::consts::OS == "windows" {
+            if java_arch == "aarch64" {
+                Os::WindowsArm64
+            } else {
+                Os::Windows
+            }
+        } else if std::env::consts::OS == "linux" {
+            if java_arch == "aarch64" {
+                Os::LinuxArm64
+            } else if java_arch == "arm" {
+                Os::LinuxArm32
+            } else {
+                Os::Linux
+            }
+        } else if std::env::consts::OS == "macos" {
+            if java_arch == "aarch64" {
+                Os::OsxArm64
+            } else {
+                Os::Osx
+            }
+        } else {
+            Os::Unknown
+        }
+    }
+
+    /// Returns the base OS of a variant (e.g. OsxArm64 -> Osx)
+    pub fn get_os(&self) -> Self {
+        match self {
+            Os::OsxArm64 => Os::Osx,
+            Os::LinuxArm32 => Os::Linux,
+            Os::LinuxArm64 => Os::Linux,
+            Os::WindowsArm64 => Os::Windows,
+            _ => self.clone(),
+        }
+    }
+}
+
 #[derive(Serialize, Deserialize, Debug, Clone)]
 /// A rule which depends on what OS the user is on
 pub struct OsRule {
@@ -277,6 +327,24 @@ pub struct Library {
     pub downloadable: bool,
 }
 
+impl Library {
+    /// Returns the OS key and classifiers for downloading natives, if applicable
+    pub fn natives_os_key_and_classifiers(
+        &self,
+        java_arch: &str,
+    ) -> Option<(&str, &HashMap<String, LibraryDownload>)> {
+        self.natives
+            .as_ref()
+            .and_then(|natives| natives.get(&Os::native_arch(java_arch)))
+            .and_then(|natives| {
+                self.downloads
+                    .as_ref()
+                    .and_then(|downloads| downloads.classifiers.as_ref())
+                    .map(|classifiers| (natives.as_str(), classifiers))
+            })
+    }
+}
+
 #[derive(Deserialize, Debug, Clone)]
 /// A partial library which should be merged with a full library
 pub struct PartialLibrary {