SAT solver for requirements (#208)

* SAT solver for requirements

Closes #204

* Add failing examples

* Add test for remote conflict

* Only insert dep found once

* Avoid lookups to repo if possible

* Better conflict output

Author: Keats

.gitignore

@@ -3,6 +3,7 @@
 out
 src/tests/RCMD/
 example_projects/*/rv
+example_projects/test
 example_projects/*/rv.lock
 !example_projects/archive/rv.lock
 from*/

example_projects/url-dep/rproject.toml

@@ -2,7 +2,7 @@
 name = "simple"
 r_version = "4.4"
 repositories = [
-    {alias = "posit", url = "https://packagemanager.posit.co/cran/2024-12-16/"}
+    {alias = "posit", url = "https://packagemanager.posit.co/cran/2024-12-16/", force_source = true}
 ]
 dependencies = [
     {name = "dplyr", url = "https://cran.r-project.org/src/contrib/Archive/dplyr/dplyr_1.1.3.tar.gz"}

src/activate.rs

@@ -97,8 +97,7 @@ fn scripts_as_paths(is_home: bool) -> (PathBuf, PathBuf) {
 
 fn write_activate_file(dir: impl AsRef<Path>, is_home: bool) -> Result<(), ActivateError> {
     let template = ACTIVATE_FILE_TEMPLATE.to_string();
-    let global_wd_content = if is_home
-    {
+    let global_wd_content = if is_home {
         r#"
         owd <- getwd()
         setwd("~")

src/lockfile.rs

@@ -5,12 +5,12 @@ use std::io::Write;
 use std::path::{Path, PathBuf};
 use std::str::FromStr;
 
+use crate::package::Dependency;
+use crate::{ConfigDependency, Repository, ResolvedDependency, Version};
 use fs_err as fs;
 use serde::Deserialize;
 use toml_edit::{Array, ArrayOfTables, InlineTable, Item, Table, Value};
 
-use crate::{ConfigDependency, Repository, ResolvedDependency, Version};
-
 const CURRENT_LOCKFILE_VERSION: i64 = 1;
 const INITIAL_COMMENT: &str = r#"# This file is automatically @generated by rv.
 # It is not intended for manual editing.
@@ -89,6 +89,10 @@ impl Source {
         matches!(self, Source::Git { .. } | Source::Url { .. })
     }
 
+    pub fn is_repo(&self) -> bool {
+        matches!(self, Source::Repository { .. })
+    }
+
     /// The key to use in the cache: URL for a package repository, git URL for a git repository
     /// and for local the actual path
     pub fn source_path(&self) -> &str {
@@ -231,11 +235,11 @@ impl fmt::Display for Source {
     }
 }
 
-fn format_array(deps: &[String]) -> Array {
+fn format_array(deps: &[Dependency]) -> Array {
     let mut deps = deps
         .iter()
         .map(|d| {
-            let mut value = Value::from(d);
+            let mut value = d.as_toml_value();
             value.decor_mut().set_prefix("\n    ");
             value
         })
@@ -257,10 +261,10 @@ pub struct LockedPackage {
     pub source: Source,
     pub path: Option<String>,
     pub force_source: bool,
-    pub dependencies: Vec<String>,
+    pub dependencies: Vec<Dependency>,
     /// Only filled if the package had install_suggests=True in the config file
     #[serde(default)]
-    pub suggests: Vec<String>,
+    pub suggests: Vec<Dependency>,
 }
 
 impl LockedPackage {
@@ -274,9 +278,9 @@ impl LockedPackage {
             dependencies: dep
                 .dependencies
                 .into_iter()
-                .map(|d| d.into_owned())
+                .map(|x| x.into_owned())
                 .collect(),
-            suggests: dep.suggests.into_iter().map(|d| d.into_owned()).collect(),
+            suggests: dep.suggests.into_iter().map(|x| x.into_owned()).collect(),
         }
     }
 
@@ -350,8 +354,8 @@ impl Lockfile {
 
         for p in &self.packages {
             for d in &p.dependencies {
-                if !package_names.contains(d.as_str()) {
-                    not_found.insert(d.as_str());
+                if !package_names.contains(d.name()) {
+                    not_found.insert(d.name());
                 }
             }
         }
@@ -456,7 +460,7 @@ impl Lockfile {
             let mut out = HashSet::new();
             out.insert(p.name.as_str());
             for p in &p.dependencies {
-                out.extend(self.get_package_tree(p.as_str(), None));
+                out.extend(self.get_package_tree(p.name(), None));
             }
             out
         } else {

src/main.rs

@@ -158,9 +158,16 @@ fn resolve_dependencies(context: &CliContext) -> Vec<ResolvedDependency> {
     );
     if !resolution.is_success() {
         eprintln!("Failed to resolve all dependencies");
+        let req_error_messages = resolution.req_error_messages();
+
         for d in resolution.failed {
             eprintln!("    {d}");
         }
+
+        if !req_error_messages.is_empty() {
+            eprintln!("{}", req_error_messages.join("\n"));
+        }
+
         ::std::process::exit(1)
     }
 

src/package/mod.rs

@@ -4,6 +4,7 @@ use serde::{Deserialize, Serialize};
 use std::collections::HashMap;
 use std::fmt;
 use std::path::Path;
+use toml_edit::{InlineTable, Value};
 
 mod description;
 mod parser;
@@ -30,8 +31,9 @@ impl fmt::Display for PackageType {
     }
 }
 
-#[derive(Debug, PartialEq, Clone, Encode, Decode, Serialize, Deserialize)]
-pub(crate) enum Dependency {
+#[derive(Debug, Hash, Eq, PartialEq, Clone, Encode, Decode, Serialize, Deserialize)]
+#[serde(untagged)]
+pub enum Dependency {
     Simple(String),
     Pinned {
         name: String,
@@ -55,6 +57,18 @@ impl Dependency {
             } => Some(requirement),
         }
     }
+
+    pub(crate) fn as_toml_value(&self) -> Value {
+        match self {
+            Self::Simple(name) => Value::from(name.as_str()),
+            Self::Pinned { name, requirement } => {
+                let mut table = InlineTable::new();
+                table.insert("name", Value::from(name.as_str()));
+                table.insert("requirement", Value::from(&requirement.to_string()));
+                Value::InlineTable(table)
+            }
+        }
+    }
 }
 
 #[derive(Debug, Default, PartialEq, Clone, Encode, Decode)]
@@ -76,7 +90,7 @@ pub struct Package {
     pub(crate) remotes: HashMap<String, (Option<String>, PackageRemote)>,
 }
 
-#[derive(Debug, Default, PartialEq, Clone, Serialize)]
+#[derive(Debug, Default, PartialEq, Clone)]
 pub struct InstallationDependencies<'a> {
     pub(crate) direct: Vec<&'a Dependency>,
     pub(crate) suggests: Vec<&'a Dependency>,

src/package/version.rs

@@ -2,9 +2,10 @@ use bincode::{Decode, Encode};
 use serde::{Deserialize, Serialize};
 use std::cmp::Ordering;
 use std::fmt;
+use std::hash::Hash;
 use std::str::FromStr;
 
-#[derive(Debug, PartialEq, Eq, Copy, Clone, Serialize, Deserialize, Encode, Decode)]
+#[derive(Debug, Hash, PartialEq, Eq, Copy, Clone, Serialize, Deserialize, Encode, Decode)]
 pub enum Operator {
     Equal,
     Greater,
@@ -99,6 +100,12 @@ impl PartialEq for Version {
     }
 }
 
+impl Hash for Version {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        self.original.hash(state);
+    }
+}
+
 impl Eq for Version {}
 
 impl Ord for Version {
@@ -127,7 +134,8 @@ where
 /// A package can require specific version for some versions.
 /// Most of the time it's using >= but there are also some
 /// >, <, <= here and there and a couple of ==
-#[derive(Debug, PartialEq, Clone, Encode, Decode, Serialize, Deserialize)]
+#[derive(Debug, Hash, Eq, PartialEq, Clone, Encode, Decode, Serialize, Deserialize)]
+#[serde(try_from = "String")]
 pub struct VersionRequirement {
     pub(crate) version: Version,
     op: Operator,
@@ -150,7 +158,7 @@ impl VersionRequirement {
 }
 
 impl FromStr for VersionRequirement {
-    type Err = ();
+    type Err = String;
 
     // s is for format `(>= 4.5)`
     fn from_str(s: &str) -> Result<Self, Self::Err> {
@@ -187,6 +195,14 @@ impl FromStr for VersionRequirement {
     }
 }
 
+impl TryFrom<String> for VersionRequirement {
+    type Error = String;
+
+    fn try_from(s: String) -> Result<Self, Self::Error> {
+        s.parse()
+    }
+}
+
 impl fmt::Display for VersionRequirement {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         write!(f, "({} {})", self.op, self.version)

src/resolver/dependency.rs

@@ -3,7 +3,7 @@ use serde::Deserialize;
 use crate::cache::InstallationStatus;
 use crate::http::HttpError;
 use crate::lockfile::{LockedPackage, Source};
-use crate::package::{InstallationDependencies, Package, PackageRemote, PackageType};
+use crate::package::{Dependency, InstallationDependencies, Package, PackageRemote, PackageType};
 use crate::resolver::QueueItem;
 use crate::{Http, HttpDownload, Version, VersionRequirement};
 use std::borrow::Cow;
@@ -20,8 +20,8 @@ pub struct ResolvedDependency<'d> {
     pub(crate) name: Cow<'d, str>,
     pub(crate) version: Cow<'d, Version>,
     pub(crate) source: Source,
-    pub(crate) dependencies: Vec<Cow<'d, str>>,
-    pub(crate) suggests: Vec<Cow<'d, str>>,
+    pub(crate) dependencies: Vec<Cow<'d, Dependency>>,
+    pub(crate) suggests: Vec<Cow<'d, Dependency>>,
     pub(crate) force_source: bool,
     pub(crate) install_suggests: bool,
     pub(crate) kind: PackageType,
@@ -56,16 +56,8 @@ impl<'d> ResolvedDependency<'d> {
             name: Cow::Borrowed(&package.name),
             version: Cow::Owned(Version::from_str(package.version.as_str()).unwrap()),
             source: package.source.clone(),
-            dependencies: package
-                .dependencies
-                .iter()
-                .map(|d| Cow::Borrowed(d.as_str()))
-                .collect(),
-            suggests: package
-                .suggests
-                .iter()
-                .map(|s| Cow::Borrowed(s.as_str()))
-                .collect(),
+            dependencies: package.dependencies.iter().map(Cow::Borrowed).collect(),
+            suggests: package.suggests.iter().map(Cow::Borrowed).collect(),
             // TODO: what should we do here?
             kind: if package.force_source {
                 PackageType::Source
@@ -113,16 +105,8 @@ impl<'d> ResolvedDependency<'d> {
             name: Cow::Borrowed(&package.name),
             version: Cow::Borrowed(&package.version),
             source,
-            dependencies: deps
-                .direct
-                .iter()
-                .map(|d| Cow::Borrowed(d.name()))
-                .collect(),
-            suggests: deps
-                .suggests
-                .iter()
-                .map(|d| Cow::Borrowed(d.name()))
-                .collect(),
+            dependencies: deps.direct.iter().map(|d| Cow::Borrowed(*d)).collect(),
+            suggests: deps.suggests.iter().map(|d| Cow::Borrowed(*d)).collect(),
             kind: package_type,
             force_source,
             install_suggests,
@@ -148,15 +132,11 @@ impl<'d> ResolvedDependency<'d> {
         let deps = package.dependencies_to_install(install_suggests);
 
         let res = Self {
-            dependencies: deps
-                .direct
-                .iter()
-                .map(|d| Cow::Owned(d.name().to_string()))
-                .collect(),
+            dependencies: deps.direct.iter().map(|&d| Cow::Owned(d.clone())).collect(),
             suggests: deps
                 .suggests
                 .iter()
-                .map(|s| Cow::Owned(s.name().to_string()))
+                .map(|&d| Cow::Owned(d.clone()))
                 .collect(),
             kind: PackageType::Source,
             force_source: true,
@@ -183,15 +163,11 @@ impl<'d> ResolvedDependency<'d> {
     ) -> (Self, InstallationDependencies) {
         let deps = package.dependencies_to_install(install_suggests);
         let res = Self {
-            dependencies: deps
-                .direct
-                .iter()
-                .map(|d| Cow::Owned(d.name().to_string()))
-                .collect(),
+            dependencies: deps.direct.iter().map(|&d| Cow::Owned(d.clone())).collect(),
             suggests: deps
                 .suggests
                 .iter()
-                .map(|s| Cow::Owned(s.name().to_string()))
+                .map(|&d| Cow::Owned(d.clone()))
                 .collect(),
             kind: PackageType::Source,
             force_source: true,
@@ -219,15 +195,11 @@ impl<'d> ResolvedDependency<'d> {
     ) -> (Self, InstallationDependencies) {
         let deps = package.dependencies_to_install(install_suggests);
         let res = Self {
-            dependencies: deps
-                .direct
-                .iter()
-                .map(|d| Cow::Owned(d.name().to_string()))
-                .collect(),
+            dependencies: deps.direct.iter().map(|&d| Cow::Owned(d.clone())).collect(),
             suggests: deps
                 .suggests
                 .iter()
-                .map(|s| Cow::Owned(s.name().to_string()))
+                .map(|&d| Cow::Owned(d.clone()))
                 .collect(),
             kind,
             force_source: false,