From 003e75ec8e309bd991099fb275e569c6b24b2ccd Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Sun, 14 Jul 2024 13:28:56 +0200 Subject: [PATCH 1/4] Make arch optional in descriptor --- lib/descriptor/mod.rs | 46 +++++++++++++++++++++---------------------- src/runner/info.rs | 2 +- 2 files changed, 24 insertions(+), 24 deletions(-) diff --git a/lib/descriptor/mod.rs b/lib/descriptor/mod.rs index 8fd29df..af75747 100644 --- a/lib/descriptor/mod.rs +++ b/lib/descriptor/mod.rs @@ -29,7 +29,7 @@ pub enum DescriptionParseError { #[derive(Debug, Clone, Copy, PartialEq, Eq)] pub struct Descriptor { os: OS, - arch: Arch, + arch: Option, toolchain: Option, } @@ -41,7 +41,7 @@ impl Descriptor { pub fn current_system() -> Self { Self { os: OS::current_system(), - arch: Arch::current_system(), + arch: Some(Arch::current_system()), toolchain: Toolchain::current_system(), } } @@ -55,7 +55,7 @@ impl Descriptor { let search_string = search_string.as_ref(); let os = OS::detect(search_string)?; - let arch = Arch::detect(search_string).unwrap_or_default(); + let arch = Arch::detect(search_string); let toolchain = Toolchain::detect(search_string); Some(Self { @@ -76,7 +76,7 @@ impl Descriptor { let (os, arch) = parse_executable(binary_contents)?; Some(Self { os, - arch, + arch: Some(arch), toolchain: None, }) } @@ -93,7 +93,7 @@ impl Descriptor { Get the architecture of this description. */ #[must_use] - pub fn arch(&self) -> Arch { + pub fn arch(&self) -> Option { self.arch } @@ -125,9 +125,9 @@ impl Descriptor { // ... or special cases for architecture compatibility || matches!( (self.os, self.arch, other.arch), - (OS::Windows, Arch::X64, Arch::X86) - | (OS::Linux, Arch::X64, Arch::X86) - | (OS::MacOS, Arch::Arm64, Arch::X64) + (OS::Windows, Some(Arch::X64), Some(Arch::X86)) + | (OS::Linux, Some(Arch::X64), Some(Arch::X86)) + | (OS::MacOS, Some(Arch::Arm64), Some(Arch::X64)) ) ) } @@ -170,7 +170,7 @@ impl FromStr for Descriptor { type Err = DescriptionParseError; fn from_str(s: &str) -> Result { let os = OS::detect(s).ok_or(DescriptionParseError::OS)?; - let arch = Arch::detect(s).unwrap_or_default(); + let arch = Arch::detect(s); let toolchain = Toolchain::detect(s); Ok(Self { @@ -201,7 +201,7 @@ mod tests { fn current_description() { let current = Descriptor::current_system(); assert_eq!(current.os, OS::current_system()); - assert_eq!(current.arch, Arch::current_system()); + assert_eq!(current.arch, Some(Arch::current_system())); assert_eq!(current.toolchain, Toolchain::current_system()); } @@ -212,7 +212,7 @@ mod tests { "windows-x64-msvc", Descriptor { os: OS::Windows, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: Some(Toolchain::Msvc), }, ); @@ -220,7 +220,7 @@ mod tests { "win64", Descriptor { os: OS::Windows, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: None, }, ); @@ -228,7 +228,7 @@ mod tests { "windows-x86-gnu", Descriptor { os: OS::Windows, - arch: Arch::X86, + arch: Some(Arch::X86), toolchain: Some(Toolchain::Gnu), }, ); @@ -236,7 +236,7 @@ mod tests { "windows-x86", Descriptor { os: OS::Windows, - arch: Arch::X86, + arch: Some(Arch::X86), toolchain: None, }, ); @@ -244,7 +244,7 @@ mod tests { "win32", Descriptor { os: OS::Windows, - arch: Arch::X86, + arch: Some(Arch::X86), toolchain: None, }, ); @@ -253,7 +253,7 @@ mod tests { "aarch64-macos", Descriptor { os: OS::MacOS, - arch: Arch::Arm64, + arch: Some(Arch::Arm64), toolchain: None, }, ); @@ -261,7 +261,7 @@ mod tests { "macos-x64-gnu", Descriptor { os: OS::MacOS, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: Some(Toolchain::Gnu), }, ); @@ -269,7 +269,7 @@ mod tests { "macos-x64", Descriptor { os: OS::MacOS, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: None, }, ); @@ -278,7 +278,7 @@ mod tests { "linux-x86_64-gnu", Descriptor { os: OS::Linux, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: Some(Toolchain::Gnu), }, ); @@ -286,7 +286,7 @@ mod tests { "linux-gnu-x86", Descriptor { os: OS::Linux, - arch: Arch::X86, + arch: Some(Arch::X86), toolchain: Some(Toolchain::Gnu), }, ); @@ -294,7 +294,7 @@ mod tests { "armv7-linux-musl", Descriptor { os: OS::Linux, - arch: Arch::Arm32, + arch: Some(Arch::Arm32), toolchain: Some(Toolchain::Musl), }, ); @@ -307,7 +307,7 @@ mod tests { "macos-universal", Descriptor { os: OS::MacOS, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: None, }, ); @@ -315,7 +315,7 @@ mod tests { "darwin-universal", Descriptor { os: OS::MacOS, - arch: Arch::X64, + arch: Some(Arch::X64), toolchain: None, }, ); diff --git a/src/runner/info.rs b/src/runner/info.rs index 5b286cf..9d232f0 100644 --- a/src/runner/info.rs +++ b/src/runner/info.rs @@ -20,7 +20,7 @@ fn is_likely_rosetta2_error(e: &Error) -> bool { let is_running_macos_aarch64 = { let current = Descriptor::current_system(); - matches!(current.os(), OS::MacOS) && matches!(current.arch(), Arch::Arm64) + matches!(current.os(), OS::MacOS) && matches!(current.arch(), Some(Arch::Arm64)) }; is_bad_cpu_type && is_running_macos_aarch64 From 1d75c13aad40b063428b584f21a61adefbc75126 Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Sun, 14 Jul 2024 13:37:50 +0200 Subject: [PATCH 2/4] Add method for finding partially compatible artifact --- lib/sources/artifact.rs | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/lib/sources/artifact.rs b/lib/sources/artifact.rs index 55ea948..e6c401f 100644 --- a/lib/sources/artifact.rs +++ b/lib/sources/artifact.rs @@ -245,4 +245,38 @@ impl Artifact { .map(|(_, artifact)| artifact.clone()) .collect() } + + /** + Tries to find a partially compatible artifact, to be used as a fallback + during artifact selection if [`Artifact::sort_by_system_compatibility`] + finds no system-compatible artifacts to use. + + Returns `None` if more than one artifact is partially compatible. + */ + pub fn find_partially_compatible_fallback(artifacts: impl AsRef<[Self]>) -> Option { + let current_desc = Descriptor::current_system(); + + let os_compatible_artifacts = artifacts + .as_ref() + .iter() + .filter_map(|artifact| { + let name = artifact.name.as_deref()?; + if let Some(asset_desc) = Descriptor::detect(name) { + if current_desc.os() == asset_desc.os() { + Some(artifact) + } else { + None + } + } else { + None + } + }) + .collect::>(); + + if os_compatible_artifacts.len() == 1 { + Some(os_compatible_artifacts[0].clone()) + } else { + None + } + } } From 31c9d823c4be27ea903e0de69af3603859006df9 Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Sun, 14 Jul 2024 14:15:02 +0200 Subject: [PATCH 3/4] Implement user and maintainer-friendly artifact selection fallback in CLI --- src/cli/add.rs | 15 +++------ src/cli/install.rs | 9 ++---- src/cli/self_update.rs | 8 ++--- src/cli/update.rs | 15 ++++----- src/util/artifacts.rs | 73 ++++++++++++++++++++++++++++++++++++++++++ src/util/mod.rs | 2 ++ 6 files changed, 92 insertions(+), 30 deletions(-) create mode 100644 src/util/artifacts.rs diff --git a/src/cli/add.rs b/src/cli/add.rs index 792e0b7..d6c3120 100644 --- a/src/cli/add.rs +++ b/src/cli/add.rs @@ -5,12 +5,13 @@ use console::style; use rokit::{ discovery::discover_all_manifests, manifests::RokitManifest, - sources::Artifact, storage::Home, tool::{ToolAlias, ToolId}, }; -use crate::util::{prompt_for_trust, CliProgressTracker, ToolIdOrSpec}; +use crate::util::{ + find_most_compatible_artifact, prompt_for_trust, CliProgressTracker, ToolIdOrSpec, +}; /// Adds a new tool to Rokit and installs it. #[derive(Debug, Parser)] @@ -85,18 +86,12 @@ impl AddSubcommand { let (spec, artifact) = match self.tool.clone() { ToolIdOrSpec::Spec(spec) => { let artifacts = source.get_specific_release(&spec).await?; - let artifact = Artifact::sort_by_system_compatibility(&artifacts) - .first() - .cloned() - .with_context(|| format!("No compatible artifact found for {id}"))?; + let artifact = find_most_compatible_artifact(&artifacts, &id)?; (spec, artifact) } ToolIdOrSpec::Id(id) => { let artifacts = source.get_latest_release(&id).await?; - let artifact = Artifact::sort_by_system_compatibility(&artifacts) - .first() - .cloned() - .with_context(|| format!("No compatible artifact found for {id}"))?; + let artifact = find_most_compatible_artifact(&artifacts, &id)?; (artifact.tool_spec.clone(), artifact) } }; diff --git a/src/cli/install.rs b/src/cli/install.rs index 595ff53..28325a8 100644 --- a/src/cli/install.rs +++ b/src/cli/install.rs @@ -5,9 +5,9 @@ use clap::Parser; use console::style; use futures::{stream::FuturesUnordered, TryStreamExt}; -use rokit::{discovery::discover_all_manifests, sources::Artifact, storage::Home}; +use rokit::{discovery::discover_all_manifests, storage::Home}; -use crate::util::{prompt_for_trust_specs, CliProgressTracker}; +use crate::util::{find_most_compatible_artifact, prompt_for_trust_specs, CliProgressTracker}; /// Adds a new tool using Rokit and installs it. #[derive(Debug, Parser)] @@ -84,10 +84,7 @@ impl InstallSubcommand { let artifacts = source.get_specific_release(&tool_spec).await?; pt.subtask_completed(); - let artifact = Artifact::sort_by_system_compatibility(&artifacts) - .first() - .cloned() - .with_context(|| format!("No compatible artifact found for {tool_spec}"))?; + let artifact = find_most_compatible_artifact(&artifacts, tool_spec.id())?; pt.subtask_completed(); let contents = source diff --git a/src/cli/self_update.rs b/src/cli/self_update.rs index e503112..92e9a20 100644 --- a/src/cli/self_update.rs +++ b/src/cli/self_update.rs @@ -3,9 +3,9 @@ use clap::Parser; use console::style; use semver::Version; -use rokit::{sources::Artifact, storage::Home, tool::ToolId}; +use rokit::{storage::Home, tool::ToolId}; -use crate::util::CliProgressTracker; +use crate::util::{find_most_compatible_artifact, CliProgressTracker}; /// Updates Rokit to the latest version. #[derive(Debug, Parser)] @@ -55,9 +55,7 @@ impl SelfUpdateSubcommand { pt.task_completed(); pt.update_message("Downloading"); - let artifact = Artifact::sort_by_system_compatibility(&artifacts) - .first() - .cloned() + let artifact = find_most_compatible_artifact(&artifacts, &tool_id) .context("No compatible Rokit artifact was found (WAT???)")?; let artifact_contents = source .download_artifact_contents(&artifact) diff --git a/src/cli/update.rs b/src/cli/update.rs index d243295..5a2bfa4 100644 --- a/src/cli/update.rs +++ b/src/cli/update.rs @@ -3,11 +3,11 @@ use clap::Parser; use console::style; use futures::{stream::FuturesUnordered, TryStreamExt}; -use rokit::{ - discovery::discover_all_manifests, manifests::RokitManifest, sources::Artifact, storage::Home, -}; +use rokit::{discovery::discover_all_manifests, manifests::RokitManifest, storage::Home}; -use crate::util::{CliProgressTracker, ToolAliasOrIdOrSpec, ToolIdOrSpec}; +use crate::util::{ + find_most_compatible_artifact, CliProgressTracker, ToolAliasOrIdOrSpec, ToolIdOrSpec, +}; /// Updates all tools, or specific tools, to the latest version. #[derive(Debug, Parser)] @@ -141,12 +141,9 @@ impl UpdateSubcommand { } }; - let artifact = Artifact::sort_by_system_compatibility(&artifacts) - .first() - .cloned() - .with_context(|| format!("No compatible artifact found for {id}"))?; - + let artifact = find_most_compatible_artifact(&artifacts, &id)?; pt.subtask_completed(); + Ok::<_, anyhow::Error>((alias, id, artifact)) }) .collect::>() diff --git a/src/util/artifacts.rs b/src/util/artifacts.rs new file mode 100644 index 0000000..00376f2 --- /dev/null +++ b/src/util/artifacts.rs @@ -0,0 +1,73 @@ +use anyhow::{Context, Result}; + +use rokit::{ + descriptor::Descriptor, + sources::{Artifact, ArtifactProvider}, + tool::ToolId, +}; + +pub fn find_most_compatible_artifact(artifacts: &[Artifact], tool_id: &ToolId) -> Result { + let mut artifact_opt = Artifact::sort_by_system_compatibility(artifacts) + .first() + .cloned(); + + if artifact_opt.is_none() { + let current_desc = Descriptor::current_system(); + + // If we failed to find an artifact compatible with the current system, + // we may be able to give additional information to Rokit's users, or tool + // maintainers who want to be Rokit-compatible, by examining the artifacts + let no_artifacts_with_arch = artifacts.iter().all(|artifact| { + artifact + .name + .as_deref() + .and_then(Descriptor::detect) + .map_or(false, |desc| desc.arch().is_none()) + }); + let additional_information = if no_artifacts_with_arch { + let source_is_github = artifacts + .iter() + .all(|artifact| matches!(artifact.provider, ArtifactProvider::GitHub)); + let source_name = if source_is_github { + "GitHub release files" + } else { + "tool release files" + }; + Some(format!( + "This seems to have been caused by {0} not \ + specifying an architecture in any of its artifacts.\ + \nIf you are the maintainer of this tool, you can resolve \ + this issue by specifying an architecture in {source_name}:\ + \n {0}-{1}-{2}.zip", + tool_id.name(), + current_desc.os().as_str(), + current_desc.arch().expect("no current arch (??)").as_str(), + )) + } else { + None + }; + + // Let the user know about failing to find an artifact, + // potentially with additional information generated above + tracing::warn!( + "Failed to find a fully compatible artifact for {tool_id}!{}\ + \nSearching for a fallback...", + match additional_information { + Some(info) => format!("\n{info}"), + None => String::new(), + } + ); + + if let Some(artifact) = Artifact::find_partially_compatible_fallback(artifacts) { + tracing::info!( + "Found fallback artifact '{}' for tool {tool_id}", + artifact.name.as_deref().unwrap_or("N/A") + ); + artifact_opt.replace(artifact); + } + } + + // If we did not find a compatible artifact, either directly + // or through a fallback mechanism, this should be a hard error + artifact_opt.with_context(|| format!("No compatible artifact found for {tool_id}")) +} diff --git a/src/util/mod.rs b/src/util/mod.rs index 5d8ed38..1e0a9b1 100644 --- a/src/util/mod.rs +++ b/src/util/mod.rs @@ -1,4 +1,5 @@ mod alias_or_id_or_spec; +mod artifacts; mod constants; mod id_or_spec; mod progress; @@ -6,6 +7,7 @@ mod prompts; mod tracing; pub use self::alias_or_id_or_spec::ToolAliasOrIdOrSpec; +pub use self::artifacts::find_most_compatible_artifact; pub use self::id_or_spec::ToolIdOrSpec; pub use self::progress::CliProgressTracker; pub use self::prompts::{prompt_for_trust, prompt_for_trust_specs}; From fbcd1b0d5d4b04031ac33f18b1ebe7e8326ed127 Mon Sep 17 00:00:00 2001 From: Filip Tibell Date: Sun, 14 Jul 2024 14:26:31 +0200 Subject: [PATCH 4/4] Update changelog --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cb02fd0..60b85f4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,9 +12,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Failed to parse tool specifications in foreman.toml if they were an inline table ([#36]) +- Fixed tool specifications failing to parse in `foreman.toml` when using inline tables ([#36]) +- Fixed tools not specifying architectures (such as `wally-macos.zip`) failing to install ([#38]) [#36]: https://github.com/rojo-rbx/rokit/pull/36 +[#38]: https://github.com/rojo-rbx/rokit/pull/38 ## `0.1.4` - July 11th, 2024