From 02b50abfa1e0eba228061a91b9d89646c2bd4341 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Tue, 22 Aug 2023 23:24:47 +0200 Subject: [PATCH 01/10] feat: normalize package names where applicable --- crates/rattler-bin/src/commands/create.rs | 5 +- .../src/conda_lock/content_hash.rs | 11 +- .../rattler_conda_types/src/conda_lock/mod.rs | 4 +- crates/rattler_conda_types/src/lib.rs | 2 + .../rattler_conda_types/src/match_spec/mod.rs | 8 +- .../src/match_spec/parse.rs | 6 +- .../rattler_conda_types/src/package_name.rs | 129 ++++++++++++++++++ crates/rattler_libsolv_rs/src/conda_util.rs | 4 +- crates/rattler_libsolv_rs/src/pool.rs | 30 ++-- crates/rattler_libsolv_rs/src/problem.rs | 2 +- crates/rattler_libsolv_rs/src/solver/mod.rs | 108 +++++++-------- ...olver__test__unsat_after_backtracking.snap | 16 +-- ...test__unsat_applies_graph_compression.snap | 16 +-- ...lv_rs__solver__test__unsat_constrains.snap | 14 +- ..._unsat_incompatible_root_requirements.snap | 8 +- ...lver__test__unsat_locked_and_excluded.snap | 6 +- ...__test__unsat_missing_top_level_dep_2.snap | 2 +- ...test__unsat_no_candidates_for_child_2.snap | 4 +- crates/rattler_solve/tests/backends.rs | 9 +- 19 files changed, 267 insertions(+), 117 deletions(-) create mode 100644 crates/rattler_conda_types/src/package_name.rs diff --git a/crates/rattler-bin/src/commands/create.rs b/crates/rattler-bin/src/commands/create.rs index 5b40a7c4f..4105aedab 100644 --- a/crates/rattler-bin/src/commands/create.rs +++ b/crates/rattler-bin/src/commands/create.rs @@ -157,7 +157,10 @@ pub async fn create(opt: Opt) -> anyhow::Result<()> { .collect::, _>>()?; // Get the package names from the matchspecs so we can only load the package records that we need. - let package_names = specs.iter().filter_map(|spec| spec.name.as_ref()); + let package_names = specs + .iter() + .filter_map(|spec| spec.name.as_ref()) + .map(|name| name.as_normalized().as_ref()); let repodatas = wrap_in_progress("parsing repodata", move || { SparseRepoData::load_records_recursive( &sparse_repo_datas, diff --git a/crates/rattler_conda_types/src/conda_lock/content_hash.rs b/crates/rattler_conda_types/src/conda_lock/content_hash.rs index 20d0ae197..45d5b50b9 100644 --- a/crates/rattler_conda_types/src/conda_lock/content_hash.rs +++ b/crates/rattler_conda_types/src/conda_lock/content_hash.rs @@ -58,9 +58,14 @@ pub fn calculate_content_data( .iter() .map(|spec| { Ok(CondaLockVersionedDependency { - name: spec.name.clone().ok_or_else(|| { - CalculateContentHashError::RequiredAttributeMissing("name".to_string()) - })?, + name: spec + .name + .clone() + .ok_or_else(|| { + CalculateContentHashError::RequiredAttributeMissing("name".to_string()) + })? + .as_source() + .to_string(), manager: "conda".to_string(), optional: false, category: "main".to_string(), diff --git a/crates/rattler_conda_types/src/conda_lock/mod.rs b/crates/rattler_conda_types/src/conda_lock/mod.rs index 84fd67036..a3ce8aae2 100644 --- a/crates/rattler_conda_types/src/conda_lock/mod.rs +++ b/crates/rattler_conda_types/src/conda_lock/mod.rs @@ -348,7 +348,9 @@ impl TryFrom for RepoDataRecord { let matchspecs = value .dependencies .into_iter() - .map(|(name, matchspec)| MatchSpec::from_nameless(matchspec, Some(name)).to_string()) + .map(|(name, matchspec)| { + MatchSpec::from_nameless(matchspec, Some(name.into())).to_string() + }) .collect::>(); let version = value.version.parse()?; diff --git a/crates/rattler_conda_types/src/lib.rs b/crates/rattler_conda_types/src/lib.rs index 5c2d8a928..8df3a4344 100644 --- a/crates/rattler_conda_types/src/lib.rs +++ b/crates/rattler_conda_types/src/lib.rs @@ -18,6 +18,7 @@ pub mod version_spec; pub mod conda_lock; mod generic_virtual_package; pub mod package; +mod package_name; pub mod prefix_record; pub use channel::{Channel, ChannelConfig, ParseChannelError}; @@ -31,6 +32,7 @@ pub use match_spec::matcher::StringMatcher; pub use match_spec::parse::ParseMatchSpecError; pub use match_spec::{MatchSpec, NamelessMatchSpec}; pub use no_arch_type::{NoArchKind, NoArchType}; +pub use package_name::PackageName; pub use platform::{ParsePlatformError, Platform}; pub use prefix_record::PrefixRecord; pub use repo_data::patches::{PackageRecordPatch, PatchInstructions, RepoDataPatch}; diff --git a/crates/rattler_conda_types/src/match_spec/mod.rs b/crates/rattler_conda_types/src/match_spec/mod.rs index 3b1a7ede6..46be07666 100644 --- a/crates/rattler_conda_types/src/match_spec/mod.rs +++ b/crates/rattler_conda_types/src/match_spec/mod.rs @@ -1,4 +1,4 @@ -use crate::{PackageRecord, VersionSpec}; +use crate::{PackageName, PackageRecord, VersionSpec}; use rattler_digest::{serde::SerializableHash, Md5Hash, Sha256Hash}; use serde::Serialize; use serde_with::{serde_as, skip_serializing_none}; @@ -114,7 +114,7 @@ use matcher::StringMatcher; #[derive(Debug, Default, Clone, Serialize, Eq, PartialEq)] pub struct MatchSpec { /// The name of the package - pub name: Option, + pub name: Option, /// The version spec of the package (e.g. `1.2.3`, `>=1.2.3`, `1.2.*`) pub version: Option, /// The build string of the package (e.g. `py37_0`, `py37h6de7cb9_0`, `py*`) @@ -189,7 +189,7 @@ impl MatchSpec { /// Match a MatchSpec against a PackageRecord pub fn matches(&self, record: &PackageRecord) -> bool { if let Some(name) = self.name.as_ref() { - if name != &record.name { + if name.as_normalized().as_ref() != &record.name { return false; } } @@ -328,7 +328,7 @@ impl From for NamelessMatchSpec { impl MatchSpec { /// Constructs a [`MatchSpec`] from a [`NamelessMatchSpec`] and a name. - pub fn from_nameless(spec: NamelessMatchSpec, name: Option) -> Self { + pub fn from_nameless(spec: NamelessMatchSpec, name: Option) -> Self { Self { name, version: spec.version, diff --git a/crates/rattler_conda_types/src/match_spec/parse.rs b/crates/rattler_conda_types/src/match_spec/parse.rs index c4759e3f2..202e65b90 100644 --- a/crates/rattler_conda_types/src/match_spec/parse.rs +++ b/crates/rattler_conda_types/src/match_spec/parse.rs @@ -397,7 +397,7 @@ fn parse(input: &str) -> Result { // Step 6. Strip off the package name from the input let (name, input) = strip_package_name(input)?; - let mut match_spec = MatchSpec::from_nameless(nameless_match_spec, Some(name.to_owned())); + let mut match_spec = MatchSpec::from_nameless(nameless_match_spec, Some(name.into())); // Step 7. Otherwise sort our version + build let input = input.trim(); @@ -561,12 +561,12 @@ mod tests { #[test] fn test_match_spec_more() { let spec = MatchSpec::from_str("conda-forge::foo[version=\"1.0.*\"]").unwrap(); - assert_eq!(spec.name, Some("foo".to_string())); + assert_eq!(spec.name, Some("foo".into())); assert_eq!(spec.version, Some(VersionSpec::from_str("1.0.*").unwrap())); assert_eq!(spec.channel, Some("conda-forge".to_string())); let spec = MatchSpec::from_str("conda-forge::foo[version=1.0.*]").unwrap(); - assert_eq!(spec.name, Some("foo".to_string())); + assert_eq!(spec.name, Some("foo".into())); assert_eq!(spec.version, Some(VersionSpec::from_str("1.0.*").unwrap())); assert_eq!(spec.channel, Some("conda-forge".to_string())); } diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs new file mode 100644 index 000000000..1eadc9887 --- /dev/null +++ b/crates/rattler_conda_types/src/package_name.rs @@ -0,0 +1,129 @@ +use serde::{Serialize, Serializer}; +use serde_with::DeserializeFromStr; +use std::cmp::Ordering; +use std::fmt::{Display, Formatter}; +use std::hash::{Hash, Hasher}; +use std::str::FromStr; +use std::sync::Arc; + +/// A representation of a conda package name. This struct both stores the source string from which +/// this instance was created as well as a normalized name that can be used to compare different +/// names. The normalized name is guaranteed to be a valid conda package name. +/// +/// Conda package names are always lowercase. +#[derive(Debug, Clone, Eq, DeserializeFromStr)] +pub struct PackageName { + source: Arc, + normalized: Arc, +} + +impl PackageName { + /// Returns the source representation of the package name. This is the string from which this + /// instance was created. + pub fn as_source(&self) -> &Arc { + &self.source + } + + /// Returns the normalized version of the package name. The normalized string is guaranteed to + /// be a valid conda package name. + pub fn as_normalized(&self) -> &Arc { + &self.normalized + } +} + +impl From<&String> for PackageName { + fn from(value: &String) -> Self { + Arc::::from(value.clone()).into() + } +} + +impl From for PackageName { + fn from(value: String) -> Self { + Arc::::from(value).into() + } +} + +impl From> for PackageName { + fn from(value: Arc) -> Self { + let normalized = if value.chars().any(char::is_uppercase) { + Arc::from(value.to_lowercase()) + } else { + value.clone() + }; + Self { + source: value, + normalized, + } + } +} + +impl<'a> From<&'a str> for PackageName { + fn from(value: &'a str) -> Self { + Arc::::from(value.to_owned()).into() + } +} + +impl FromStr for PackageName { + type Err = String; + + fn from_str(s: &str) -> Result { + Ok(Self::from(s)) + } +} + +impl Hash for PackageName { + fn hash(&self, state: &mut H) { + self.normalized.hash(state) + } +} + +impl PartialEq for PackageName { + fn eq(&self, other: &Self) -> bool { + self.normalized.eq(&other.normalized) + } +} + +impl PartialOrd for PackageName { + fn partial_cmp(&self, other: &Self) -> Option { + Some(self.cmp(other)) + } +} + +impl Ord for PackageName { + fn cmp(&self, other: &Self) -> Ordering { + self.normalized.cmp(&other.normalized) + } +} + +impl Serialize for PackageName { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + self.source.as_ref().serialize(serializer) + } +} + +impl Display for PackageName { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + write!(f, "{}", self.source.as_ref()) + } +} + +#[cfg(test)] +mod test { + use super::*; + + #[test] + fn test_packagename_basics() { + let name1 = PackageName::from("cuDNN"); + assert_eq!(name1.as_source().as_ref(), "cuDNN"); + assert_eq!(name1.as_normalized().as_ref(), "cudnn"); + + let name2 = PackageName::from("cudnn"); + assert_eq!(name2.as_source().as_ref(), "cudnn"); + assert_eq!(name2.as_normalized().as_ref(), "cudnn"); + + assert_eq!(name1, name2); + } +} diff --git a/crates/rattler_libsolv_rs/src/conda_util.rs b/crates/rattler_libsolv_rs/src/conda_util.rs index e655d269f..cef078db1 100644 --- a/crates/rattler_libsolv_rs/src/conda_util.rs +++ b/crates/rattler_libsolv_rs/src/conda_util.rs @@ -191,8 +191,8 @@ pub(crate) fn find_candidates<'b>( ) -> &'b Vec { match_spec_to_candidates[match_spec_id].get_or_init(|| { let match_spec = &match_specs[match_spec_id]; - let Some(match_spec_name) = match_spec.name.as_deref() else { return Vec::new() }; - let Some(name_id) = names_to_ids.get(match_spec_name) else { return Vec::new() }; + let Some(match_spec_name) = match_spec.name.as_ref() else { return Vec::new() }; + let Some(name_id) = names_to_ids.get(match_spec_name.as_normalized().as_ref()) else { return Vec::new() }; packages_by_name[*name_id] .iter() diff --git a/crates/rattler_libsolv_rs/src/pool.rs b/crates/rattler_libsolv_rs/src/pool.rs index b357d09ae..f164cd986 100644 --- a/crates/rattler_libsolv_rs/src/pool.rs +++ b/crates/rattler_libsolv_rs/src/pool.rs @@ -3,7 +3,7 @@ use crate::conda_util; use crate::id::{MatchSpecId, NameId, RepoId, SolvableId}; use crate::mapping::Mapping; use crate::solvable::{PackageSolvable, Solvable}; -use rattler_conda_types::{MatchSpec, PackageRecord, Version}; +use rattler_conda_types::{MatchSpec, PackageName, PackageRecord, Version}; use std::cell::OnceCell; use std::cmp::Ordering; use std::collections::hash_map::Entry; @@ -135,11 +135,11 @@ impl<'a> Pool<'a> { solvable_order: &mut HashMap, ) { let match_spec = &self.match_specs[match_spec_id]; - let match_spec_name = match_spec - .name - .as_deref() - .expect("match spec without name!"); - let name_id = match self.names_to_ids.get(match_spec_name) { + let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); + let name_id = match self + .names_to_ids + .get(match_spec_name.as_normalized().as_ref()) + { None => return, Some(&name_id) => name_id, }; @@ -187,11 +187,11 @@ impl<'a> Pool<'a> { match_spec_to_forbidden: &mut Mapping>, ) { let match_spec = &self.match_specs[match_spec_id]; - let match_spec_name = match_spec - .name - .as_deref() - .expect("match spec without name!"); - let name_id = match self.names_to_ids.get(match_spec_name) { + let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); + let name_id = match self + .names_to_ids + .get(match_spec_name.as_normalized().as_ref()) + { None => return, Some(&name_id) => name_id, }; @@ -228,8 +228,12 @@ impl<'a> Pool<'a> { } /// Interns a package name into the `Pool`, returning its `NameId` - fn intern_package_name>(&mut self, str: T) -> NameId { - match self.names_to_ids.entry(str.into()) { + fn intern_package_name>(&mut self, name: T) -> NameId { + let package_name = name.into(); + match self + .names_to_ids + .entry(package_name.as_normalized().to_string()) + { Entry::Occupied(e) => *e.get(), Entry::Vacant(e) => { let next_id = self.package_names.alloc(e.key().clone()); diff --git a/crates/rattler_libsolv_rs/src/problem.rs b/crates/rattler_libsolv_rs/src/problem.rs index b5b7d56ae..8ba524554 100644 --- a/crates/rattler_libsolv_rs/src/problem.rs +++ b/crates/rattler_libsolv_rs/src/problem.rs @@ -100,7 +100,7 @@ impl Problem { .cloned() .find(|&ms| { let ms = solver.pool().resolve_match_spec(ms); - ms.name.as_deref().unwrap() == dep.record.name + ms.name.as_ref().unwrap().as_normalized().as_ref() == dep.record.name }) .unwrap(); diff --git a/crates/rattler_libsolv_rs/src/solver/mod.rs b/crates/rattler_libsolv_rs/src/solver/mod.rs index 146256f34..246852d38 100644 --- a/crates/rattler_libsolv_rs/src/solver/mod.rs +++ b/crates/rattler_libsolv_rs/src/solver/mod.rs @@ -1176,13 +1176,13 @@ mod test { #[test] fn test_resolve_favor_without_conflict() { let pool = pool(&[ - ("A", "1", vec![]), - ("A", "2", vec![]), - ("B", "1", vec![]), - ("B", "2", vec![]), + ("a", "1", vec![]), + ("a", "2", vec![]), + ("b", "1", vec![]), + ("b", "2", vec![]), ]); - let mut jobs = install(&["A", "B>=2"]); + let mut jobs = install(&["a", "b>=2"]); // Already installed: A=1; B=1 let already_installed = pool @@ -1203,23 +1203,23 @@ mod test { let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - B 2 - A 1 + b 2 + a 1 "###); } #[test] fn test_resolve_favor_with_conflict() { let pool = pool(&[ - ("A", "1", vec!["C=1"]), - ("A", "2", vec![]), - ("B", "1", vec!["C=1"]), - ("B", "2", vec!["C=2"]), - ("C", "1", vec![]), - ("C", "2", vec![]), + ("a", "1", vec!["c=1"]), + ("a", "2", vec![]), + ("b", "1", vec!["c=1"]), + ("b", "2", vec!["c=2"]), + ("c", "1", vec![]), + ("c", "2", vec![]), ]); - let mut jobs = install(&["A", "B>=2"]); + let mut jobs = install(&["a", "b>=2"]); // Already installed: A=1; B=1; C=1 let already_installed = pool @@ -1240,32 +1240,32 @@ mod test { let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - B 2 - C 2 - A 2 + b 2 + c 2 + a 2 "###); } #[test] fn test_resolve_cyclic() { - let pool = pool(&[("A", "2", vec!["B<=10"]), ("B", "5", vec!["A>=2,<=4"])]); - let jobs = install(&["A<100"]); + let pool = pool(&[("a", "2", vec!["b<=10"]), ("b", "5", vec!["a>=2,<=4"])]); + let jobs = install(&["a<100"]); let mut solver = Solver::new(pool); let solved = solver.solve(jobs).unwrap(); let result = transaction_to_string(&solver.pool, &solved); insta::assert_snapshot!(result, @r###" - A 2 - B 5 + a 2 + b 5 "###); } #[test] fn test_unsat_locked_and_excluded() { let pool = pool(&[ - ("asdf", "1.2.3", vec!["C>1"]), - ("C", "2.0.0", vec![]), - ("C", "1.0.0", vec![]), + ("asdf", "1.2.3", vec!["c>1"]), + ("c", "2.0.0", vec![]), + ("c", "1.0.0", vec![]), ]); let mut job = install(&["asdf"]); job.lock(SolvableId::from_usize(3)); // C 1.0.0 @@ -1283,7 +1283,7 @@ mod test { #[test] fn test_unsat_no_candidates_for_child_2() { - let pool = pool(&[("A", "41", vec!["B<20"])]); + let pool = pool(&[("a", "41", vec!["B<20"])]); let jobs = install(&["A<1000"]); let error = solve_unsat(pool, jobs); @@ -1299,8 +1299,8 @@ mod test { #[test] fn test_unsat_missing_top_level_dep_2() { - let pool = pool(&[("A", "41", vec!["B=15"]), ("B", "15", vec![])]); - let jobs = install(&["A=41", "B=14"]); + let pool = pool(&[("a", "41", vec!["b=15"]), ("b", "15", vec![])]); + let jobs = install(&["a=41", "b=14"]); let error = solve_unsat(pool, jobs); insta::assert_snapshot!(error); @@ -1309,24 +1309,24 @@ mod test { #[test] fn test_unsat_after_backtracking() { let pool = pool(&[ - ("B", "4.5.7", vec!["D=1"]), - ("B", "4.5.6", vec!["D=1"]), - ("C", "1.0.1", vec!["D=2"]), - ("C", "1.0.0", vec!["D=2"]), - ("D", "2.0.0", vec![]), - ("D", "1.0.0", vec![]), - ("E", "1.0.0", vec![]), - ("E", "1.0.1", vec![]), + ("b", "4.5.7", vec!["d=1"]), + ("b", "4.5.6", vec!["d=1"]), + ("c", "1.0.1", vec!["d=2"]), + ("c", "1.0.0", vec!["d=2"]), + ("d", "2.0.0", vec![]), + ("d", "1.0.0", vec![]), + ("e", "1.0.0", vec![]), + ("e", "1.0.1", vec![]), ]); - let error = solve_unsat(pool, install(&["B", "C", "E"])); + let error = solve_unsat(pool, install(&["b", "c", "e"])); insta::assert_snapshot!(error); } #[test] fn test_unsat_incompatible_root_requirements() { - let pool = pool(&[("A", "2", vec![]), ("A", "5", vec![])]); - let jobs = install(&["A<4", "A>=5,<10"]); + let pool = pool(&[("a", "2", vec![]), ("a", "5", vec![])]); + let jobs = install(&["a<4", "a>=5,<10"]); let error = solve_unsat(pool, jobs); insta::assert_snapshot!(error); @@ -1381,17 +1381,17 @@ mod test { #[test] fn test_unsat_applies_graph_compression() { let pool = pool(&[ - ("A", "10", vec!["B"]), - ("A", "9", vec!["B"]), - ("B", "100", vec!["C<100"]), - ("B", "42", vec!["C<100"]), - ("C", "103", vec![]), - ("C", "101", vec![]), - ("C", "100", vec![]), - ("C", "99", vec![]), + ("a", "10", vec!["b"]), + ("a", "9", vec!["b"]), + ("b", "100", vec!["c<100"]), + ("b", "42", vec!["c<100"]), + ("c", "103", vec![]), + ("c", "101", vec![]), + ("c", "100", vec![]), + ("c", "99", vec![]), ]); - let jobs = install(&["A", "C>100"]); + let jobs = install(&["a", "c>100"]); let error = solve_unsat(pool, jobs); insta::assert_snapshot!(error); @@ -1400,16 +1400,16 @@ mod test { #[test] fn test_unsat_constrains() { let mut pool = pool(&[ - ("A", "10", vec!["B>=50"]), - ("A", "9", vec!["B>=50"]), - ("B", "50", vec![]), - ("B", "42", vec![]), + ("a", "10", vec!["b>=50"]), + ("a", "9", vec!["b>=50"]), + ("b", "50", vec![]), + ("b", "42", vec![]), ]); - add_package(&mut pool, package("C", "10", &[], &["B<50"])); - add_package(&mut pool, package("C", "8", &[], &["B<50"])); + add_package(&mut pool, package("c", "10", &[], &["b<50"])); + add_package(&mut pool, package("c", "8", &[], &["b<50"])); - let jobs = install(&["A", "C"]); + let jobs = install(&["a", "c"]); let error = solve_unsat(pool, jobs); insta::assert_snapshot!(error); diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap index 6b519931d..68524e81b 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_after_backtracking.snap @@ -3,12 +3,12 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- The following packages are incompatible -|-- B can be installed with any of the following options: - |-- B 4.5.6 | 4.5.7 would require - |-- D 1.*, which can be installed with any of the following options: - |-- D 1.0.0 -|-- C cannot be installed because there are no viable options: - |-- C 1.0.0 | 1.0.1 would require - |-- D 2.*, which cannot be installed because there are no viable options: - |-- D 2.0.0, which conflicts with the versions reported above. +|-- b can be installed with any of the following options: + |-- b 4.5.6 | 4.5.7 would require + |-- d 1.*, which can be installed with any of the following options: + |-- d 1.0.0 +|-- c cannot be installed because there are no viable options: + |-- c 1.0.0 | 1.0.1 would require + |-- d 2.*, which cannot be installed because there are no viable options: + |-- d 2.0.0, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap index dec5f04d8..b63959c31 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_applies_graph_compression.snap @@ -3,12 +3,12 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- The following packages are incompatible -|-- A can be installed with any of the following options: - |-- A 9 | 10 would require - |-- B, which can be installed with any of the following options: - |-- B 42 | 100 would require - |-- C <100, which can be installed with any of the following options: - |-- C 99 -|-- C >100 cannot be installed because there are no viable options: - |-- C 101 | 103, which conflicts with the versions reported above. +|-- a can be installed with any of the following options: + |-- a 9 | 10 would require + |-- b, which can be installed with any of the following options: + |-- b 42 | 100 would require + |-- c <100, which can be installed with any of the following options: + |-- c 99 +|-- c >100 cannot be installed because there are no viable options: + |-- c 101 | 103, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap index b4c3c5fc4..deca94925 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_constrains.snap @@ -3,11 +3,11 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- The following packages are incompatible -|-- A can be installed with any of the following options: - |-- A 9 | 10 would require - |-- B >=50, which can be installed with any of the following options: - |-- B 50 -|-- C cannot be installed because there are no viable options: - |-- C 8 | 10 would constrain - |-- B <50 , which conflicts with any installable versions previously reported +|-- a can be installed with any of the following options: + |-- a 9 | 10 would require + |-- b >=50, which can be installed with any of the following options: + |-- b 50 +|-- c cannot be installed because there are no viable options: + |-- c 8 | 10 would constrain + |-- b <50 , which conflicts with any installable versions previously reported diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap index 7b407b808..133fa4685 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_incompatible_root_requirements.snap @@ -3,8 +3,8 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- The following packages are incompatible -|-- A >=5,<10 can be installed with any of the following options: - |-- A 5 -|-- A <4 cannot be installed because there are no viable options: - |-- A 2, which conflicts with the versions reported above. +|-- a >=5,<10 can be installed with any of the following options: + |-- a 5 +|-- a <4 cannot be installed because there are no viable options: + |-- a 2, which conflicts with the versions reported above. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap index 6bfae5807..3ceda4b45 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_locked_and_excluded.snap @@ -5,7 +5,7 @@ expression: error The following packages are incompatible |-- asdf can be installed with any of the following options: |-- asdf 1.2.3 would require - |-- C >1, which can be installed with any of the following options: - |-- C 2.0.0 -|-- C 1.0.0 is locked, but another version is required as reported above + |-- c >1, which can be installed with any of the following options: + |-- c 2.0.0 +|-- c 1.0.0 is locked, but another version is required as reported above diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap index 0ee601109..12ca4df4e 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap @@ -2,5 +2,5 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- -No candidates were found for B 14.*. +No candidates where found for b 14.*. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap index 8cd476b60..a75e5d80c 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap @@ -3,6 +3,6 @@ source: crates/rattler_libsolv_rs/src/solver/mod.rs expression: error --- A <1000 cannot be installed because there are no viable options: -|-- A 41 would require - |-- B <20, for which no candidates were found. +|-- a 41 would require + |-- B <20, for which no candidates where found. diff --git a/crates/rattler_solve/tests/backends.rs b/crates/rattler_solve/tests/backends.rs index 0a0a13f64..b97230c6e 100644 --- a/crates/rattler_solve/tests/backends.rs +++ b/crates/rattler_solve/tests/backends.rs @@ -107,7 +107,9 @@ fn solve_real_world(specs: Vec<&str>) -> Vec { let sparse_repo_datas = read_real_world_repo_data(); - let names = specs.iter().map(|s| s.name.clone().unwrap()); + let names = specs + .iter() + .map(|s| s.name.as_ref().unwrap().as_normalized().to_string()); let available_packages = SparseRepoData::load_records_recursive(sparse_repo_datas, names, None).unwrap(); @@ -510,7 +512,10 @@ fn compare_solve(specs: Vec<&str>) { let sparse_repo_datas = read_real_world_repo_data(); - let names = specs.iter().filter_map(|s| s.name.clone()); + let names = specs + .iter() + .filter_map(|s| s.name.as_ref()) + .map(|name| name.as_normalized().as_ref()); let available_packages = SparseRepoData::load_records_recursive(sparse_repo_datas, names, None).unwrap(); From a97a7dca9a9badce8e0863646c902813d82bf2c3 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Tue, 22 Aug 2023 23:32:28 +0200 Subject: [PATCH 02/10] fix: lint --- crates/rattler_conda_types/src/match_spec/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rattler_conda_types/src/match_spec/mod.rs b/crates/rattler_conda_types/src/match_spec/mod.rs index 46be07666..9806a276b 100644 --- a/crates/rattler_conda_types/src/match_spec/mod.rs +++ b/crates/rattler_conda_types/src/match_spec/mod.rs @@ -189,7 +189,7 @@ impl MatchSpec { /// Match a MatchSpec against a PackageRecord pub fn matches(&self, record: &PackageRecord) -> bool { if let Some(name) = self.name.as_ref() { - if name.as_normalized().as_ref() != &record.name { + if name.as_normalized().as_ref() != record.name { return false; } } From 1e66bbec506842890c1b4d44f87c666355620dc3 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 14:31:18 +0200 Subject: [PATCH 03/10] feat: use PackageName everywhere --- crates/rattler-bin/src/commands/create.rs | 40 +++--- crates/rattler/src/install/transaction.rs | 2 +- crates/rattler/src/package_cache.rs | 2 +- .../src/conda_lock/builder.rs | 26 ++-- .../rattler_conda_types/src/conda_lock/mod.rs | 17 +-- .../src/generic_virtual_package.rs | 12 +- crates/rattler_conda_types/src/lib.rs | 2 +- .../rattler_conda_types/src/match_spec/mod.rs | 22 ++-- .../src/match_spec/parse.rs | 55 ++++---- .../rattler_conda_types/src/package/index.rs | 4 +- .../rattler_conda_types/src/package_name.rs | 117 +++++++++++------- .../rattler_conda_types/src/repo_data/mod.rs | 14 ++- .../src/repo_data/topological_sort.rs | 23 ++-- crates/rattler_libsolv_rs/src/conda_util.rs | 10 +- crates/rattler_libsolv_rs/src/pool.rs | 24 ++-- crates/rattler_libsolv_rs/src/problem.rs | 17 ++- crates/rattler_libsolv_rs/src/solvable.rs | 2 +- .../rattler_libsolv_rs/src/solver/clause.rs | 2 +- crates/rattler_libsolv_rs/src/solver/mod.rs | 16 +-- ...__test__unsat_missing_top_level_dep_2.snap | 3 +- ...test__unsat_no_candidates_for_child_1.snap | 3 +- ...test__unsat_no_candidates_for_child_2.snap | 5 +- crates/rattler_solve/src/libsolv_c/input.rs | 4 +- crates/rattler_solve/tests/backends.rs | 26 ++-- crates/rattler_virtual_packages/src/lib.rs | 18 +-- 25 files changed, 265 insertions(+), 201 deletions(-) diff --git a/crates/rattler-bin/src/commands/create.rs b/crates/rattler-bin/src/commands/create.rs index 4105aedab..5fb21c65c 100644 --- a/crates/rattler-bin/src/commands/create.rs +++ b/crates/rattler-bin/src/commands/create.rs @@ -160,13 +160,13 @@ pub async fn create(opt: Opt) -> anyhow::Result<()> { let package_names = specs .iter() .filter_map(|spec| spec.name.as_ref()) - .map(|name| name.as_normalized().as_ref()); + .map(|name| name.as_normalized()); let repodatas = wrap_in_progress("parsing repodata", move || { SparseRepoData::load_records_recursive( &sparse_repo_datas, package_names, Some(|record| { - if record.name == "python" { + if record.name.as_normalized() == "python" { record.depends.push("pip".to_string()); } }), @@ -182,24 +182,26 @@ pub async fn create(opt: Opt) -> anyhow::Result<()> { .iter() .map(|virt_pkg| { let elems = virt_pkg.split('=').collect::>(); - GenericVirtualPackage { - name: elems[0].to_string(), + Ok(GenericVirtualPackage { + name: elems[0].try_into()?, version: elems .get(1) .map(|s| Version::from_str(s)) .unwrap_or(Version::from_str("0")) .expect("Could not parse virtual package version"), build_string: elems.get(2).unwrap_or(&"").to_string(), - } + }) }) - .collect::>()) + .collect::>>()?) } else { - rattler_virtual_packages::VirtualPackage::current().map(|vpkgs| { - vpkgs - .iter() - .map(|vpkg| GenericVirtualPackage::from(vpkg.clone())) - .collect::>() - }) + rattler_virtual_packages::VirtualPackage::current() + .map(|vpkgs| { + vpkgs + .iter() + .map(|vpkg| GenericVirtualPackage::from(vpkg.clone())) + .collect::>() + }) + .map_err(anyhow::Error::from) } })?; @@ -250,7 +252,9 @@ pub async fn create(opt: Opt) -> anyhow::Result<()> { let format_record = |r: &RepoDataRecord| { format!( "{} {} {}", - r.package_record.name, r.package_record.version, r.package_record.build + r.package_record.name.as_normalized(), + r.package_record.version, + r.package_record.build ) }; @@ -495,7 +499,11 @@ async fn install_package_to_environment( // Write the conda-meta information let pkg_meta_path = conda_meta_path.join(format!( "{}-{}-{}.json", - prefix_record.repodata_record.package_record.name, + prefix_record + .repodata_record + .package_record + .name + .as_normalized(), prefix_record.repodata_record.package_record.version, prefix_record.repodata_record.package_record.build )); @@ -539,7 +547,7 @@ async fn remove_package_from_environment( // Remove the conda-meta file let conda_meta_path = target_prefix.join("conda-meta").join(format!( "{}-{}-{}.json", - package.repodata_record.package_record.name, + package.repodata_record.package_record.name.as_normalized(), package.repodata_record.package_record.version, package.repodata_record.package_record.build )); @@ -623,7 +631,7 @@ async fn fetch_repo_data_records_with_progress( platform.to_string(), repo_data_json_path, Some(|record: &mut PackageRecord| { - if record.name == "python" { + if record.name.as_normalized() == "python" { record.depends.push("pip".to_string()); } }), diff --git a/crates/rattler/src/install/transaction.rs b/crates/rattler/src/install/transaction.rs index 6ebec560c..b5040cf0a 100644 --- a/crates/rattler/src/install/transaction.rs +++ b/crates/rattler/src/install/transaction.rs @@ -166,7 +166,7 @@ fn find_python_info( /// Returns true if the specified record refers to Python. fn is_python_record(record: &PackageRecord) -> bool { - record.name == "python" + record.name.as_normalized() == "python" } /// Returns true if the `from` and `to` describe the same package content diff --git a/crates/rattler/src/package_cache.rs b/crates/rattler/src/package_cache.rs index bad1f9aff..f267b610c 100644 --- a/crates/rattler/src/package_cache.rs +++ b/crates/rattler/src/package_cache.rs @@ -56,7 +56,7 @@ impl From for CacheKey { impl From<&PackageRecord> for CacheKey { fn from(record: &PackageRecord) -> Self { Self { - name: record.name.to_string(), + name: record.name.as_normalized().to_string(), version: record.version.to_string(), build_string: record.build.to_string(), } diff --git a/crates/rattler_conda_types/src/conda_lock/builder.rs b/crates/rattler_conda_types/src/conda_lock/builder.rs index c61e30781..367a3487a 100644 --- a/crates/rattler_conda_types/src/conda_lock/builder.rs +++ b/crates/rattler_conda_types/src/conda_lock/builder.rs @@ -6,7 +6,7 @@ use crate::conda_lock::{ content_hash, Channel, CondaLock, GitMeta, LockMeta, LockedDependency, Manager, PackageHashes, TimeMeta, }; -use crate::{MatchSpec, NamelessMatchSpec, NoArchType, Platform, RepoDataRecord}; +use crate::{MatchSpec, NamelessMatchSpec, NoArchType, PackageName, Platform, RepoDataRecord}; use fxhash::{FxHashMap, FxHashSet}; use std::str::FromStr; use url::Url; @@ -169,7 +169,7 @@ impl LockedPackages { /// Short-hand for creating a LockedPackage that transforms into a [`LockedDependency`] pub struct LockedPackage { /// Name of the locked package - pub name: String, + pub name: PackageName, /// Package version pub version: String, /// Package build string @@ -179,7 +179,7 @@ pub struct LockedPackage { /// Collection of package hash fields pub package_hashes: PackageHashes, /// List of dependencies for this package - pub dependency_list: FxHashMap, + pub dependency_list: FxHashMap, /// Check if package is optional pub optional: Option, @@ -245,9 +245,9 @@ impl TryFrom for LockedPackage { .ok_or_else(|| { ConversionError::Missing(format!("dependency name for {}", match_spec_str)) })? - .to_string(); + .clone(); let version_constraint = NamelessMatchSpec::from(matchspec); - dependencies.insert(name, version_constraint); + dependencies.insert(name.clone(), version_constraint); } Ok(Self { @@ -281,20 +281,19 @@ impl LockedPackage { } /// Add a single dependency - pub fn add_dependency>( + pub fn add_dependency( mut self, - key: S, + key: PackageName, version_constraint: NamelessMatchSpec, ) -> Self { - self.dependency_list - .insert(key.as_ref().to_string(), version_constraint); + self.dependency_list.insert(key, version_constraint); self } /// Add multiple dependencies pub fn add_dependencies( mut self, - value: impl IntoIterator, + value: impl IntoIterator, ) -> Self { self.dependency_list.extend(value); self @@ -391,7 +390,8 @@ mod tests { use crate::conda_lock::builder::{LockFileBuilder, LockedPackage, LockedPackages}; use crate::conda_lock::PackageHashes; use crate::{ - ChannelConfig, MatchSpec, NamelessMatchSpec, NoArchType, Platform, RepoDataRecord, + ChannelConfig, MatchSpec, NamelessMatchSpec, NoArchType, PackageName, Platform, + RepoDataRecord, }; use rattler_digest::parse_digest_from_hex; @@ -405,13 +405,13 @@ mod tests { ) .add_locked_packages(LockedPackages::new(Platform::Osx64) .add_locked_package(LockedPackage { - name: "python".to_string(), + name: PackageName::new_unchecked("python"), version: "3.11.0".to_string(), build_string: "h4150a38_1_cpython".to_string(), url: "https://conda.anaconda.org/conda-forge/osx-64/python-3.11.0-h4150a38_1_cpython.conda".parse().unwrap(), package_hashes: PackageHashes::Md5Sha256(parse_digest_from_hex::("c6f4b87020c72e2700e3e94c1fc93b70").unwrap(), parse_digest_from_hex::("7c58de8c7d98b341bd9be117feec64782e704fec5c30f6e14713ebccaab9b5d8").unwrap()), - dependency_list: FxHashMap::from_iter([("python".to_string(), NamelessMatchSpec::from_str("3.11.0.*").unwrap())]), + dependency_list: FxHashMap::from_iter([(PackageName::new_unchecked("python"), NamelessMatchSpec::from_str("3.11.0.*").unwrap())]), optional: None, arch: Some("x86_64".to_string()), subdir: Some("noarch".to_string()), diff --git a/crates/rattler_conda_types/src/conda_lock/mod.rs b/crates/rattler_conda_types/src/conda_lock/mod.rs index a3ce8aae2..e93d407cd 100644 --- a/crates/rattler_conda_types/src/conda_lock/mod.rs +++ b/crates/rattler_conda_types/src/conda_lock/mod.rs @@ -4,11 +4,11 @@ //! However, some types were added to enforce a bit more type safety. use self::PackageHashes::{Md5, Md5Sha256, Sha256}; use crate::match_spec::parse::ParseMatchSpecError; -use crate::MatchSpec; use crate::{ utils::serde::Ordered, NamelessMatchSpec, NoArchType, PackageRecord, ParsePlatformError, ParseVersionError, Platform, RepoDataRecord, }; +use crate::{MatchSpec, PackageName}; use fxhash::FxHashMap; use rattler_digest::{serde::SerializableHash, Md5Hash, Sha256Hash}; use serde::{de::Error, Deserialize, Deserializer, Serialize, Serializer}; @@ -237,7 +237,7 @@ fn default_category() -> String { #[derive(Serialize, Deserialize, Eq, PartialEq, Clone, Debug)] pub struct LockedDependency { /// Package name of dependency - pub name: String, + pub name: PackageName, /// Locked version pub version: String, /// Pip or Conda managed @@ -248,7 +248,7 @@ pub struct LockedDependency { /// What are its own dependencies mapping name to version constraint #[serde_as(as = "FxHashMap<_, DisplayFromStr>")] - pub dependencies: FxHashMap, + pub dependencies: FxHashMap, /// URL to find it at pub url: Url, /// Hashes of the package @@ -348,9 +348,7 @@ impl TryFrom for RepoDataRecord { let matchspecs = value .dependencies .into_iter() - .map(|(name, matchspec)| { - MatchSpec::from_nameless(matchspec, Some(name.into())).to_string() - }) + .map(|(name, matchspec)| MatchSpec::from_nameless(matchspec, Some(name)).to_string()) .collect::>(); let version = value.version.parse()?; @@ -594,12 +592,15 @@ mod test { let result: crate::conda_lock::LockedDependency = from_str(yaml).unwrap(); - assert_eq!(result.name, "ncurses"); + assert_eq!(result.name.as_normalized(), "ncurses"); assert_eq!(result.version, "6.4"); let repodata_record = RepoDataRecord::try_from(result.clone()).unwrap(); - assert_eq!(repodata_record.package_record.name, "ncurses"); + assert_eq!( + repodata_record.package_record.name.as_normalized(), + "ncurses" + ); assert_eq!( repodata_record.package_record.version, VersionWithSource::from_str("6.4").unwrap() diff --git a/crates/rattler_conda_types/src/generic_virtual_package.rs b/crates/rattler_conda_types/src/generic_virtual_package.rs index 11ee8c313..258fedeac 100644 --- a/crates/rattler_conda_types/src/generic_virtual_package.rs +++ b/crates/rattler_conda_types/src/generic_virtual_package.rs @@ -1,4 +1,4 @@ -use crate::Version; +use crate::{PackageName, Version}; use std::fmt::{Display, Formatter}; /// A `GenericVirtualPackage` is a Conda package description that contains a `name` and a @@ -6,7 +6,7 @@ use std::fmt::{Display, Formatter}; #[derive(Clone, Debug, Eq, PartialEq, Hash)] pub struct GenericVirtualPackage { /// The name of the package - pub name: String, + pub name: PackageName, /// The version of the package pub version: Version, @@ -17,6 +17,12 @@ pub struct GenericVirtualPackage { impl Display for GenericVirtualPackage { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{}={}={}", &self.name, &self.version, &self.build_string) + write!( + f, + "{}={}={}", + &self.name.as_normalized(), + &self.version, + &self.build_string + ) } } diff --git a/crates/rattler_conda_types/src/lib.rs b/crates/rattler_conda_types/src/lib.rs index 8df3a4344..15c6cd087 100644 --- a/crates/rattler_conda_types/src/lib.rs +++ b/crates/rattler_conda_types/src/lib.rs @@ -32,7 +32,7 @@ pub use match_spec::matcher::StringMatcher; pub use match_spec::parse::ParseMatchSpecError; pub use match_spec::{MatchSpec, NamelessMatchSpec}; pub use no_arch_type::{NoArchKind, NoArchType}; -pub use package_name::PackageName; +pub use package_name::{PackageName, InvalidPackageNameError}; pub use platform::{ParsePlatformError, Platform}; pub use prefix_record::PrefixRecord; pub use repo_data::patches::{PackageRecordPatch, PatchInstructions, RepoDataPatch}; diff --git a/crates/rattler_conda_types/src/match_spec/mod.rs b/crates/rattler_conda_types/src/match_spec/mod.rs index 9806a276b..4b33d4fcc 100644 --- a/crates/rattler_conda_types/src/match_spec/mod.rs +++ b/crates/rattler_conda_types/src/match_spec/mod.rs @@ -63,38 +63,38 @@ use matcher::StringMatcher; /// # Examples: /// /// ```rust -/// use rattler_conda_types::{MatchSpec, VersionSpec, StringMatcher}; +/// use rattler_conda_types::{MatchSpec, VersionSpec, StringMatcher, PackageName}; /// use std::str::FromStr; /// /// let spec = MatchSpec::from_str("foo 1.0 py27_0").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.version, Some(VersionSpec::from_str("1.0").unwrap())); /// assert_eq!(spec.build, Some(StringMatcher::from_str("py27_0").unwrap())); /// /// let spec = MatchSpec::from_str("foo=1.0=py27_0").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.version, Some(VersionSpec::from_str("==1.0").unwrap())); /// assert_eq!(spec.build, Some(StringMatcher::from_str("py27_0").unwrap())); /// /// let spec = MatchSpec::from_str("conda-forge::foo[version=\"1.0.*\"]").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.version, Some(VersionSpec::from_str("1.0.*").unwrap())); /// assert_eq!(spec.channel, Some("conda-forge".to_string())); /// /// let spec = MatchSpec::from_str("conda-forge/linux-64::foo>=1.0").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.version, Some(VersionSpec::from_str(">=1.0").unwrap())); /// assert_eq!(spec.channel, Some("conda-forge".to_string())); /// assert_eq!(spec.subdir, Some("linux-64".to_string())); /// /// let spec = MatchSpec::from_str("*/linux-64::foo>=1.0").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.version, Some(VersionSpec::from_str(">=1.0").unwrap())); /// assert_eq!(spec.channel, Some("*".to_string())); /// assert_eq!(spec.subdir, Some("linux-64".to_string())); /// /// let spec = MatchSpec::from_str("foo[build=\"py2*\"]").unwrap(); -/// assert_eq!(spec.name, Some("foo".to_string())); +/// assert_eq!(spec.name, Some(PackageName::new_unchecked("foo"))); /// assert_eq!(spec.build, Some(StringMatcher::from_str("py2*").unwrap())); /// ``` /// @@ -149,7 +149,7 @@ impl Display for MatchSpec { } match &self.name { - Some(name) => write!(f, "{name}")?, + Some(name) => write!(f, "{}", name.as_normalized())?, None => write!(f, "*")?, } @@ -189,7 +189,7 @@ impl MatchSpec { /// Match a MatchSpec against a PackageRecord pub fn matches(&self, record: &PackageRecord) -> bool { if let Some(name) = self.name.as_ref() { - if name.as_normalized().as_ref() != record.name { + if name != &record.name { return false; } } @@ -350,7 +350,7 @@ mod tests { use rattler_digest::{parse_digest_from_hex, Md5, Sha256}; - use crate::{MatchSpec, NamelessMatchSpec, PackageRecord, Version}; + use crate::{MatchSpec, NamelessMatchSpec, PackageName, PackageRecord, Version}; #[test] fn test_matchspec_format_eq() { @@ -378,7 +378,7 @@ mod tests { ), md5: parse_digest_from_hex::("dede6252c964db3f3e41c7d30d07f6bf"), ..PackageRecord::new( - String::from("mamba"), + PackageName::new_unchecked("mamba"), Version::from_str("1.0").unwrap(), String::from(""), ) diff --git a/crates/rattler_conda_types/src/match_spec/parse.rs b/crates/rattler_conda_types/src/match_spec/parse.rs index 202e65b90..e6ce59f12 100644 --- a/crates/rattler_conda_types/src/match_spec/parse.rs +++ b/crates/rattler_conda_types/src/match_spec/parse.rs @@ -3,7 +3,9 @@ use super::MatchSpec; use crate::package::ArchiveType; use crate::version_spec::version_tree::{recognize_constraint, recognize_version}; use crate::version_spec::{is_start_of_version_constraint, ParseVersionSpecError}; -use crate::{NamelessMatchSpec, ParseChannelError, VersionSpec}; +use crate::{ + InvalidPackageNameError, NamelessMatchSpec, PackageName, ParseChannelError, VersionSpec, +}; use nom::branch::alt; use nom::bytes::complete::{tag, take_till1, take_until, take_while, take_while1}; use nom::character::complete::{char, multispace0, one_of}; @@ -22,7 +24,7 @@ use thiserror::Error; use url::Url; /// The type of parse error that occurred when parsing match spec. -#[derive(Debug, Clone, Eq, PartialEq, Error)] +#[derive(Debug, Clone, Error)] pub enum ParseMatchSpecError { /// The path or url of the package was invalid #[error("invalid package path or url")] @@ -57,11 +59,11 @@ pub enum ParseMatchSpecError { InvalidVersionAndBuild(String), /// Invalid version spec - #[error("invalid version spec: {0}")] + #[error(transparent)] InvalidVersionSpec(#[from] ParseVersionSpecError), /// Invalid string matcher - #[error("invalid string matcher: {0}")] + #[error(transparent)] InvalidStringMatcher(#[from] StringMatcherParseError), /// Invalid build number @@ -71,6 +73,10 @@ pub enum ParseMatchSpecError { /// Unable to parse hash digest from hex #[error("Unable to parse hash digest from hex")] InvalidHashDigest, + + /// The package name was invalid + #[error(transparent)] + InvalidPackageName(#[from] InvalidPackageNameError), } impl FromStr for MatchSpec { @@ -221,11 +227,11 @@ fn parse_bracket_vec_into_components( } /// Strip the package name from the input. -fn strip_package_name(input: &str) -> Result<(&str, &str), ParseMatchSpecError> { +fn strip_package_name(input: &str) -> Result<(PackageName, &str), ParseMatchSpecError> { match take_while1(|c: char| !c.is_whitespace() && !is_start_of_version_constraint(c))(input) .finish() { - Ok((input, name)) => Ok((name.trim(), input.trim())), + Ok((input, name)) => Ok((PackageName::from_str(name.trim())?, input.trim())), Err(nom::error::Error { .. }) => Err(ParseMatchSpecError::MissingPackageName), } } @@ -397,7 +403,7 @@ fn parse(input: &str) -> Result { // Step 6. Strip off the package name from the input let (name, input) = strip_package_name(input)?; - let mut match_spec = MatchSpec::from_nameless(nameless_match_spec, Some(name.into())); + let mut match_spec = MatchSpec::from_nameless(nameless_match_spec, Some(name)); // Step 7. Otherwise sort our version + build let input = input.trim(); @@ -454,6 +460,7 @@ fn parse(input: &str) -> Result { #[cfg(test)] mod tests { + use assert_matches::assert_matches; use rattler_digest::{parse_digest_from_hex, Md5, Sha256}; use serde::Serialize; use std::collections::BTreeMap; @@ -493,11 +500,11 @@ mod tests { let expected: BracketVec = smallvec![("version", "1.2.3"), ("build_number", "1")]; assert_eq!(result.1, expected); - assert_eq!( + assert_matches!( strip_brackets(r#"bla [version="1.2.3", build_number=]"#), Err(ParseMatchSpecError::InvalidBracket) ); - assert_eq!( + assert_matches!( strip_brackets(r#"bla [version="1.2.3, build_number=1]"#), Err(ParseMatchSpecError::InvalidBracket) ); @@ -505,35 +512,35 @@ mod tests { #[test] fn test_split_version_and_build() { - assert_eq!( + assert_matches!( split_version_and_build("==1.0=py27_0"), Ok(("==1.0", Some("py27_0"))) ); - assert_eq!(split_version_and_build("=*=cuda"), Ok(("=*", Some("cuda")))); - assert_eq!( + assert_matches!(split_version_and_build("=*=cuda"), Ok(("=*", Some("cuda")))); + assert_matches!( split_version_and_build("=1.2.3 0"), Ok(("=1.2.3", Some("0"))) ); - assert_eq!(split_version_and_build("1.2.3=0"), Ok(("1.2.3", Some("0")))); - assert_eq!( + assert_matches!(split_version_and_build("1.2.3=0"), Ok(("1.2.3", Some("0")))); + assert_matches!( split_version_and_build(">=1.0 , < 2.0 py34_0"), Ok((">=1.0 , < 2.0", Some("py34_0"))) ); - assert_eq!( + assert_matches!( split_version_and_build(">=1.0 , < 2.0 =py34_0"), Ok((">=1.0 , < 2.0", Some("=py34_0"))) ); - assert_eq!(split_version_and_build("=1.2.3 "), Ok(("=1.2.3", None))); - assert_eq!( + assert_matches!(split_version_and_build("=1.2.3 "), Ok(("=1.2.3", None))); + assert_matches!( split_version_and_build(">1.8,<2|==1.7"), Ok((">1.8,<2|==1.7", None)) ); - assert_eq!( + assert_matches!( split_version_and_build("* openblas_0"), Ok(("*", Some("openblas_0"))) ); - assert_eq!(split_version_and_build("* *"), Ok(("*", Some("*")))); - assert_eq!( + assert_matches!(split_version_and_build("* *"), Ok(("*", Some("*")))); + assert_matches!( split_version_and_build(">=1!164.3095,<1!165"), Ok((">=1!164.3095,<1!165", None)) ); @@ -561,12 +568,12 @@ mod tests { #[test] fn test_match_spec_more() { let spec = MatchSpec::from_str("conda-forge::foo[version=\"1.0.*\"]").unwrap(); - assert_eq!(spec.name, Some("foo".into())); + assert_eq!(spec.name, Some("foo".parse().unwrap())); assert_eq!(spec.version, Some(VersionSpec::from_str("1.0.*").unwrap())); assert_eq!(spec.channel, Some("conda-forge".to_string())); let spec = MatchSpec::from_str("conda-forge::foo[version=1.0.*]").unwrap(); - assert_eq!(spec.name, Some("foo".into())); + assert_eq!(spec.name, Some("foo".parse().unwrap())); assert_eq!(spec.version, Some(VersionSpec::from_str("1.0.*").unwrap())); assert_eq!(spec.channel, Some("conda-forge".to_string())); } @@ -574,10 +581,10 @@ mod tests { #[test] fn test_hash_spec() { let spec = MatchSpec::from_str("conda-forge::foo[md5=1234567890]"); - assert_eq!(spec, Err(ParseMatchSpecError::InvalidHashDigest)); + assert_matches!(spec, Err(ParseMatchSpecError::InvalidHashDigest)); let spec = MatchSpec::from_str("conda-forge::foo[sha256=1234567890]"); - assert_eq!(spec, Err(ParseMatchSpecError::InvalidHashDigest)); + assert_matches!(spec, Err(ParseMatchSpecError::InvalidHashDigest)); let spec = MatchSpec::from_str("conda-forge::foo[sha256=315f5bdb76d078c43b8ac0064e4a0164612b1fce77c869345bfc94c75894edd3]").unwrap(); assert_eq!( diff --git a/crates/rattler_conda_types/src/package/index.rs b/crates/rattler_conda_types/src/package/index.rs index 16fbcba15..5aea26a52 100644 --- a/crates/rattler_conda_types/src/package/index.rs +++ b/crates/rattler_conda_types/src/package/index.rs @@ -1,7 +1,7 @@ use std::path::Path; use super::PackageFile; -use crate::{NoArchType, VersionWithSource}; +use crate::{NoArchType, PackageName, VersionWithSource}; use serde::{Deserialize, Serialize}; use serde_with::{serde_as, skip_serializing_none, OneOrMany}; @@ -45,7 +45,7 @@ pub struct IndexJson { pub license_family: Option, /// The lowercase name of the package - pub name: String, + pub name: PackageName, /// If this package is independent of architecture this field specifies in what way. See /// [`NoArchType`] for more information. diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs index 1eadc9887..94f505dd7 100644 --- a/crates/rattler_conda_types/src/package_name.rs +++ b/crates/rattler_conda_types/src/package_name.rs @@ -1,85 +1,114 @@ use serde::{Serialize, Serializer}; use serde_with::DeserializeFromStr; use std::cmp::Ordering; -use std::fmt::{Display, Formatter}; use std::hash::{Hash, Hasher}; use std::str::FromStr; -use std::sync::Arc; +use thiserror::Error; /// A representation of a conda package name. This struct both stores the source string from which /// this instance was created as well as a normalized name that can be used to compare different /// names. The normalized name is guaranteed to be a valid conda package name. /// -/// Conda package names are always lowercase. +/// Conda package names are always lowercase and can only contain ascii characters. +/// +/// This struct explicitly does not implement [`Display`] because its ambiguous if that would +/// display the source or the normalized version. Simply call `as_source` or `as_normalized` to make +/// the distinction. #[derive(Debug, Clone, Eq, DeserializeFromStr)] pub struct PackageName { - source: Arc, - normalized: Arc, + normalized: Option, + source: String, } impl PackageName { + /// Constructs a new `PackageName` from a string without checking if the string is actually a + /// valid or normalized conda package name. This should only be used if you are sure that the + /// input string is valid, otherwise use the `TryFrom` implementations. + pub fn new_unchecked>(normalized: S) -> Self { + Self { + normalized: None, + source: normalized.into(), + } + } + /// Returns the source representation of the package name. This is the string from which this /// instance was created. - pub fn as_source(&self) -> &Arc { + pub fn as_source(&self) -> &str { &self.source } /// Returns the normalized version of the package name. The normalized string is guaranteed to /// be a valid conda package name. - pub fn as_normalized(&self) -> &Arc { - &self.normalized + pub fn as_normalized(&self) -> &str { + self.normalized.as_ref().unwrap_or(&self.source) } } -impl From<&String> for PackageName { - fn from(value: &String) -> Self { - Arc::::from(value.clone()).into() - } +/// An error that is returned when conversion from a string to a [`PackageName`] fails. +#[derive(Clone, Debug, Error)] +pub enum InvalidPackageNameError { + /// The package name contains illegal characters + #[error("'{0}' is not a valid package name. Package names can only contain 0-9, a-z, A-Z, -, _, or .")] + InvalidCharacters(String), } -impl From for PackageName { - fn from(value: String) -> Self { - Arc::::from(value).into() +impl TryFrom<&String> for PackageName { + type Error = InvalidPackageNameError; + + fn try_from(value: &String) -> Result { + value.clone().try_into() } } -impl From> for PackageName { - fn from(value: Arc) -> Self { - let normalized = if value.chars().any(char::is_uppercase) { - Arc::from(value.to_lowercase()) +impl TryFrom for PackageName { + type Error = InvalidPackageNameError; + + fn try_from(source: String) -> Result { + // Ensure that the string only contains valid characters + if !source + .chars() + .all(|c| matches!(c, 'a'..='z'|'A'..='Z'|'0'..='9'|'-'|'_'|'.')) + { + return Err(InvalidPackageNameError::InvalidCharacters(source)); + } + + // Convert all characters to lowercase but only if it actually contains uppercase. This way + // we dont allocate the memory of the string if it is already lowercase. + let normalized = if source.chars().any(|c| c.is_ascii_uppercase()) { + Some(source.to_ascii_lowercase()) } else { - value.clone() + None }; - Self { - source: value, - normalized, - } + + Ok(Self { source, normalized }) } } -impl<'a> From<&'a str> for PackageName { - fn from(value: &'a str) -> Self { - Arc::::from(value.to_owned()).into() +impl<'a> TryFrom<&'a str> for PackageName { + type Error = InvalidPackageNameError; + + fn try_from(value: &'a str) -> Result { + value.to_owned().try_into() } } impl FromStr for PackageName { - type Err = String; + type Err = InvalidPackageNameError; fn from_str(s: &str) -> Result { - Ok(Self::from(s)) + s.to_owned().try_into() } } impl Hash for PackageName { fn hash(&self, state: &mut H) { - self.normalized.hash(state) + self.as_normalized().hash(state) } } impl PartialEq for PackageName { fn eq(&self, other: &Self) -> bool { - self.normalized.eq(&other.normalized) + self.as_normalized().eq(other.as_normalized()) } } @@ -91,7 +120,7 @@ impl PartialOrd for PackageName { impl Ord for PackageName { fn cmp(&self, other: &Self) -> Ordering { - self.normalized.cmp(&other.normalized) + self.as_normalized().cmp(&other.as_normalized()) } } @@ -100,13 +129,7 @@ impl Serialize for PackageName { where S: Serializer, { - self.source.as_ref().serialize(serializer) - } -} - -impl Display for PackageName { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{}", self.source.as_ref()) + self.as_source().serialize(serializer) } } @@ -115,15 +138,17 @@ mod test { use super::*; #[test] - fn test_packagename_basics() { - let name1 = PackageName::from("cuDNN"); - assert_eq!(name1.as_source().as_ref(), "cuDNN"); - assert_eq!(name1.as_normalized().as_ref(), "cudnn"); + fn test_package_name_basics() { + let name1 = PackageName::try_from("cuDNN").unwrap(); + assert_eq!(name1.as_source(), "cuDNN"); + assert_eq!(name1.as_normalized(), "cudnn"); - let name2 = PackageName::from("cudnn"); - assert_eq!(name2.as_source().as_ref(), "cudnn"); - assert_eq!(name2.as_normalized().as_ref(), "cudnn"); + let name2 = PackageName::try_from("cudnn").unwrap(); + assert_eq!(name2.as_source(), "cudnn"); + assert_eq!(name2.as_normalized(), "cudnn"); assert_eq!(name1, name2); + + assert!(PackageName::try_from("invalid$").is_err()); } } diff --git a/crates/rattler_conda_types/src/repo_data/mod.rs b/crates/rattler_conda_types/src/repo_data/mod.rs index f3909e736..5407bf160 100644 --- a/crates/rattler_conda_types/src/repo_data/mod.rs +++ b/crates/rattler_conda_types/src/repo_data/mod.rs @@ -18,7 +18,7 @@ use thiserror::Error; use rattler_macros::sorted; use crate::package::IndexJson; -use crate::{Channel, NoArchType, Platform, RepoDataRecord, VersionWithSource}; +use crate::{Channel, NoArchType, PackageName, Platform, RepoDataRecord, VersionWithSource}; /// [`RepoData`] is an index of package binaries available on in a subdirectory of a Conda channel. // Note: we cannot use the sorted macro here, because the `packages` and `conda_packages` fields are @@ -106,7 +106,7 @@ pub struct PackageRecord { pub md5: Option, /// The name of the package - pub name: String, + pub name: PackageName, /// If this package is independent of architecture this field specifies in what way. See /// [`NoArchType`] for more information. @@ -149,7 +149,13 @@ pub struct PackageRecord { impl Display for PackageRecord { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "{}={}={}", self.name, self.version, self.build) + write!( + f, + "{}={}={}", + self.name.as_normalized(), + self.version, + self.build + ) } } @@ -182,7 +188,7 @@ impl RepoData { impl PackageRecord { /// A simple helper method that constructs a `PackageRecord` with the bare minimum values. - pub fn new(name: String, version: impl Into, build: String) -> Self { + pub fn new(name: PackageName, version: impl Into, build: String) -> Self { Self { arch: None, build, diff --git a/crates/rattler_conda_types/src/repo_data/topological_sort.rs b/crates/rattler_conda_types/src/repo_data/topological_sort.rs index eb6560373..a7a92917b 100644 --- a/crates/rattler_conda_types/src/repo_data/topological_sort.rs +++ b/crates/rattler_conda_types/src/repo_data/topological_sort.rs @@ -18,7 +18,7 @@ pub fn sort_topologically + Clone>(packages: Vec) -> let mut all_packages = packages .iter() .cloned() - .map(|p| (p.as_ref().name.clone(), p)) + .map(|p| (p.as_ref().name.as_normalized().to_owned(), p)) .collect(); // Detect cycles @@ -56,7 +56,10 @@ fn get_graph_roots>( records: &[T], cycle_breaks: Option<&FxHashSet<(String, String)>>, ) -> Vec { - let all_packages: FxHashSet<_> = records.iter().map(|r| r.as_ref().name.as_str()).collect(); + let all_packages: FxHashSet<_> = records + .iter() + .map(|r| r.as_ref().name.as_normalized()) + .collect(); let dependencies: FxHashSet<_> = records .iter() @@ -68,7 +71,8 @@ fn get_graph_roots>( .filter(|d| { // filter out circular dependencies if let Some(cycle_breaks) = cycle_breaks { - !cycle_breaks.contains(&(r.as_ref().name.clone(), d.to_string())) + !cycle_breaks + .contains(&(r.as_ref().name.as_normalized().to_owned(), d.to_string())) } else { true } @@ -236,11 +240,11 @@ mod tests { ) { let all_sorted_packages: FxHashSet<_> = sorted_packages .iter() - .map(|p| p.package_record.name.as_str()) + .map(|p| p.package_record.name.as_normalized()) .collect(); let all_original_packages: FxHashSet<_> = original_packages .iter() - .map(|p| p.package_record.name.as_str()) + .map(|p| p.package_record.name.as_normalized()) .collect(); let missing_in_sorted: Vec<_> = all_original_packages .difference(&all_sorted_packages) @@ -274,7 +278,7 @@ mod tests { .iter() .map(|p| { ( - p.package_record.name.as_str(), + p.package_record.name.as_normalized(), p.package_record.depends.as_slice(), ) }) @@ -282,7 +286,7 @@ mod tests { let mut installed = FxHashSet::default(); for (i, p) in sorted_packages.iter().enumerate() { - let name = p.package_record.name.as_str(); + let name = p.package_record.name.as_normalized(); let &deps = packages_by_name.get(name).unwrap(); // All the package's dependencies must have already been installed @@ -347,7 +351,10 @@ mod tests { // Sanity check: the last package should be python (or pip when it is present) let last_package = &sorted_packages[sorted_packages.len() - 1]; - assert_eq!(last_package.package_record.name, expected_last_package) + assert_eq!( + last_package.package_record.name.as_normalized(), + expected_last_package + ) } fn get_resolved_packages_for_two_roots() -> Vec { diff --git a/crates/rattler_libsolv_rs/src/conda_util.rs b/crates/rattler_libsolv_rs/src/conda_util.rs index cef078db1..49fa28cc3 100644 --- a/crates/rattler_libsolv_rs/src/conda_util.rs +++ b/crates/rattler_libsolv_rs/src/conda_util.rs @@ -3,7 +3,7 @@ use crate::id::{NameId, SolvableId}; use crate::mapping::Mapping; use crate::solvable::Solvable; use crate::MatchSpecId; -use rattler_conda_types::{MatchSpec, Version}; +use rattler_conda_types::{MatchSpec, PackageName, Version}; use std::cell::OnceCell; use std::cmp::Ordering; use std::collections::HashMap; @@ -14,7 +14,7 @@ pub(crate) fn compare_candidates( a: SolvableId, b: SolvableId, solvables: &Arena, - interned_strings: &HashMap, + interned_strings: &HashMap, packages_by_name: &Mapping>, match_specs: &Arena, match_spec_to_candidates: &Mapping>>, @@ -140,7 +140,7 @@ pub(crate) fn compare_candidates( pub(crate) fn find_highest_version( match_spec_id: MatchSpecId, solvables: &Arena, - interned_strings: &HashMap, + interned_strings: &HashMap, packages_by_name: &Mapping>, match_specs: &Arena, match_spec_to_candidates: &Mapping>>, @@ -184,7 +184,7 @@ pub(crate) fn find_highest_version( pub(crate) fn find_candidates<'b>( match_spec_id: MatchSpecId, match_specs: &Arena, - names_to_ids: &HashMap, + names_to_ids: &HashMap, packages_by_name: &Mapping>, solvables: &Arena, match_spec_to_candidates: &'b Mapping>>, @@ -192,7 +192,7 @@ pub(crate) fn find_candidates<'b>( match_spec_to_candidates[match_spec_id].get_or_init(|| { let match_spec = &match_specs[match_spec_id]; let Some(match_spec_name) = match_spec.name.as_ref() else { return Vec::new() }; - let Some(name_id) = names_to_ids.get(match_spec_name.as_normalized().as_ref()) else { return Vec::new() }; + let Some(name_id) = names_to_ids.get(match_spec_name) else { return Vec::new() }; packages_by_name[*name_id] .iter() diff --git a/crates/rattler_libsolv_rs/src/pool.rs b/crates/rattler_libsolv_rs/src/pool.rs index f164cd986..76f26627e 100644 --- a/crates/rattler_libsolv_rs/src/pool.rs +++ b/crates/rattler_libsolv_rs/src/pool.rs @@ -22,10 +22,10 @@ pub struct Pool<'a> { total_repos: u32, /// Interned package names - package_names: Arena, + package_names: Arena, /// Map from package names to the id of their interned counterpart - pub(crate) names_to_ids: HashMap, + pub(crate) names_to_ids: HashMap, /// Map from interned package names to the solvables that have that name pub(crate) packages_by_name: Mapping>, @@ -136,10 +136,7 @@ impl<'a> Pool<'a> { ) { let match_spec = &self.match_specs[match_spec_id]; let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); - let name_id = match self - .names_to_ids - .get(match_spec_name.as_normalized().as_ref()) - { + let name_id = match self.names_to_ids.get(&match_spec_name) { None => return, Some(&name_id) => name_id, }; @@ -188,10 +185,7 @@ impl<'a> Pool<'a> { ) { let match_spec = &self.match_specs[match_spec_id]; let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); - let name_id = match self - .names_to_ids - .get(match_spec_name.as_normalized().as_ref()) - { + let name_id = match self.names_to_ids.get(&match_spec_name) { None => return, Some(&name_id) => name_id, }; @@ -228,12 +222,8 @@ impl<'a> Pool<'a> { } /// Interns a package name into the `Pool`, returning its `NameId` - fn intern_package_name>(&mut self, name: T) -> NameId { - let package_name = name.into(); - match self - .names_to_ids - .entry(package_name.as_normalized().to_string()) - { + fn intern_package_name(&mut self, name: &PackageName) -> NameId { + match self.names_to_ids.entry(name.clone()) { Entry::Occupied(e) => *e.get(), Entry::Vacant(e) => { let next_id = self.package_names.alloc(e.key().clone()); @@ -250,7 +240,7 @@ impl<'a> Pool<'a> { /// Returns the package name associated to the provided id /// /// Panics if the package name is not found in the pool - pub fn resolve_package_name(&self, name_id: NameId) -> &str { + pub fn resolve_package_name(&self, name_id: NameId) -> &PackageName { &self.package_names[name_id] } diff --git a/crates/rattler_libsolv_rs/src/problem.rs b/crates/rattler_libsolv_rs/src/problem.rs index 8ba524554..3b810be88 100644 --- a/crates/rattler_libsolv_rs/src/problem.rs +++ b/crates/rattler_libsolv_rs/src/problem.rs @@ -100,7 +100,7 @@ impl Problem { .cloned() .find(|&ms| { let ms = solver.pool().resolve_match_spec(ms); - ms.name.as_ref().unwrap().as_normalized().as_ref() == dep.record.name + ms.name.as_ref().unwrap() == &dep.record.name }) .unwrap(); @@ -636,9 +636,13 @@ impl<'a> DisplayUnsat<'a> { let is_leaf = graph.edges(candidate).next().is_none(); if is_leaf { - writeln!(f, "{indent}{} {version}", solvable.record.name)?; + writeln!( + f, + "{indent}{} {version}", + solvable.record.name.as_normalized() + )?; } else if already_installed { - writeln!(f, "{indent}{} {version}, which conflicts with the versions reported above.", solvable.record.name)?; + writeln!(f, "{indent}{} {version}, which conflicts with the versions reported above.", solvable.record.name.as_normalized())?; } else if constrains_conflict { let match_specs = graph .edges(candidate) @@ -653,7 +657,7 @@ impl<'a> DisplayUnsat<'a> { writeln!( f, "{indent}{} {version} would constrain", - solvable.record.name + solvable.record.name.as_normalized() )?; let indent = Self::get_indent(depth + 1, top_level_indent); @@ -669,7 +673,7 @@ impl<'a> DisplayUnsat<'a> { writeln!( f, "{indent}{} {version} would require", - solvable.record.name + solvable.record.name.as_normalized() )?; let requirements = graph .edges(candidate) @@ -735,7 +739,8 @@ impl fmt::Display for DisplayUnsat<'_> { writeln!( f, "{indent}{} {} is locked, but another version is required as reported above", - locked.record.name, locked.record.version + locked.record.name.as_normalized(), + locked.record.version )?; } } diff --git a/crates/rattler_libsolv_rs/src/solvable.rs b/crates/rattler_libsolv_rs/src/solvable.rs index 37d0a7352..9bb68b703 100644 --- a/crates/rattler_libsolv_rs/src/solvable.rs +++ b/crates/rattler_libsolv_rs/src/solvable.rs @@ -73,7 +73,7 @@ impl<'a> Solvable<'a> { build: None, }, SolvableInner::Package(p) => SolvableDisplay { - name: &p.record.name, + name: p.record.name.as_normalized(), version: Some(&p.record.version), build: Some(&p.record.build), }, diff --git a/crates/rattler_libsolv_rs/src/solver/clause.rs b/crates/rattler_libsolv_rs/src/solver/clause.rs index 7b862656e..ad4e46e4b 100644 --- a/crates/rattler_libsolv_rs/src/solver/clause.rs +++ b/crates/rattler_libsolv_rs/src/solver/clause.rs @@ -434,7 +434,7 @@ impl Debug for ClauseDebug<'_> { .package() .record .name - .as_str(); + .as_normalized(); write!(f, "only one {name} allowed") } } diff --git a/crates/rattler_libsolv_rs/src/solver/mod.rs b/crates/rattler_libsolv_rs/src/solver/mod.rs index 246852d38..b43614497 100644 --- a/crates/rattler_libsolv_rs/src/solver/mod.rs +++ b/crates/rattler_libsolv_rs/src/solver/mod.rs @@ -923,7 +923,7 @@ mod test { license: None, license_family: None, md5: None, - name: name.to_string(), + name: name.parse().unwrap(), noarch: Default::default(), platform: None, sha256: None, @@ -1003,7 +1003,7 @@ mod test { .pool .resolve_solvable_inner(solved.steps[0]) .package(); - assert_eq!(solvable.record.name, "asdf"); + assert_eq!(solvable.record.name.as_normalized(), "asdf"); assert_eq!(solvable.record.version.to_string(), "1.2.3"); } @@ -1023,14 +1023,14 @@ mod test { .pool .resolve_solvable_inner(solved.steps[0]) .package(); - assert_eq!(solvable.record.name, "asdf"); + assert_eq!(solvable.record.name.as_normalized(), "asdf"); assert_eq!(solvable.record.version.to_string(), "1.2.3"); let solvable = solver .pool .resolve_solvable_inner(solved.steps[1]) .package(); - assert_eq!(solvable.record.name, "efgh"); + assert_eq!(solvable.record.name.as_normalized(), "efgh"); assert_eq!(solvable.record.version.to_string(), "4.5.6"); } @@ -1051,14 +1051,14 @@ mod test { .pool .resolve_solvable_inner(solved.steps[0]) .package(); - assert_eq!(solvable.record.name, "asdf"); + assert_eq!(solvable.record.name.as_normalized(), "asdf"); assert_eq!(solvable.record.version.to_string(), "1.2.4"); let solvable = solver .pool .resolve_solvable_inner(solved.steps[1]) .package(); - assert_eq!(solvable.record.name, "efgh"); + assert_eq!(solvable.record.name.as_normalized(), "efgh"); assert_eq!(solvable.record.version.to_string(), "4.5.7"); } @@ -1101,7 +1101,7 @@ mod test { .pool .resolve_solvable_inner(solved.steps[0]) .package(); - assert_eq!(solvable.record.name, "asdf"); + assert_eq!(solvable.record.name.as_normalized(), "asdf"); assert_eq!(solvable.record.version.to_string(), "1.2.3"); } @@ -1169,7 +1169,7 @@ mod test { .pool .resolve_solvable_inner(solved.steps[0]) .package(); - assert_eq!(solvable.record.name, "asdf"); + assert_eq!(solvable.record.name.as_normalized(), "asdf"); assert_eq!(solvable.record.version, Version::from_str("1.2.4").unwrap()); } diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap index 12ca4df4e..977e8300f 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_missing_top_level_dep_2.snap @@ -1,6 +1,7 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs +assertion_line: 1306 expression: error --- -No candidates where found for b 14.*. +No candidates were found for b 14.*. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap index e2616024b..4b2e9a15b 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_1.snap @@ -1,8 +1,9 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs +assertion_line: 1281 expression: error --- asdf cannot be installed because there are no viable options: |-- asdf 1.2.3 would require - |-- C >1, for which no candidates were found. + |-- c >1, for which no candidates were found. diff --git a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap index a75e5d80c..5beda1977 100644 --- a/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap +++ b/crates/rattler_libsolv_rs/src/solver/snapshots/rattler_libsolv_rs__solver__test__unsat_no_candidates_for_child_2.snap @@ -1,8 +1,9 @@ --- source: crates/rattler_libsolv_rs/src/solver/mod.rs +assertion_line: 1290 expression: error --- -A <1000 cannot be installed because there are no viable options: +a <1000 cannot be installed because there are no viable options: |-- a 41 would require - |-- B <20, for which no candidates where found. + |-- b <20, for which no candidates were found. diff --git a/crates/rattler_solve/src/libsolv_c/input.rs b/crates/rattler_solve/src/libsolv_c/input.rs index 867028401..25e28288e 100644 --- a/crates/rattler_solve/src/libsolv_c/input.rs +++ b/crates/rattler_solve/src/libsolv_c/input.rs @@ -84,7 +84,7 @@ pub fn add_repodata_records<'a>( let record = &repo_data.package_record; // Name and version - solvable.name = pool.intern_str(record.name.as_str()).into(); + solvable.name = pool.intern_str(record.name.as_normalized()).into(); solvable.evr = pool.intern_str(record.version.to_string()).into(); let rel_eq = pool.rel_eq(solvable.name, solvable.evr); repo.add_provides(solvable, rel_eq); @@ -248,7 +248,7 @@ pub fn add_virtual_packages(pool: &Pool, repo: &Repo, packages: &[GenericVirtual let solvable = unsafe { solvable_id.resolve_raw(pool).as_mut() }; // Name and version - solvable.name = pool.intern_str(package.name.as_str()).into(); + solvable.name = pool.intern_str(package.name.as_normalized()).into(); solvable.evr = pool.intern_str(package.version.to_string()).into(); let rel_eq = pool.rel_eq(solvable.name, solvable.evr); repo.add_provides(solvable, rel_eq); diff --git a/crates/rattler_solve/tests/backends.rs b/crates/rattler_solve/tests/backends.rs index b97230c6e..fd1dc2cf7 100644 --- a/crates/rattler_solve/tests/backends.rs +++ b/crates/rattler_solve/tests/backends.rs @@ -75,7 +75,7 @@ fn installed_package( channel: channel.to_string(), file_name: "dummy-filename".to_string(), package_record: PackageRecord { - name: name.to_string(), + name: name.parse().unwrap(), version: version.parse().unwrap(), build: build.to_string(), build_number, @@ -129,7 +129,9 @@ fn solve_real_world(specs: Vec<&str>) -> Vec { .map(|pkg| { format!( "{} {} {}", - pkg.package_record.name, pkg.package_record.version, pkg.package_record.build + pkg.package_record.name.as_normalized(), + pkg.package_record.version, + pkg.package_record.build ) }) .collect::>(); @@ -215,7 +217,7 @@ macro_rules! solver_backend_tests { dummy_channel_json_path(), Vec::new(), vec![GenericVirtualPackage { - name: "__unix".to_string(), + name: rattler_conda_types::PackageName::new_unchecked("__unix"), version: Version::from_str("0").unwrap(), build_string: "0".to_string(), }], @@ -226,7 +228,7 @@ macro_rules! solver_backend_tests { assert_eq!(pkgs.len(), 1); let info = &pkgs[0]; - assert_eq!("bar", &info.package_record.name); + assert_eq!("bar", info.package_record.name.as_normalized()); assert_eq!("1.2.3", &info.package_record.version.to_string()); } @@ -249,7 +251,7 @@ macro_rules! solver_backend_tests { info.url.to_string() ); assert_eq!("https://conda.anaconda.org/conda-forge/", info.channel); - assert_eq!("foo", info.package_record.name); + assert_eq!("foo", info.package_record.name.as_normalized()); assert_eq!("linux-64", info.package_record.subdir); assert_eq!("3.0.2", info.package_record.version.to_string()); assert_eq!("py36h1af98f8_1", info.package_record.build); @@ -313,7 +315,7 @@ macro_rules! solver_backend_tests { // Install let info = &pkgs[0]; - assert_eq!("foo", &info.package_record.name); + assert_eq!("foo", info.package_record.name.as_normalized()); assert_eq!("3.0.2", &info.package_record.version.to_string()); } @@ -338,7 +340,7 @@ macro_rules! solver_backend_tests { // Install let info = &pkgs[0]; - assert_eq!("foo", &info.package_record.name); + assert_eq!("foo", info.package_record.name.as_normalized()); assert_eq!("4.0.2", &info.package_record.version.to_string()); } @@ -365,7 +367,7 @@ macro_rules! solver_backend_tests { // Uninstall let info = &pkgs[0]; - assert_eq!("foo", &info.package_record.name); + assert_eq!("foo", info.package_record.name.as_normalized()); assert_eq!("3.0.2", &info.package_record.version.to_string()); } @@ -443,7 +445,7 @@ mod libsolv_c { info.url.to_string() ); assert_eq!("https://conda.anaconda.org/conda-forge/", info.channel); - assert_eq!("foo", info.package_record.name); + assert_eq!("foo", info.package_record.name.as_normalized()); assert_eq!("linux-64", info.package_record.subdir); assert_eq!("3.0.2", info.package_record.version.to_string()); assert_eq!("py36h1af98f8_1", info.package_record.build); @@ -515,7 +517,7 @@ fn compare_solve(specs: Vec<&str>) { let names = specs .iter() .filter_map(|s| s.name.as_ref()) - .map(|name| name.as_normalized().as_ref()); + .map(|name| name.as_normalized()); let available_packages = SparseRepoData::load_records_recursive(sparse_repo_datas, names, None).unwrap(); @@ -525,7 +527,9 @@ fn compare_solve(specs: Vec<&str>) { .map(|pkg| { format!( "{} {} {}", - pkg.package_record.name, pkg.package_record.version, pkg.package_record.build + pkg.package_record.name.as_normalized(), + pkg.package_record.version, + pkg.package_record.build ) }) .collect::>(); diff --git a/crates/rattler_virtual_packages/src/lib.rs b/crates/rattler_virtual_packages/src/lib.rs index d30542911..abda6d32e 100644 --- a/crates/rattler_virtual_packages/src/lib.rs +++ b/crates/rattler_virtual_packages/src/lib.rs @@ -34,7 +34,7 @@ pub mod linux; pub mod osx; use once_cell::sync::OnceCell; -use rattler_conda_types::{GenericVirtualPackage, Platform, Version}; +use rattler_conda_types::{GenericVirtualPackage, PackageName, Platform, Version}; use std::str::FromStr; use crate::osx::ParseOsxVersionError; @@ -71,12 +71,12 @@ impl From for GenericVirtualPackage { fn from(package: VirtualPackage) -> Self { match package { VirtualPackage::Win => GenericVirtualPackage { - name: "__win".into(), + name: PackageName::new_unchecked("__win"), version: Version::from_str("0").unwrap(), build_string: "0".into(), }, VirtualPackage::Unix => GenericVirtualPackage { - name: "__unix".into(), + name: PackageName::new_unchecked("__unix"), version: Version::from_str("0").unwrap(), build_string: "0".into(), }, @@ -174,7 +174,7 @@ impl Linux { impl From for GenericVirtualPackage { fn from(linux: Linux) -> Self { GenericVirtualPackage { - name: "__linux".into(), + name: PackageName::new_unchecked("__linux"), version: linux.version, build_string: "0".into(), } @@ -211,7 +211,9 @@ impl LibC { impl From for GenericVirtualPackage { fn from(libc: LibC) -> Self { GenericVirtualPackage { - name: format!("__{}", libc.family.to_lowercase()), + name: format!("__{}", libc.family.to_lowercase()) + .try_into() + .unwrap(), version: libc.version, build_string: "0".into(), } @@ -242,7 +244,7 @@ impl Cuda { impl From for GenericVirtualPackage { fn from(cuda: Cuda) -> Self { GenericVirtualPackage { - name: "__cuda".into(), + name: PackageName::new_unchecked("__cuda"), version: cuda.version, build_string: "0".into(), } @@ -296,7 +298,7 @@ impl Archspec { impl From for GenericVirtualPackage { fn from(archspec: Archspec) -> Self { GenericVirtualPackage { - name: "__archspec".into(), + name: PackageName::new_unchecked("__archspec"), version: Version::from_str("1").unwrap(), build_string: archspec.spec, } @@ -330,7 +332,7 @@ impl Osx { impl From for GenericVirtualPackage { fn from(osx: Osx) -> Self { GenericVirtualPackage { - name: "__osx".into(), + name: PackageName::new_unchecked("__osx"), version: osx.version, build_string: "0".into(), } From 6469809505ccf651429c99d095ac52c13345f5e6 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 14:38:35 +0200 Subject: [PATCH 04/10] fix: lint and format --- crates/rattler_conda_types/src/lib.rs | 2 +- crates/rattler_conda_types/src/package_name.rs | 2 +- crates/rattler_libsolv_rs/src/pool.rs | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/rattler_conda_types/src/lib.rs b/crates/rattler_conda_types/src/lib.rs index 15c6cd087..dd03ac522 100644 --- a/crates/rattler_conda_types/src/lib.rs +++ b/crates/rattler_conda_types/src/lib.rs @@ -32,7 +32,7 @@ pub use match_spec::matcher::StringMatcher; pub use match_spec::parse::ParseMatchSpecError; pub use match_spec::{MatchSpec, NamelessMatchSpec}; pub use no_arch_type::{NoArchKind, NoArchType}; -pub use package_name::{PackageName, InvalidPackageNameError}; +pub use package_name::{InvalidPackageNameError, PackageName}; pub use platform::{ParsePlatformError, Platform}; pub use prefix_record::PrefixRecord; pub use repo_data::patches::{PackageRecordPatch, PatchInstructions, RepoDataPatch}; diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs index 94f505dd7..b31f3662a 100644 --- a/crates/rattler_conda_types/src/package_name.rs +++ b/crates/rattler_conda_types/src/package_name.rs @@ -120,7 +120,7 @@ impl PartialOrd for PackageName { impl Ord for PackageName { fn cmp(&self, other: &Self) -> Ordering { - self.as_normalized().cmp(&other.as_normalized()) + self.as_normalized().cmp(other.as_normalized()) } } diff --git a/crates/rattler_libsolv_rs/src/pool.rs b/crates/rattler_libsolv_rs/src/pool.rs index 76f26627e..04f55504b 100644 --- a/crates/rattler_libsolv_rs/src/pool.rs +++ b/crates/rattler_libsolv_rs/src/pool.rs @@ -136,7 +136,7 @@ impl<'a> Pool<'a> { ) { let match_spec = &self.match_specs[match_spec_id]; let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); - let name_id = match self.names_to_ids.get(&match_spec_name) { + let name_id = match self.names_to_ids.get(match_spec_name) { None => return, Some(&name_id) => name_id, }; @@ -185,7 +185,7 @@ impl<'a> Pool<'a> { ) { let match_spec = &self.match_specs[match_spec_id]; let match_spec_name = match_spec.name.as_ref().expect("match spec without name!"); - let name_id = match self.names_to_ids.get(&match_spec_name) { + let name_id = match self.names_to_ids.get(match_spec_name) { None => return, Some(&name_id) => name_id, }; From 8a37fdb17bba63087c726c5e0d192cfe37934437 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 14:40:51 +0200 Subject: [PATCH 05/10] fix: doc link --- crates/rattler_conda_types/src/package_name.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs index b31f3662a..3a18ff6ec 100644 --- a/crates/rattler_conda_types/src/package_name.rs +++ b/crates/rattler_conda_types/src/package_name.rs @@ -11,9 +11,9 @@ use thiserror::Error; /// /// Conda package names are always lowercase and can only contain ascii characters. /// -/// This struct explicitly does not implement [`Display`] because its ambiguous if that would -/// display the source or the normalized version. Simply call `as_source` or `as_normalized` to make -/// the distinction. +/// This struct explicitly does not implement [`std::fmt::Display`] because its ambiguous if that +/// would display the source or the normalized version. Simply call `as_source` or `as_normalized` +/// to make the distinction. #[derive(Debug, Clone, Eq, DeserializeFromStr)] pub struct PackageName { normalized: Option, From 506d9c703c5f40d160d5d1c9c7b4b7a01006461b Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 15:19:06 +0200 Subject: [PATCH 06/10] feat: also fix bench --- crates/rattler-bin/src/commands/create.rs | 5 +-- .../src/sparse/mod.rs | 35 ++++++++++--------- crates/rattler_solve/tests/backends.rs | 9 ++--- 3 files changed, 22 insertions(+), 27 deletions(-) diff --git a/crates/rattler-bin/src/commands/create.rs b/crates/rattler-bin/src/commands/create.rs index 5fb21c65c..5cb41882a 100644 --- a/crates/rattler-bin/src/commands/create.rs +++ b/crates/rattler-bin/src/commands/create.rs @@ -157,10 +157,7 @@ pub async fn create(opt: Opt) -> anyhow::Result<()> { .collect::, _>>()?; // Get the package names from the matchspecs so we can only load the package records that we need. - let package_names = specs - .iter() - .filter_map(|spec| spec.name.as_ref()) - .map(|name| name.as_normalized()); + let package_names = specs.iter().filter_map(|spec| spec.name.as_ref().cloned()); let repodatas = wrap_in_progress("parsing repodata", move || { SparseRepoData::load_records_recursive( &sparse_repo_datas, diff --git a/crates/rattler_repodata_gateway/src/sparse/mod.rs b/crates/rattler_repodata_gateway/src/sparse/mod.rs index 58aa4d318..0f7e7f4ad 100644 --- a/crates/rattler_repodata_gateway/src/sparse/mod.rs +++ b/crates/rattler_repodata_gateway/src/sparse/mod.rs @@ -3,7 +3,7 @@ use futures::{stream, StreamExt, TryFutureExt, TryStreamExt}; use itertools::Itertools; -use rattler_conda_types::{Channel, PackageRecord, RepoDataRecord}; +use rattler_conda_types::{Channel, PackageName, PackageRecord, RepoDataRecord}; use serde::{ de::{Error, MapAccess, Visitor}, Deserialize, Deserializer, @@ -88,7 +88,7 @@ impl SparseRepoData { } /// Returns all the records for the specified package name. - pub fn load_records(&self, package_name: &str) -> io::Result> { + pub fn load_records(&self, package_name: &PackageName) -> io::Result> { let repo_data = self.inner.borrow_repo_data(); let mut records = parse_records( package_name, @@ -115,7 +115,7 @@ impl SparseRepoData { /// depend on. pub fn load_records_recursive<'a>( repo_data: impl IntoIterator, - package_names: impl IntoIterator>, + package_names: impl IntoIterator, patch_function: Option, ) -> io::Result>> { let repo_data: Vec<_> = repo_data.into_iter().collect(); @@ -124,8 +124,7 @@ impl SparseRepoData { let mut result = Vec::from_iter((0..repo_data.len()).map(|_| Vec::new())); // Construct a set of packages that we have seen and have been added to the pending list. - let mut seen: HashSet = - HashSet::from_iter(package_names.into_iter().map(Into::into)); + let mut seen: HashSet = HashSet::from_iter(package_names); // Construct a queue to store packages in that still need to be processed let mut pending = VecDeque::from_iter(seen.iter().cloned()); @@ -155,11 +154,12 @@ impl SparseRepoData { // Iterate over all packages to find recursive dependencies. for record in records.iter() { for dependency in &record.package_record.depends { - let dependency_name = - dependency.split_once(' ').unwrap_or((dependency, "")).0; - if !seen.contains(dependency_name) { - pending.push_back(dependency_name.to_string()); - seen.insert(dependency_name.to_string()); + let dependency_name = PackageName::new_unchecked( + dependency.split_once(' ').unwrap_or((dependency, "")).0, + ); + if !seen.contains(&dependency_name) { + pending.push_back(dependency_name.clone()); + seen.insert(dependency_name); } } } @@ -194,7 +194,7 @@ struct LazyRepoData<'i> { /// Parse the records for the specified package from the raw index fn parse_records<'i>( - package_name: &str, + package_name: &PackageName, packages: &[(PackageFilename<'i>, &'i RawValue)], channel: &Channel, subdir: &str, @@ -202,7 +202,8 @@ fn parse_records<'i>( ) -> io::Result> { let channel_name = channel.canonical_name(); - let package_indices = packages.equal_range_by(|(package, _)| package.package.cmp(package_name)); + let package_indices = + packages.equal_range_by(|(package, _)| package.package.cmp(package_name.as_normalized())); let mut result = Vec::with_capacity(package_indices.len()); for (key, raw_json) in &packages[package_indices] { let mut package_record: PackageRecord = serde_json::from_str(raw_json.get())?; @@ -237,7 +238,7 @@ fn parse_records<'i>( /// it has been loaded. pub async fn load_repo_data_recursively( repo_data_paths: impl IntoIterator, impl AsRef)>, - package_names: impl IntoIterator>, + package_names: impl IntoIterator, patch_function: Option, ) -> Result>, io::Error> { // Open the different files and memory map them to get access to their bytes. Do this in parallel. @@ -360,7 +361,7 @@ impl<'de> TryFrom<&'de str> for PackageFilename<'de> { #[cfg(test)] mod test { use super::{load_repo_data_recursively, PackageFilename}; - use rattler_conda_types::{Channel, ChannelConfig, RepoData, RepoDataRecord}; + use rattler_conda_types::{Channel, ChannelConfig, PackageName, RepoData, RepoDataRecord}; use rstest::rstest; use std::path::{Path, PathBuf}; @@ -369,7 +370,7 @@ mod test { } async fn load_sparse( - package_names: impl IntoIterator>, + package_names: impl IntoIterator>, ) -> Vec> { load_repo_data_recursively( [ @@ -384,7 +385,9 @@ mod test { test_dir().join("channels/conda-forge/linux-64/repodata.json"), ), ], - package_names, + package_names + .into_iter() + .map(|name| PackageName::try_from(name.as_ref()).unwrap()), None, ) .await diff --git a/crates/rattler_solve/tests/backends.rs b/crates/rattler_solve/tests/backends.rs index fd1dc2cf7..e9875e0cd 100644 --- a/crates/rattler_solve/tests/backends.rs +++ b/crates/rattler_solve/tests/backends.rs @@ -107,9 +107,7 @@ fn solve_real_world(specs: Vec<&str>) -> Vec { let sparse_repo_datas = read_real_world_repo_data(); - let names = specs - .iter() - .map(|s| s.name.as_ref().unwrap().as_normalized().to_string()); + let names = specs.iter().filter_map(|s| s.name.as_ref().cloned()); let available_packages = SparseRepoData::load_records_recursive(sparse_repo_datas, names, None).unwrap(); @@ -514,10 +512,7 @@ fn compare_solve(specs: Vec<&str>) { let sparse_repo_datas = read_real_world_repo_data(); - let names = specs - .iter() - .filter_map(|s| s.name.as_ref()) - .map(|name| name.as_normalized()); + let names = specs.iter().filter_map(|s| s.name.as_ref().cloned()); let available_packages = SparseRepoData::load_records_recursive(sparse_repo_datas, names, None).unwrap(); From e176940de1663dcd38ff70c8540614cc868672df Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 15:31:48 +0200 Subject: [PATCH 07/10] feat: assume package names are valid in repodata --- crates/rattler_conda_types/src/package_name.rs | 15 +++++++++++++-- crates/rattler_conda_types/src/repo_data/mod.rs | 7 +++++-- crates/rattler_conda_types/src/utils/serde.rs | 3 +++ 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs index 3a18ff6ec..a33d201f0 100644 --- a/crates/rattler_conda_types/src/package_name.rs +++ b/crates/rattler_conda_types/src/package_name.rs @@ -1,9 +1,10 @@ -use serde::{Serialize, Serializer}; -use serde_with::DeserializeFromStr; +use serde::{Deserialize, Deserializer, Serialize, Serializer}; +use serde_with::{DeserializeAs, DeserializeFromStr}; use std::cmp::Ordering; use std::hash::{Hash, Hasher}; use std::str::FromStr; use thiserror::Error; +use crate::utils::serde::DeserializeFromStrUnchecked; /// A representation of a conda package name. This struct both stores the source string from which /// this instance was created as well as a normalized name that can be used to compare different @@ -133,6 +134,16 @@ impl Serialize for PackageName { } } +impl<'de> DeserializeAs<'de, PackageName> for DeserializeFromStrUnchecked { + fn deserialize_as(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let source = String::deserialize(deserializer)?; + Ok(PackageName::new_unchecked(source)) + } +} + #[cfg(test)] mod test { use super::*; diff --git a/crates/rattler_conda_types/src/repo_data/mod.rs b/crates/rattler_conda_types/src/repo_data/mod.rs index 5407bf160..e925c0b73 100644 --- a/crates/rattler_conda_types/src/repo_data/mod.rs +++ b/crates/rattler_conda_types/src/repo_data/mod.rs @@ -17,8 +17,10 @@ use thiserror::Error; use rattler_macros::sorted; -use crate::package::IndexJson; -use crate::{Channel, NoArchType, PackageName, Platform, RepoDataRecord, VersionWithSource}; +use crate::{ + package::IndexJson, utils::serde::DeserializeFromStrUnchecked, Channel, NoArchType, + PackageName, Platform, RepoDataRecord, VersionWithSource, +}; /// [`RepoData`] is an index of package binaries available on in a subdirectory of a Conda channel. // Note: we cannot use the sorted macro here, because the `packages` and `conda_packages` fields are @@ -106,6 +108,7 @@ pub struct PackageRecord { pub md5: Option, /// The name of the package + #[serde_as(deserialize_as = "DeserializeFromStrUnchecked")] pub name: PackageName, /// If this package is independent of architecture this field specifies in what way. See diff --git a/crates/rattler_conda_types/src/utils/serde.rs b/crates/rattler_conda_types/src/utils/serde.rs index 03a02c82c..c380c86d7 100644 --- a/crates/rattler_conda_types/src/utils/serde.rs +++ b/crates/rattler_conda_types/src/utils/serde.rs @@ -172,3 +172,6 @@ impl> SerializeAs> for Ordered { SerializeAsWrap::, Vec<&TAs>>::new(&elements).serialize(serializer) } } + +/// A helper struct to deserialize types from a string without checking the string. +pub struct DeserializeFromStrUnchecked; From 2b2a481cd986ab9ea7fc408a6377d9e339b897ba Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Thu, 24 Aug 2023 15:46:06 +0200 Subject: [PATCH 08/10] fix: format .. --- crates/rattler_conda_types/src/package_name.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/rattler_conda_types/src/package_name.rs b/crates/rattler_conda_types/src/package_name.rs index a33d201f0..f831000b9 100644 --- a/crates/rattler_conda_types/src/package_name.rs +++ b/crates/rattler_conda_types/src/package_name.rs @@ -1,10 +1,10 @@ +use crate::utils::serde::DeserializeFromStrUnchecked; use serde::{Deserialize, Deserializer, Serialize, Serializer}; use serde_with::{DeserializeAs, DeserializeFromStr}; use std::cmp::Ordering; use std::hash::{Hash, Hasher}; use std::str::FromStr; use thiserror::Error; -use crate::utils::serde::DeserializeFromStrUnchecked; /// A representation of a conda package name. This struct both stores the source string from which /// this instance was created as well as a normalized name that can be used to compare different From 369c54b2a954b3246b9cc35d661558e03c691fd8 Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Mon, 28 Aug 2023 14:45:26 +0200 Subject: [PATCH 09/10] fix: package name in wrapper --- py-rattler/rattler/match_spec/match_spec.py | 12 +++++++++++- py-rattler/src/error.rs | 9 ++++++++- py-rattler/src/lib.rs | 9 ++++++++- py-rattler/src/match_spec.rs | 13 ++++++++----- 4 files changed, 35 insertions(+), 8 deletions(-) diff --git a/py-rattler/rattler/match_spec/match_spec.py b/py-rattler/rattler/match_spec/match_spec.py index d8b4a705c..64e36f794 100644 --- a/py-rattler/rattler/match_spec/match_spec.py +++ b/py-rattler/rattler/match_spec/match_spec.py @@ -1,7 +1,7 @@ from __future__ import annotations from typing import Self, TYPE_CHECKING -from rattler.rattler import PyMatchSpec +from rattler.rattler import PyMatchSpec, InvalidPackageNameError if TYPE_CHECKING: from rattler.match_spec import NamelessMatchSpec @@ -105,6 +105,16 @@ def from_nameless(cls, spec: NamelessMatchSpec, name: str) -> Self: """ Constructs a MatchSpec from a NamelessMatchSpec and a name. + + Examples + -------- + >>> from rattler import NamelessMatchSpec + >>> spec = NamelessMatchSpec('3.4') + >>> MatchSpec.from_nameless(spec, "foo") + foo ==3.4 + >>> MatchSpec.from_nameless(spec, "$foo") # doctest: +IGNORE_EXCEPTION_DETAIL + Traceback (most recent call last): + exceptions.InvalidPackageNameException """ return cls._from_py_match_spec( PyMatchSpec.from_nameless(spec._nameless_match_spec, name) diff --git a/py-rattler/src/error.rs b/py-rattler/src/error.rs index 982ca38dc..4b7feefc5 100644 --- a/py-rattler/src/error.rs +++ b/py-rattler/src/error.rs @@ -1,14 +1,17 @@ use pyo3::exceptions::PyException; use pyo3::{create_exception, PyErr}; -use rattler_conda_types::{ParseMatchSpecError, ParseVersionError}; +use rattler_conda_types::{InvalidPackageNameError, ParseMatchSpecError, ParseVersionError}; use thiserror::Error; #[derive(Error, Debug)] +#[allow(clippy::enum_variant_names)] pub enum PyRattlerError { #[error(transparent)] InvalidVersion(#[from] ParseVersionError), #[error(transparent)] InvalidMatchSpec(#[from] ParseMatchSpecError), + #[error(transparent)] + InvalidPackageName(#[from] InvalidPackageNameError), } impl From for PyErr { @@ -20,9 +23,13 @@ impl From for PyErr { PyRattlerError::InvalidMatchSpec(err) => { InvalidMatchSpecException::new_err(err.to_string()) } + PyRattlerError::InvalidPackageName(err) => { + InvalidPackageNameException::new_err(err.to_string()) + } } } } create_exception!(exceptions, InvalidVersionException, PyException); create_exception!(exceptions, InvalidMatchSpecException, PyException); +create_exception!(exceptions, InvalidPackageNameException, PyException); diff --git a/py-rattler/src/lib.rs b/py-rattler/src/lib.rs index e6d32d4e8..c73b6fd19 100644 --- a/py-rattler/src/lib.rs +++ b/py-rattler/src/lib.rs @@ -4,7 +4,9 @@ mod nameless_match_spec; mod repo_data; mod version; -use error::{InvalidMatchSpecException, InvalidVersionException, PyRattlerError}; +use error::{ + InvalidMatchSpecException, InvalidPackageNameException, InvalidVersionException, PyRattlerError, +}; use match_spec::PyMatchSpec; use nameless_match_spec::PyNamelessMatchSpec; use repo_data::package_record::PyPackageRecord; @@ -32,6 +34,11 @@ fn rattler(py: Python, m: &PyModule) -> PyResult<()> { py.get_type::(), ) .unwrap(); + m.add( + "InvalidPackageNameError", + py.get_type::(), + ) + .unwrap(); Ok(()) } diff --git a/py-rattler/src/match_spec.rs b/py-rattler/src/match_spec.rs index f29cd076a..7c2a3a241 100644 --- a/py-rattler/src/match_spec.rs +++ b/py-rattler/src/match_spec.rs @@ -1,5 +1,5 @@ use pyo3::{pyclass, pymethods}; -use rattler_conda_types::MatchSpec; +use rattler_conda_types::{MatchSpec, PackageName}; use std::str::FromStr; use crate::{ @@ -47,9 +47,12 @@ impl PyMatchSpec { /// Constructs a PyMatchSpec from a PyNamelessMatchSpec and a name. #[staticmethod] - pub fn from_nameless(spec: &PyNamelessMatchSpec, name: String) -> Self { - Self { - inner: MatchSpec::from_nameless(spec.clone().into(), Some(name)), - } + pub fn from_nameless(spec: &PyNamelessMatchSpec, name: String) -> pyo3::PyResult { + Ok(Self { + inner: MatchSpec::from_nameless( + spec.clone().into(), + Some(PackageName::try_from(name).map_err(PyRattlerError::from)?), + ), + }) } } From 5e50e2134b619411a72c343b7eca7f2a1c25f25c Mon Sep 17 00:00:00 2001 From: Bas Zalmstra Date: Mon, 28 Aug 2023 14:48:57 +0200 Subject: [PATCH 10/10] fix: lint --- py-rattler/rattler/match_spec/match_spec.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/py-rattler/rattler/match_spec/match_spec.py b/py-rattler/rattler/match_spec/match_spec.py index 64e36f794..10ba05c3c 100644 --- a/py-rattler/rattler/match_spec/match_spec.py +++ b/py-rattler/rattler/match_spec/match_spec.py @@ -1,7 +1,7 @@ from __future__ import annotations from typing import Self, TYPE_CHECKING -from rattler.rattler import PyMatchSpec, InvalidPackageNameError +from rattler.rattler import PyMatchSpec if TYPE_CHECKING: from rattler.match_spec import NamelessMatchSpec