Resolve with locked versions (#779)

commit-id:2fd1f61f

---

**Stack**:
- #781
- #800
- #780
- #779 ⬅


⚠️ *Part of a stack created by [spr](https://github.com/ejoffe/spr). Do
not merge manually using the UI - doing so may have unexpected results.*

scarb/src/core/lockfile.rs

@@ -5,7 +5,7 @@ use serde_repr::{Deserialize_repr, Serialize_repr};
 use std::collections::BTreeSet;
 use toml_edit::Document;
 
-use crate::core::{PackageId, PackageName, Resolve, SourceId};
+use crate::core::{ManifestDependency, PackageId, PackageName, Resolve, SourceId};
 
 const HEADER: &str = "# Code generated by scarb DO NOT EDIT.";
 
@@ -62,6 +62,23 @@ impl Lockfile {
         Self::new(packages)
     }
 
+    pub fn packages(&self) -> impl Iterator<Item = &PackageLock> {
+        self.packages.iter()
+    }
+
+    pub fn packages_matching(&self, dependency: ManifestDependency) -> Option<Result<PackageId>> {
+        self.packages()
+            .filter(|p| dependency.matches_name_and_version(&p.name, &p.version))
+            .find(|p| {
+                p.source
+                    .map(|sid| sid.can_lock_source_id(dependency.source_id))
+                    // No locking occurs on path sources.
+                    .unwrap_or(false)
+            })
+            .cloned()
+            .map(|p| p.try_into())
+    }
+
     fn body(&self) -> Result<Document> {
         let doc = toml_edit::ser::to_string_pretty(self)?;
         let mut doc = doc.parse::<Document>()?;

scarb/src/core/manifest/dependency.rs

@@ -1,3 +1,4 @@
+use semver::Version;
 use std::fmt;
 use std::ops::Deref;
 use std::sync::Arc;
@@ -54,7 +55,11 @@ impl ManifestDependency {
     }
 
     pub fn matches_package_id(&self, package_id: PackageId) -> bool {
-        package_id.name == self.name && self.version_req.matches(&package_id.version)
+        self.matches_name_and_version(&package_id.name, &package_id.version)
+    }
+
+    pub fn matches_name_and_version(&self, name: &PackageName, version: &Version) -> bool {
+        *name == self.name && self.version_req.matches(version)
     }
 }
 

scarb/src/core/manifest/version_req.rs

@@ -48,6 +48,16 @@ impl From<VersionReq> for DependencyVersionReq {
     }
 }
 
+impl From<DependencyVersionReq> for VersionReq {
+    fn from(req: DependencyVersionReq) -> Self {
+        match req {
+            DependencyVersionReq::Any => VersionReq::STAR,
+            DependencyVersionReq::Req(req) => req,
+            DependencyVersionReq::Locked { req, .. } => req,
+        }
+    }
+}
+
 impl fmt::Display for DependencyVersionReq {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {

scarb/src/core/resolver.rs

@@ -5,7 +5,6 @@ use smallvec::SmallVec;
 
 use crate::core::{PackageId, TargetKind};
 
-// TODO(#126): Produce lockfile out of this.
 /// Represents a fully-resolved package dependency graph.
 ///
 /// Each node in the graph is a package and edges represent dependencies between packages.

scarb/src/core/source/id.rs

@@ -52,6 +52,45 @@ impl SourceKind {
             _ => None,
         }
     }
+
+    pub fn can_lock_source_kind(&self, other: &Self) -> bool {
+        if self == other {
+            return true;
+        }
+
+        match self {
+            // We can reject specs without precise,
+            // as they would need to be identical anyway.
+            SourceKind::Git(spec) if spec.precise.is_none() => false,
+            SourceKind::Git(spec) => {
+                let other_precise = other
+                    .as_git_source_spec()
+                    .and_then(|other_spec| other_spec.precise.clone());
+
+                // If the other source kind has a precise revision locked,
+                // and the other source kind does not equal self,
+                // then self cannot lock the other source kind.
+                if other_precise.is_some() {
+                    return false;
+                }
+
+                spec.precise
+                    .clone()
+                    .and_then(|precise| {
+                        // Compare other attributes apart from precise revision.
+                        // Note that `other` with different source kind defaults to false on unwrap.
+                        other
+                            .as_git_source_spec()
+                            // Overwrite precise in other.
+                            .map(|p| p.clone().with_precise(precise))
+                            .map(|s| s == *spec)
+                    })
+                    .unwrap_or(false)
+            }
+            // Reject rest as handled by equality check.
+            _ => false,
+        }
+    }
 }
 
 const PATH_SOURCE_PROTOCOL: &str = "path";
@@ -119,6 +158,29 @@ impl SourceId {
         }))
     }
 
+    pub fn can_lock_source_id(self, other: Self) -> bool {
+        if self == other {
+            return true;
+        }
+
+        let can_lock = self.kind.can_lock_source_kind(&other.kind);
+
+        // Check if other attributes apart from kind are equal.
+        can_lock && self.equals_ignoring_kind(other)
+    }
+
+    fn equals_ignoring_kind(self, other: Self) -> bool {
+        let first = SourceIdInner {
+            kind: SourceKind::Std,
+            ..(self.0).clone()
+        };
+        let second = SourceIdInner {
+            kind: SourceKind::Std,
+            ..(other.0).clone()
+        };
+        first == second
+    }
+
     fn intern(inner: SourceIdInner) -> Self {
         static CACHE: StaticHashCache<SourceIdInner> = StaticHashCache::new();
         Self(CACHE.intern(inner))

scarb/src/resolver/mod.rs

@@ -8,7 +8,9 @@ use scarb_ui::Ui;
 use crate::core::lockfile::Lockfile;
 use crate::core::registry::Registry;
 use crate::core::resolver::{DependencyEdge, Resolve};
-use crate::core::{DepKind, ManifestDependency, PackageId, Summary, TargetKind};
+use crate::core::{
+    DepKind, DependencyVersionReq, ManifestDependency, PackageId, Summary, TargetKind,
+};
 
 /// Builds the list of all packages required to build the first argument.
 ///
@@ -27,7 +29,7 @@ use crate::core::{DepKind, ManifestDependency, PackageId, Summary, TargetKind};
 pub async fn resolve(
     summaries: &[Summary],
     registry: &dyn Registry,
-    _lockfile: Lockfile,
+    lockfile: Lockfile,
     ui: &Ui,
 ) -> Result<Resolve> {
     // TODO(#2): This is very bad, use PubGrub here.
@@ -54,6 +56,13 @@ pub async fn resolve(
             for dep in summaries[&package_id].clone().full_dependencies() {
                 let dep = rewrite_dependency_source_id(registry, &package_id, dep).await?;
 
+                let locked_package_id = lockfile.packages_matching(dep.clone());
+                let dep = if let Some(locked_package_id) = locked_package_id {
+                    rewrite_locked_dependency(dep.clone(), locked_package_id?)
+                } else {
+                    dep
+                };
+
                 let results = registry.query(&dep).await?;
 
                 let Some(dep_summary) = results.first() else {
@@ -142,6 +151,21 @@ pub async fn resolve(
     Ok(Resolve { graph })
 }
 
+fn rewrite_locked_dependency(
+    dependency: ManifestDependency,
+    locked_package_id: PackageId,
+) -> ManifestDependency {
+    ManifestDependency::builder()
+        .kind(dependency.kind.clone())
+        .name(dependency.name.clone())
+        .source_id(locked_package_id.source_id)
+        .version_req(DependencyVersionReq::Locked {
+            exact: locked_package_id.version.clone(),
+            req: dependency.version_req.clone().into(),
+        })
+        .build()
+}
+
 async fn rewrite_dependency_source_id(
     registry: &dyn Registry,
     package_id: &PackageId,

scarb/src/sources/git/mod.rs

@@ -62,7 +62,6 @@ impl<'c> GitSource<'c> {
             config,
             remote: GitRemote::new(canonical_url),
             requested_reference,
-            // TODO(#126): Pull this somehow from the lockfile.
             locked_rev,
             inner: OnceCell::new(),
         })

scarb/tests/git_source.rs

@@ -210,6 +210,9 @@ fn stale_cached_version() {
             .build(&t)
     });
 
+    // Use the same cache dir to prevent downloading git dep second time for the locked rev.
+    let cache_dir = TempDir::new().unwrap();
+
     let t = TempDir::new().unwrap();
     ProjectBuilder::start()
         .name("hello")
@@ -220,6 +223,7 @@ fn stale_cached_version() {
 
     Scarb::quick_snapbox()
         .arg("build")
+        .env("SCARB_CACHE", cache_dir.path())
         .current_dir(&t)
         .assert()
         .success()
@@ -232,27 +236,32 @@ fn stale_cached_version() {
     t.child("target/dev/hello.sierra.json")
         .assert(predicates::str::contains("11111111111101"));
 
-    // TODO(#126): Lockfile should prevent updating.
-    //   When lockfile will be implemented, uncomment this and implement missing parts.
-    // Scarb::quick_snapbox()
-    //     .arg("build")
-    //     .current_dir(&t)
-    //     .assert()
-    //     .success()
-    //     .stdout_matches(indoc! {r#"
-    //     [..] Compiling hello v1.0.0 ([..])
-    //     [..]  Finished release target(s) in [..]
-    //     "#});
-    //
-    // t.child("target/release/hello.sierra.json")
-    //     .assert(predicates::str::contains("11111111111101"));
-    //
-    // remove lockfile here
+    Scarb::quick_snapbox()
+        .arg("build")
+        .env("SCARB_CACHE", cache_dir.path())
+        .current_dir(&t)
+        .assert()
+        .success()
+        .stdout_matches(indoc! {r#"
+        [..] Compiling hello v1.0.0 ([..])
+        [..]  Finished release target(s) in [..]
+        "#});
+
+    t.child("target/dev/hello.sierra.json")
+        .assert(predicates::str::contains("11111111111101"));
+
+    // Remove lockfile.
+    let lockfile = t.child("Scarb.lock");
+    if lockfile.exists() {
+        fs::remove_file(&lockfile)
+            .unwrap_or_else(|_| panic!("failed to remove {}", lockfile.to_str().unwrap()));
+    }
 
     dep.change_file("src/lib.cairo", "fn hello() -> felt252 { 11111111111102 }");
 
     Scarb::quick_snapbox()
         .arg("build")
+        .env("SCARB_CACHE", cache_dir.path())
         .current_dir(&t)
         .assert()
         .success()

website/docs/guides/dependencies.md

@@ -31,11 +31,6 @@ Actually this is how the OpenZeppelin Contracts for Cairo library is released, s
 openzeppelin = { git = "https://github.com/OpenZeppelin/cairo-contracts.git", tag = "v0.7.0-rc.0" }
 ```
 
-::: info
-In the future commit pinning will not be needed because Scarb will maintain a lockfile.
-We track this feature in this issue: [#126](https://github.com/software-mansion/scarb/issues/126).
-:::
-
 Note, that if you want to add more dependencies, you do not have to add `[dependencies]` for each package separately. For example:
 
 ```toml