Improve lock file format

cli/src/input.rs

@@ -39,7 +39,7 @@ pub struct InputOptions<Customize: clap::Args> {
     /// (Future versions may auto-detect or auto-create a lock file, in which
     /// case this argument will become optional.)
     #[arg(long, global = true)]
-    pub lock_file: Option<PathBuf>,
+    pub manifest_path: Option<PathBuf>,
 
     #[arg(long, global = true)]
     /// Filesystem location for caching fetched packages.
@@ -88,7 +88,7 @@ impl<C: clap::Args + Customize> Prepare for InputOptions<C> {
         }
 
         #[cfg(feature = "package-experimental")]
-        if let Some(lock_file_path) = self.lock_file.as_ref() {
+        if let Some(manifest_path) = self.manifest_path.as_ref() {
             let lock_file = LockFile::from_path(lock_file_path)?;
             let lock_dir = lock_file_path
                 .parent()

git/src/lib.rs

@@ -81,7 +81,18 @@ impl Spec {
 
 /// The different kinds of git "thing" that we can target.
 #[serde_with::serde_as]
-#[derive(Clone, Debug, PartialEq, Eq, Hash, Default, serde::Serialize, serde::Deserialize)]
+#[derive(
+    Clone,
+    Debug,
+    PartialEq,
+    Eq,
+    Hash,
+    Default,
+    serde::Serialize,
+    serde::Deserialize,
+    PartialOrd,
+    Ord,
+)]
 pub enum Target {
     /// By default, we target the remote HEAD.
     #[default]

package/src/lib.rs

@@ -1,5 +1,6 @@
 use std::path::{Path, PathBuf};
 
+use lock::LockFileDep;
 use nickel_lang_core::cache::normalize_abs_path;
 
 use config::Config;
@@ -17,7 +18,7 @@ pub use gix::ObjectId;
 pub use manifest::ManifestFile;
 
 /// A dependency that comes from a git repository.
-#[derive(Clone, Debug, PartialEq, Eq, Hash, Deserialize)]
+#[derive(Clone, Debug, PartialEq, Eq, Hash, Serialize, Deserialize, PartialOrd, Ord)]
 pub struct GitDependency {
     /// The url of the git repo, in any format understood by `gix`.
     /// For example, it can be a path, an https url, or an ssh url.
@@ -40,11 +41,16 @@ pub enum Dependency {
 }
 
 impl Dependency {
-    pub fn matches(&self, precise: &Precise) -> bool {
-        match (self, precise) {
-            (Dependency::Git(git), Precise::Git { url: repo, .. }) => &git.url == repo,
-            (Dependency::Path { path }, Precise::Path { path: locked_path }) => path == locked_path,
-            _ => false,
+    pub fn matches(&self, entry: &LockFileDep) -> bool {
+        match self {
+            Dependency::Git(git) => match &entry.spec {
+                Some(locked_git) => git == locked_git,
+                None => false,
+            },
+            Dependency::Path { path } => match &entry.precise {
+                Precise::Path { path: locked_path } => path == locked_path,
+                _ => false,
+            },
         }
     }
 }

package/src/lock.rs

@@ -9,10 +9,9 @@ use nickel_lang_core::{cache::normalize_path, identifier::Ident, package::Packag
 use serde::{Deserialize, Serialize};
 
 use crate::{
-    config::Config,
     error::{Error, IoResultExt},
     realization::Realization,
-    ManifestFile, Precise,
+    Dependency, GitDependency, ManifestFile, Precise,
 };
 
 mod package_list {
@@ -80,7 +79,7 @@ pub struct LockFile {
     /// The dependencies of the current (top-level) package.
     ///
     /// These should be sorted so that the serialization doesn't change all the time.
-    pub dependencies: BTreeMap<String, Precise>,
+    pub dependencies: BTreeMap<String, LockFileDep>,
     /// All packages that we know about, and the dependencies of each one.
     ///
     /// Note that the package list is not guaranteed to be closed: path dependencies
@@ -101,12 +100,11 @@ impl LockFile {
             pkg: &Precise,
             acc: &mut HashMap<Precise, LockFileEntry>,
         ) -> Result<(), Error> {
-            // Let's try out what happens if we include path deps and their
-            // dependencies in the lock file. This makes the lock file
-            // potentially non-portable to different systems, but on the other
-            // hand it allows the package map to be read straight from the lock
-            // file. This is probably the way to go if we require manual lock
-            // file refreshing.
+            // We exclude path dependencies (and their recursive dependencies)
+            // from the lock file. This ensures that the lock file is portable
+            // to different systems, but on the other hand it means that the
+            // package map cannot be derived straight from the lock file: we
+            // always need to read the manifest and look for path dependencies.
             //
             // To clarify the trade-off a little bit, we have a choice between
             // user-refreshed lock files or automatically-refreshed lock files.
@@ -130,30 +128,44 @@ impl LockFile {
             // if they're in the lock file. So we may as well make the lock
             // files portable by leaving out path dependencies.
 
-            // The following commented-out code is what we want if we decide *not*
+            let entry = LockFileEntry {
+                dependencies: if pkg.is_path() {
+                    // Skip dependencies of path deps
+                    Default::default()
+                } else {
+                    realization
+                        .dependencies(pkg)
+                        .into_iter()
+                        .map(|(id, (dep, precise))| {
+                            let spec = match dep {
+                                Dependency::Git(g) => Some(g.clone()),
+                                Dependency::Path { .. } => None,
+                            };
+                            let entry = LockFileDep {
+                                precise: precise.clone(),
+                                spec,
+                            };
+                            (id.label().to_owned(), entry)
+                        })
+                        .collect()
+                },
+            };
+
+            // The following commented-out code is what we want if we decide
             // to include path deps in the lock file.
             //
             // let entry = LockFileEntry {
-            //     dependencies: if pkg.is_path() {
-            //         // Skip dependencies of path deps
-            //         Default::default()
-            //     } else {
-            //         res.dependencies(pkg)
-            //     },
+            //     dependencies: realization
+            //         .dependencies(pkg)
+            //         .into_iter()
+            //         .map(|(id, entry)| (id.label().to_owned(), entry))
+            //         .collect(),
             // };
 
-            let entry = LockFileEntry {
-                dependencies: realization
-                    .dependencies(pkg)
-                    .into_iter()
-                    .map(|(id, entry)| (id.label().to_owned(), entry))
-                    .collect(),
-            };
-
             // Only recurse if this is the first time we've encountered this precise package.
-            if acc.insert(pkg.clone(), entry).is_none() {
-                for (_, dep) in acc[pkg].clone().dependencies {
-                    collect_packages(realization, &dep, acc)?;
+            if acc.insert(pkg.clone(), entry).is_none() && !pkg.is_path() {
+                for (_id, (_dep, precise)) in realization.dependencies(pkg) {
+                    collect_packages(realization, &precise, acc)?;
                 }
             }
             Ok(())
@@ -168,7 +180,17 @@ impl LockFile {
             dependencies: manifest
                 .dependencies
                 .iter()
-                .map(|(name, dep)| (name.label().to_owned(), realization.precise(dep)))
+                .map(|(name, dep)| {
+                    let spec = match dep {
+                        Dependency::Git(g) => Some(g.clone()),
+                        Dependency::Path { .. } => None,
+                    };
+                    let entry = LockFileDep {
+                        precise: realization.precise(dep),
+                        spec,
+                    };
+                    (name.label().to_owned(), entry)
+                })
                 .collect(),
 
             packages: acc,
@@ -187,13 +209,15 @@ impl LockFile {
 
     /// Build a package map from a lock file.
     ///
-    /// This only works if the lock file contains path dependencies and their
-    /// recursive dependencies. See [`LockFile`].
-    ///
     /// `manifest_dir` is the directory containing the manifest file. Relative
     /// path dependencies in the lock file will be interpreted relative to the
     /// manifest directory and turned into absolute paths.
-    pub fn package_map(&self, manifest_dir: &Path, config: &Config) -> Result<PackageMap, Error> {
+    pub fn package_map(
+        &self,
+        manifest_dir: &Path,
+        realization: &Realization,
+    ) -> Result<PackageMap, Error> {
+        let config = &realization.config;
         let manifest_dir = normalize_path(manifest_dir).without_path()?;
 
         let path = |pkg: &Precise| pkg.clone().with_abs_path(&manifest_dir).local_path(config);
@@ -202,7 +226,7 @@ impl LockFile {
             top_level: self
                 .dependencies
                 .iter()
-                .map(|(id, pkg)| (Ident::new(id), path(pkg)))
+                .map(|(id, entry)| (Ident::new(id), path(&entry.precise)))
                 .collect(),
             packages: self
                 .packages
@@ -211,7 +235,7 @@ impl LockFile {
                     entry
                         .dependencies
                         .iter()
-                        .map(|(id, dep)| ((path(pkg), Ident::new(id)), path(dep)))
+                        .map(|(id, dep)| ((path(pkg), Ident::new(id)), path(&dep.precise)))
                 })
                 .collect(),
         })
@@ -228,8 +252,20 @@ impl LockFile {
     }
 }
 
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
+pub struct LockFileDep {
+    pub precise: Precise,
+
+    /// For git packages, we store their original git spec in the lock-file, so
+    /// that if someone changes the spec in the manifest we can tell that we
+    /// need to re-fetch the repo.
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default)]
+    pub spec: Option<GitDependency>,
+}
+
 /// The dependencies of a single package.
-#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, PartialOrd, Eq, Ord)]
+#[derive(Clone, Debug, Serialize, Deserialize, PartialEq, Eq, PartialOrd, Ord)]
 pub struct LockFileEntry {
-    dependencies: BTreeMap<String, Precise>,
+    dependencies: BTreeMap<String, LockFileDep>,
 }

package/src/manifest.rs

@@ -151,7 +151,7 @@ impl ManifestFile {
             lock_file
                 .dependencies
                 .get(name.label())
-                .map_or(false, |id| src.matches(id))
+                .map_or(false, |entry| src.matches(&entry))
         })
     }
 

package/src/realization.rs

@@ -34,7 +34,7 @@ pub struct Realization {
     ///
     /// The realization process can involve downloading and caching git
     /// packages; the configuration tells us where to put them.
-    config: Config,
+    pub(crate) config: Config,
     /// A map from the possibly-underspecified dependencies given in the
     /// manifest to exact git object ids.
     git: HashMap<GitDependency, ObjectId>,
@@ -51,6 +51,10 @@ impl Realization {
     /// Runs realization, downloading all necessary git dependencies and finding their exact versions.
     ///
     /// We resolve path dependencies relative to `root_path`.
+    ///
+    /// FIXME: we also need a version of this that takes in a lock file and doesn't re-fetch all
+    /// the git repos. But how do we know which ones need re-fetching? It looks like we need to store
+    /// the original git spec in the lockfile.
     pub fn new<'a>(
         config: Config,
         root_path: &Path,
@@ -191,7 +195,7 @@ impl Realization {
     ///
     /// Panics if the package was not part of the dependency tree that this resolution
     /// was generated for.
-    pub fn dependencies(&self, pkg: &Precise) -> HashMap<Ident, Precise> {
+    pub fn dependencies(&self, pkg: &Precise) -> HashMap<Ident, (Dependency, Precise)> {
         let manifest = &self.manifests[pkg];
         manifest
             .dependencies
@@ -200,7 +204,7 @@ impl Realization {
                 // unwrap: we ensure at construction time that our dependency graph is closed
                 // Note that this will change when we introduce index packages.
                 let precise_dep = self.dependency.get(&(pkg.clone(), dep.clone())).unwrap();
-                (*dep_name, precise_dep.clone())
+                (*dep_name, (dep.clone(), precise_dep.clone()))
             })
             .collect()
     }
@@ -244,7 +248,7 @@ impl Realization {
         for p in &all {
             let p_path = p.clone().with_abs_path(&manifest_dir).local_path(config);
             let root_path = &manifest_dir;
-            for (dep_id, dep_precise) in self.dependencies(p) {
+            for (dep_id, (_, dep_precise)) in self.dependencies(p) {
                 packages.insert(
                     (p_path.clone(), dep_id),
                     dep_precise.with_abs_path(root_path).local_path(config),

package/tests/integration/inputs/path/double-path-dep/package.ncl

@@ -0,0 +1,10 @@
+{
+  name = "double-path-dep",
+  description = "A package with a single path dependency, which has a further path dependency",
+  version = "0.1.0",
+  authors = ["Joe"],
+  minimal_nickel_version = "1.9.0",
+  dependencies = {
+    dep = 'Path "../single-path-dep"
+  },
+} | std.package.Manifest

package/tests/integration/snapshots/integration__package__tests__integration__inputs__path__double-path-dep__package.ncl.snap

@@ -0,0 +1,25 @@
+---
+source: package/tests/integration/main.rs
+expression: lock_contents
+---
+{
+  "dependencies": {
+    "dep": {
+      "precise": {
+        "Path": {
+          "path": "../single-path-dep"
+        }
+      }
+    }
+  },
+  "packages": [
+    {
+      "source": {
+        "Path": {
+          "path": "../single-path-dep"
+        }
+      },
+      "dependencies": {}
+    }
+  ]
+}

package/tests/integration/snapshots/integration__package__tests__integration__inputs__path__git-branch-and-tag-dep__package.ncl.snap

@@ -5,16 +5,34 @@ expression: lock_contents
 {
   "dependencies": {
     "branch": {
-      "Git": {
+      "precise": {
+        "Git": {
+          "url": "https://example.com/branch-leaf",
+          "id": <GENERATED>,
+          "path": ""
+        }
+      },
+      "spec": {
         "url": "https://example.com/branch-leaf",
-        "id": <GENERATED>,
+        "ref": {
+          "Branch": "cành"
+        },
         "path": ""
       }
     },
     "tag": {
-      "Git": {
+      "precise": {
+        "Git": {
+          "url": "https://example.com/tag-leaf",
+          "id": <GENERATED>,
+          "path": ""
+        }
+      },
+      "spec": {
         "url": "https://example.com/tag-leaf",
-        "id": <GENERATED>,
+        "ref": {
+          "Tag": "mytag"
+        },
         "path": ""
       }
     }