From 5aa45b25fac02f62ddbf81d2f28bb4318130f026 Mon Sep 17 00:00:00 2001 From: Joe Neeman Date: Fri, 14 Jun 2024 18:34:10 -0500 Subject: [PATCH] Account for the lock file in resolution (only lightly tested) --- Cargo.lock | 21 ++---- cli/src/package.rs | 1 - package/src/lib.rs | 18 +++++- package/src/manifest.rs | 9 +-- package/src/resolve.rs | 139 +++++++++++++++++++++++++++++++++++++--- 5 files changed, 156 insertions(+), 32 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 14fe235158..6510b6eb89 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1279,7 +1279,7 @@ dependencies = [ "gix-utils", "itoa", "thiserror", - "winnow 0.6.8", + "winnow", ] [[package]] @@ -1374,7 +1374,7 @@ dependencies = [ "smallvec", "thiserror", "unicode-bom", - "winnow 0.6.8", + "winnow", ] [[package]] @@ -1671,7 +1671,7 @@ dependencies = [ "itoa", "smallvec", "thiserror", - "winnow 0.6.8", + "winnow", ] [[package]] @@ -1795,7 +1795,7 @@ dependencies = [ "gix-utils", "maybe-async", "thiserror", - "winnow 0.6.8", + "winnow", ] [[package]] @@ -1828,7 +1828,7 @@ dependencies = [ "gix-validate", "memmap2 0.9.4", "thiserror", - "winnow 0.6.8", + "winnow", ] [[package]] @@ -4486,7 +4486,7 @@ dependencies = [ "serde", "serde_spanned", "toml_datetime", - "winnow 0.5.40", + "winnow", ] [[package]] @@ -5055,15 +5055,6 @@ version = "0.52.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "32b752e52a2da0ddfbdbcc6fceadfeede4c939ed16d13e648833a61dfb611ed8" -[[package]] -name = "winnow" -version = "0.6.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "56c52728401e1dc672a56e81e593e912aa54c78f40246869f78359a2bf24d29d" -dependencies = [ - "memchr", -] - [[package]] name = "winnow" version = "0.6.8" diff --git a/cli/src/package.rs b/cli/src/package.rs index 56cdd16eeb..b26f86164e 100644 --- a/cli/src/package.rs +++ b/cli/src/package.rs @@ -69,7 +69,6 @@ impl PackageCommand { Command::DebugResolution => { let path = self.find_manifest()?; let manifest = ManifestFile::from_path(path.clone())?; - // TODO: if a lockfile exists, account for it in resolution let resolution = manifest.resolve()?; let package_map = resolution.package_map(&manifest)?; print_package_map(&package_map); diff --git a/package/src/lib.rs b/package/src/lib.rs index 30337fff86..56fd6a6208 100644 --- a/package/src/lib.rs +++ b/package/src/lib.rs @@ -66,6 +66,12 @@ mod serde_url { } } +#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)] +pub struct IndexPrecise { + id: index::Id, + version: Version, +} + /// A precise package version, in a format suitable for putting into a lockfile. #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)] pub enum Precise { @@ -84,10 +90,9 @@ pub enum Precise { /// to the top-level package manifest. /// /// Note that when normalizing we only look at the path and not at the actual filesystem. - Path { - path: PathBuf, - }, + Path { path: PathBuf }, Index { + // TODO: IndexPrecise id: index::Id, version: Version, }, @@ -132,6 +137,13 @@ impl Precise { x => x, } } + + pub fn version(&self) -> Option { + match self { + Precise::Index { version, .. } => Some(version.clone()), + _ => None, + } + } } fn repo_root(id: &ObjectId) -> PathBuf { diff --git a/package/src/manifest.rs b/package/src/manifest.rs index a652efb9cc..a47f3b587b 100644 --- a/package/src/manifest.rs +++ b/package/src/manifest.rs @@ -58,10 +58,10 @@ impl ManifestFile { // Evaluate the manifest with an extra contract applied, so that nice error message will be generated. // (Probably they applied the Manifest contract already, but just in case...) // `contract` is `std.package.Manifest` - use nickel_lang_core::term::UnaryOp::StaticAccess; + use nickel_lang_core::term::UnaryOp::RecordAccess; let contract = make::op1( - StaticAccess("Manifest".into()), - make::op1(StaticAccess("package".into()), Term::Var("std".into())), + RecordAccess("Manifest".into()), + make::op1(RecordAccess("package".into()), Term::Var("std".into())), ); prog.add_contract(RuntimeContract::new(contract, Label::default())); @@ -109,7 +109,8 @@ impl ManifestFile { /// Path dependencies aren't resolved recursively: path dependencies /// don't get locked because they can change at any time. pub fn resolve(&self) -> Result { - crate::resolve::resolve(self) + let lock = self.find_lockfile().unwrap_or_default(); + crate::resolve::resolve_with_lock(self, &lock) } /// Determine the fully-resolved dependencies. diff --git a/package/src/resolve.rs b/package/src/resolve.rs index f320c2cf58..e428724aa4 100644 --- a/package/src/resolve.rs +++ b/package/src/resolve.rs @@ -36,12 +36,18 @@ use crate::{ index::{Id, IndexDependency, PackageIndex}, lock::{LockFile, LockFileEntry}, manifest::Realization, - Dependency, ManifestFile, Precise, + util::semver_to_pg, + Dependency, IndexPrecise, ManifestFile, Precise, }; type VersionRange = pubgrub::range::Range; pub struct PackageRegistry { + // The packages whose versions were locked in a lockfile; we'll try to prefer using + // those same versions. We won't absolutely insist on it, because if the manifest + // changed (or some path-dependency changed) then the old locked versions might not + // resolve anymore. + previously_locked: HashMap, index: PackageIndex, realized_unversioned: Realization, } @@ -51,7 +57,8 @@ impl PackageRegistry { &'a self, package: &VirtualPackage, ) -> impl Iterator + 'a { - match package { + let locked_version = self.previously_locked.get(package).cloned(); + let rest = match package { VirtualPackage::Package(Package::Unversioned(_)) => { Box::new(std::iter::once(SemanticVersion::zero())) as Box> } @@ -81,7 +88,12 @@ impl PackageRegistry { Box::new(iter) } - } + }; + + // Put the locked version first, and then the other versions in any order (filtering to ensure that the locked version isn't repeated). + locked_version + .into_iter() + .chain(rest.filter(move |v| Some(v) != locked_version.as_ref())) } pub fn dep(&self, pkg: &Package, version: &SemanticVersion, dep_id: &Id) -> VersionReq { @@ -270,6 +282,22 @@ impl std::fmt::Display for Package { } } +// Makes the precise less precise, by returning the bucket that it falls into. +impl From for Package { + fn from(p: Precise) -> Self { + match p { + Precise::Git { repo, .. } => { + Package::Unversioned(UnversionedPackage::Git { url: repo }) + } + Precise::Path { path } => Package::Unversioned(UnversionedPackage::Path { path }), + Precise::Index { id, version } => Package::Bucket(Bucket { + id, + version: semver_to_pg(version).into(), + }), + } + } +} + #[derive(Debug, Clone, Eq, PartialEq, Hash)] pub enum VirtualPackage { Package(Package), @@ -319,10 +347,25 @@ impl DependencyProvider for PackageRegistry { &self, potential_packages: impl Iterator, ) -> Result<(T, Option), Box> { - Ok(pubgrub::solver::choose_package_with_fewest_versions( - |p| self.list_versions(p), - potential_packages, - )) + // We try to choose the package with the fewest available versions, as the pubgrub + // docs recommend this as a reasonably-performant heuristic. We count a previously locked package + // as having one version (even if we'd theoretically be willing to ignore the lock). + let count_valid = |(p, range): &(T, U)| { + if self.previously_locked.contains_key(p.borrow()) { + 1 + } else { + self.list_versions(p.borrow()) + .filter(|v| range.borrow().contains(v)) + .count() + } + }; + let (pkg, range) = potential_packages + .min_by_key(count_valid) + .expect("potential_packages gave us an empty iterator"); + let version = self + .list_versions(pkg.borrow()) + .find(|v| range.borrow().contains(v)); + Ok((pkg, version)) } fn get_dependencies( @@ -428,6 +471,72 @@ pub struct Resolution { } pub fn resolve(manifest: &ManifestFile) -> Result { + resolve_with_lock(manifest, &LockFile::default()) +} + +fn previously_locked( + top_level: &Package, + lock: &LockFile, +) -> HashMap { + fn precise_to_index(p: &Precise) -> Option { + match p { + Precise::Index { id, version } => Some(IndexPrecise { + id: id.clone(), + version: version.clone(), + }), + _ => None, + } + } + + // A list of (package: Package, version of the package: SemanticVersion, dependency: IndexPrecise) + let pkg_deps = lock + .dependencies + .values() + .filter_map(precise_to_index) + // FIXME: another place we're hardcoding an unversioned version + .map(|dep| (top_level.clone(), SemanticVersion::zero(), dep)) + .chain(lock.packages.iter().flat_map(|(parent, entry)| { + let pkg = Package::from(parent.clone()); + let pkg_version = parent + .version() + .map(semver_to_pg) + .unwrap_or(SemanticVersion::zero()); + entry + .dependencies + .values() + .filter_map(precise_to_index) + .map(move |v| (pkg.clone(), pkg_version, v)) + })); + + // Because of the virtual package system, each parent->dep needs two entries + // in the locked map: one for recording which of the "union" packages the parent + // depends on, and another for recording which precise version the "union" package depends on. + pkg_deps + .flat_map(|(pkg, version, dep)| { + let dep_version = semver_to_pg(dep.version); + let dep_bucket: BucketVersion = dep_version.into(); + [ + ( + VirtualPackage::Union { + source: pkg, + source_version: version, + target: dep.id.clone(), + }, + dep_bucket.into(), + ), + ( + VirtualPackage::Package(Package::Bucket(Bucket { + id: dep.id, + version: dep_bucket, + })), + dep_version, + ), + ] + }) + .collect() +} + +pub fn resolve_with_lock(manifest: &ManifestFile, lock: &LockFile) -> Result { let mut realization = Realization::default(); // pubgrub insists on resolving a top-level package. We'll represent it as a `Path` dependency, @@ -448,10 +557,14 @@ pub fn resolve(manifest: &ManifestFile) -> Result { }; realization .precise - .insert(top_level_dep.into(), precise.clone()); + .insert(top_level_dep.clone().into(), precise.clone()); realization.manifests.insert(precise, manifest.clone()); let registry = PackageRegistry { + previously_locked: dbg!(previously_locked( + &Package::Unversioned(top_level_dep), + lock + )), index: PackageIndex::new(), realized_unversioned: realization, }; @@ -488,6 +601,12 @@ pub fn resolve(manifest: &ManifestFile) -> Result { } impl Resolution { + /// Finds the precise resolved version of this dependency. + /// + /// # Panics + /// + /// Panics if the dependency was not part of the dependency tree that this resolution + /// was generated for. pub fn precise(&self, dep: &Dependency) -> Precise { match dep { Dependency::Git { url } => Precise::Git { @@ -510,6 +629,7 @@ impl Resolution { } } + /// Returns all the dependencies of a package, along with their package-local names. pub fn dependencies(&self, pkg: &Precise) -> HashMap { match pkg { p @ Precise::Git { .. } | p @ Precise::Path { .. } => { @@ -546,6 +666,7 @@ impl Resolution { } } + /// Returns all the resolved packages in the dependency tree. pub fn all_precises(&self) -> Vec { let mut ret: Vec<_> = self.realization.precise.values().cloned().collect(); ret.sort(); @@ -595,7 +716,7 @@ impl Resolution { ) }) }) - .collect(), //,realized_packages.chain(index_packages).collect(), + .collect(), }) }