diff --git a/Cargo.lock b/Cargo.lock index 3a713511d2..343709a4b1 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3075,6 +3075,7 @@ dependencies = [ "anyhow", "clap 4.5.20", "gix", + "serde", "tempfile", "thiserror", ] diff --git a/cli/src/input.rs b/cli/src/input.rs index 2266264b55..5af3d2648d 100644 --- a/cli/src/input.rs +++ b/cli/src/input.rs @@ -57,10 +57,7 @@ impl Prepare for InputOptions { let resolution = manifest.resolve()?; for (pkg_id, versions) in &resolution.package_map { for v in versions { - resolution - .index - .ensure_downloaded(pkg_id, v.clone()) - .unwrap(); + resolution.index.ensure_downloaded(pkg_id, *v).unwrap(); } } let package_map = resolution.package_map(&manifest)?; diff --git a/git/Cargo.toml b/git/Cargo.toml index 7f2e774b0b..dc71d502a7 100644 --- a/git/Cargo.toml +++ b/git/Cargo.toml @@ -7,6 +7,7 @@ edition = "2021" [dependencies] anyhow.workspace = true gix = { workspace = true, features = ["blocking-network-client"] } +serde.workspace = true tempfile.workspace = true thiserror.workspace = true diff --git a/git/src/lib.rs b/git/src/lib.rs index d969da49a5..78b007ae4a 100644 --- a/git/src/lib.rs +++ b/git/src/lib.rs @@ -49,7 +49,7 @@ pub struct Spec { } /// The different kinds of git "thing" that we can target. -#[derive(Clone, Debug, PartialEq, Eq, Hash, Default)] +#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, serde::Serialize, serde::Deserialize)] pub enum Target { /// By default, we target the remote HEAD. #[default] diff --git a/package/src/error.rs b/package/src/error.rs index ee11e5cdb2..0c295efd16 100644 --- a/package/src/error.rs +++ b/package/src/error.rs @@ -1,6 +1,8 @@ use std::path::{Path, PathBuf}; -use nickel_lang_core::{eval::cache::CacheImpl, identifier::Ident, program::Program}; +use nickel_lang_core::{ + eval::cache::CacheImpl, identifier::Ident, package::ObjectId, program::Program, +}; pub enum Error { Io { @@ -16,7 +18,12 @@ pub enum Error { path: PathBuf, }, RestrictedPath { - package: Ident, + /// The url of the git package that tried the bad import. + package_url: Box, + /// The git id of the bad package. + package_commit: ObjectId, + /// The relative path of the bad package within its git repo. + package_path: PathBuf, attempted: PathBuf, restriction: PathBuf, }, @@ -63,13 +70,16 @@ impl std::fmt::Display for Error { } } Error::RestrictedPath { - package, attempted, restriction, + package_url, + package_commit, + package_path, } => { write!( f, - "package {package} tried to import path {}, but can only import from {}", + "git package {package_url}@{package_commit}/{} tried to import path {}, but can only import from {}", + package_path.display(), attempted.display(), restriction.display() ) diff --git a/package/src/index/mod.rs b/package/src/index/mod.rs index 2698d7ef7b..252d2bba4a 100644 --- a/package/src/index/mod.rs +++ b/package/src/index/mod.rs @@ -226,7 +226,7 @@ impl PackageIndex { pub fn ensure_downloaded(&self, id: &Id, v: SemanticVersion) -> anyhow::Result<()> { let package = self - .package(id, v.clone()) + .package(id, v) .ok_or(anyhow!("tried to download an unknown package"))?; let precise = Precise::Index { id: id.clone(), diff --git a/package/src/lib.rs b/package/src/lib.rs index c0469bf868..3b11d6b215 100644 --- a/package/src/lib.rs +++ b/package/src/lib.rs @@ -10,16 +10,13 @@ pub mod util; pub use manifest::ManifestFile; use nickel_lang_core::{cache::normalize_abs_path, package::ObjectId}; use pubgrub::version::SemanticVersion; -use semver::Version; use serde::{Deserialize, Serialize}; use util::cache_dir; -// TODO: allow targeting branches or revisions, and allow supplying a relative path #[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize)] pub struct GitDependency { #[serde(with = "serde_url")] url: gix::Url, - #[serde(with = "git_target")] spec: nickel_lang_git::Target, } @@ -87,7 +84,7 @@ impl Dependency { version: dep_version, }, Precise::Index { id, version }, - ) => id == dep_id && dep_version.matches(&version), + ) => id == dep_id && dep_version.matches(version), _ => false, } } @@ -110,19 +107,6 @@ mod serde_url { } } -mod git_target { - use nickel_lang_git::Target; - use serde::{de::Error, Deserialize, Serialize as _}; - - pub fn serialize(url: &Target, ser: S) -> Result { - todo!() - } - - pub fn deserialize<'de, D: serde::Deserializer<'de>>(de: D) -> Result { - todo!() - } -} - #[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)] pub struct IndexPrecise { id: index::Id, @@ -198,7 +182,7 @@ impl Precise { pub fn version(&self) -> Option { match self { - Precise::Index { version, .. } => Some(version.clone()), + Precise::Index { version, .. } => Some(*version), _ => None, } } diff --git a/package/src/lock.rs b/package/src/lock.rs index ad435263e6..d65fcd5ac1 100644 --- a/package/src/lock.rs +++ b/package/src/lock.rs @@ -61,7 +61,6 @@ pub struct LockFile { } impl LockFile { - // TODO: move the implementation here pub fn new(manifest: &ManifestFile, resolution: &Resolution) -> Result { // We don't put all packages in the lock file: we ignore dependencies (and therefore also // transitive dependencies) of path deps. In order to figure out what to include, we diff --git a/package/src/manifest.rs b/package/src/manifest.rs index 248241e2aa..e0304e1933 100644 --- a/package/src/manifest.rs +++ b/package/src/manifest.rs @@ -195,7 +195,7 @@ impl Realization { return Ok(()); } (Dependency::Git(git), _) => { - let id = self.realize_one(&git)?; + let id = self.realize_one(git)?; UnversionedPrecise::Git { id, url: git.url.clone(), @@ -206,12 +206,18 @@ impl Realization { (Dependency::Path { path }, Some(relative_to)) => { let p = normalize_rel_path(&relative_to.local_path().join(path)); match relative_to { - Precise::Git { id, url: repo, .. } => { + Precise::Git { + id, + url: repo, + path, + } => { let repo_path = repo_root(id); let p = p .strip_prefix(&repo_path) .map_err(|_| Error::RestrictedPath { - package: "TODO".into(), + package_url: Box::new(repo.clone()), + package_commit: *id, + package_path: path.clone(), attempted: p.clone(), restriction: repo_path.to_owned(), })?; diff --git a/package/src/resolve.rs b/package/src/resolve.rs index 529b4d24ac..1478123311 100644 --- a/package/src/resolve.rs +++ b/package/src/resolve.rs @@ -10,28 +10,21 @@ //! with version `2.0`. Since we present them to pubgrub //! as different packages, they can both appear in the final resolution. -use std::{ - borrow::Borrow, - collections::HashMap, - path::{Path, PathBuf}, -}; +use std::{borrow::Borrow, collections::HashMap, path::PathBuf}; use nickel_lang_core::{cache::normalize_path, identifier::Ident, package::PackageMap}; use pubgrub::{ report::{DefaultStringReporter, Reporter as _}, solver::DependencyProvider, - version::{SemanticVersion, Version}, + version::SemanticVersion, }; -use semver::Comparator; use crate::{ error::{Error, IoResultExt as _}, index::{Id, IndexDependency, PackageIndex}, lock::LockFile, manifest::Realization, - util::semver_to_pg, - Dependency, GitDependency, IndexPrecise, ManifestFile, ObjectId, Precise, UnversionedPackage, - VersionReq, + Dependency, IndexPrecise, ManifestFile, ObjectId, Precise, VersionReq, }; type VersionRange = pubgrub::range::Range; @@ -104,18 +97,6 @@ impl PackageRegistry { } } -fn bucket_versions( - vs: impl Iterator, -) -> impl Iterator { - let mut vs: Vec<_> = vs - .map(BucketVersion::from) - .map(SemanticVersion::from) - .collect(); - vs.sort(); - vs.dedup(); - vs.into_iter() -} - /// A bucket version represents a collection of compatible semver versions. #[derive(Debug, Clone, Copy, Eq, PartialEq, Hash)] pub enum BucketVersion { @@ -298,32 +279,25 @@ impl DependencyProvider for PackageRegistry { let deps = self .unversioned_deps(p) .into_iter() - .map(|dep| { - match dep { - Dependency::Git(_) | Dependency::Path { .. } => { - let dep_precise = self.realized_unversioned.dependency - [&(precise.clone(), dep.clone())] - .clone(); - ( - Package::Unversioned(dep_precise.into()), - VersionRange::any(), - ) - } - // We're making a proxy for every dependency on a repo package, but we could skip - // the proxy if the dependency range stays within a single semver range. - Dependency::Index { id, version } => { - let pkg = Package::Bucket(Bucket { - id, - version: version.clone().into(), - }); - - let range = match version { - VersionReq::Compatible(v) => VersionRange::higher_than(v), - VersionReq::Exact(v) => VersionRange::exact(v), - }; - - (pkg, range) - } + .map(|dep| match dep { + Dependency::Git(_) | Dependency::Path { .. } => { + let dep_precise = self.realized_unversioned.dependency + [&(precise.clone(), dep.clone())] + .clone(); + (Package::Unversioned(dep_precise), VersionRange::any()) + } + Dependency::Index { id, version } => { + let pkg = Package::Bucket(Bucket { + id, + version: version.clone().into(), + }); + + let range = match version { + VersionReq::Compatible(v) => VersionRange::higher_than(v), + VersionReq::Exact(v) => VersionRange::exact(v), + }; + + (pkg, range) } }) .collect(); @@ -358,13 +332,6 @@ impl DependencyProvider for PackageRegistry { } } -fn version_req_to_range(req: &VersionReq) -> pubgrub::range::Range { - match req { - VersionReq::Compatible(v) => VersionRange::higher_than(v.clone()), - VersionReq::Exact(v) => VersionRange::exact(v.clone()), - } -} - pub struct Resolution { pub realization: Realization, pub package_map: HashMap>, @@ -375,12 +342,12 @@ pub fn resolve(manifest: &ManifestFile) -> Result { resolve_with_lock(manifest, &LockFile::default()) } -fn previously_locked(top_level: &Package, lock: &LockFile) -> HashMap { +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(), + version: *version, }), _ => None, } @@ -414,11 +381,10 @@ fn previously_locked(top_level: &Package, lock: &LockFile) -> HashMap Result { let mut realization = Realization::default(); - // FIXME: figure out how to represent the top-level package, recalling that `realization` assumes - // that every time we need to look up a dependency we need a parent package. + // TODO: this assumes that the top-level package has a path. Is there a a use-case for resolving + // packages without a top-level path? let root_path = manifest.parent_dir.as_deref(); for dep in manifest.dependencies.values() { - // FIXME: unwrap realization.realize_all(root_path.unwrap(), dep, None)?; } let top_level = UnversionedPrecise::Path { @@ -449,7 +415,7 @@ pub fn resolve_with_lock(manifest: &ManifestFile, lock: &LockFile) -> Result>::new(); for (pkg, vers) in resolution.iter() { if let Package::Bucket(Bucket { id, .. }) = pkg { - selected.entry(id.clone()).or_default().push(vers.clone()); + selected.entry(id.clone()).or_default().push(*vers); } } Ok(Resolution { @@ -478,12 +444,11 @@ impl Resolution { }, Dependency::Index { id, version } => Precise::Index { id: id.clone(), - version: self.package_map[id] + version: *self.package_map[id] .iter() .filter(|v| version.matches(v)) .max() - .unwrap() - .clone(), + .unwrap(), }, } } @@ -500,7 +465,7 @@ impl Resolution { .collect() } Precise::Index { id, version } => { - let pkg = self.index.package(id, version.clone()).unwrap(); + let pkg = self.index.package(id, *version).unwrap(); pkg.deps .into_iter() .map(move |(dep_name, dep)| { @@ -529,7 +494,7 @@ impl Resolution { let index_precises = self.package_map.iter().flat_map(|(id, vs)| { vs.iter().map(|v| Precise::Index { id: id.clone(), - version: v.clone(), + version: *v, }) }); ret.extend(index_precises);