🐛 Write manifest even if only requirements changed

Author: fruno-bulax

compiler-cli/src/cli.rs

@@ -1,7 +1,7 @@
 use gleam_core::{
     build::Telemetry,
     error::{Error, StandardIoAction},
-    manifest::{Changed, ChangedGit, Resolved},
+    manifest::{Changed, ChangedGit, PackageChanges},
 };
 use hexpm::version::Version;
 use itertools::Itertools as _;
@@ -57,8 +57,8 @@ impl Telemetry for Reporter {
         print_waiting_for_build_directory_lock()
     }
 
-    fn resolved_package_versions(&self, resolved: &Resolved) {
-        print_resolved(resolved)
+    fn resolved_package_versions(&self, changes: &PackageChanges) {
+        print_package_changes(changes)
     }
 }
 
@@ -154,22 +154,22 @@ pub(crate) fn print_running(text: &str) {
     print_colourful_prefix("Running", text)
 }
 
-pub(crate) fn print_resolved(resolved: &Resolved) {
-    for (name, version) in resolved.added.iter().sorted() {
+pub(crate) fn print_package_changes(changes: &PackageChanges) {
+    for (name, version) in changes.added.iter().sorted() {
         print_added(&format!("{name} v{version}"));
     }
-    for Changed { name, old, new } in resolved.changed.iter().sorted_by_key(|p| &p.name) {
+    for Changed { name, old, new } in changes.changed.iter().sorted_by_key(|p| &p.name) {
         print_changed(&format!("{name} v{old} -> v{new}"));
     }
     for ChangedGit {
         name,
         old_hash,
         new_hash,
-    } in resolved.changed_git.iter().sorted_by_key(|p| &p.name)
+    } in changes.changed_git.iter().sorted_by_key(|p| &p.name)
     {
         print_changed(&format!("{name} {old_hash} -> {new_hash}"));
     }
-    for name in resolved.removed.iter().sorted() {
+    for name in changes.removed.iter().sorted() {
         print_removed(name);
     }
 }

compiler-cli/src/dependencies.rs

@@ -20,7 +20,7 @@ use gleam_core::{
     error::{FileIoAction, FileKind, ShellCommandFailureReason, StandardIoAction},
     hex::{self, HEXPM_PUBLIC_KEY},
     io::{HttpClient as _, TarUnpacker, WrappedReader},
-    manifest::{Base16Checksum, Manifest, ManifestPackage, ManifestPackageSource, Resolved},
+    manifest::{Base16Checksum, Manifest, ManifestPackage, ManifestPackageSource, PackageChanges},
     paths::ProjectPaths,
     requirement::Requirement,
 };
@@ -291,10 +291,10 @@ pub fn cleanup<Telem: Telemetry>(paths: &ProjectPaths, telemetry: Telem) -> Resu
     write_manifest_to_disc(paths, &manifest)?;
     LocalPackages::from_manifest(&manifest).write_to_disc(paths)?;
 
-    let resolved = Resolved::with_updates(&old_manifest, manifest);
-    telemetry.resolved_package_versions(&resolved);
+    let changes = PackageChanges::between_manifests(&old_manifest, &manifest);
+    telemetry.resolved_package_versions(&changes);
 
-    Ok(resolved.manifest)
+    Ok(manifest)
 }
 
 /// Remove requirements and unneeded packages from manifest that are no longer present in config.

compiler-cli/src/dependencies/dependency_manager.rs

@@ -5,7 +5,7 @@ use gleam_core::{
     build::{Mode, Telemetry},
     config::PackageConfig,
     dependency,
-    manifest::{Manifest, ManifestPackageSource, Resolved},
+    manifest::{Manifest, ManifestPackageSource, PackageChanges, Resolved},
     paths::ProjectPaths,
     requirement::Requirement,
 };
@@ -87,24 +87,24 @@ where
         let existing_manifest = read_manifest_from_disc(paths)?;
 
         // If we have been asked not to use the manifest then
-        let manifest_for_resolver = match self.use_manifest {
-            UseManifest::No => None,
+        let (requirements_changed, manifest_for_resolver) = match self.use_manifest {
+            UseManifest::No => (true, None),
             UseManifest::Yes => {
+                let same_requirements = is_same_requirements(
+                    &existing_manifest.requirements,
+                    &config.all_direct_dependencies()?,
+                    paths.root(),
+                )?;
+
                 // If the manifest is to be used and the requirements have not changed then there's
                 // no point in performing resolution, it'll always result in the same versions
                 // already specified in the manifest.
-                if packages_to_update.is_empty()
-                    && is_same_requirements(
-                        &existing_manifest.requirements,
-                        &config.all_direct_dependencies()?,
-                        paths.root(),
-                    )?
-                {
+                if packages_to_update.is_empty() && same_requirements {
                     return Ok(Resolved::no_change(existing_manifest));
                 }
 
                 // Otherwise, use the manifest to inform resolution.
-                Some(&existing_manifest)
+                (!same_requirements, Some(&existing_manifest))
             }
         };
 
@@ -115,7 +115,12 @@ where
             manifest_for_resolver,
             packages_to_update,
         )?;
-        Ok(Resolved::with_updates(&existing_manifest, new_manifest))
+        let resolved = Resolved {
+            package_changes: PackageChanges::between_manifests(&existing_manifest, &new_manifest),
+            manifest: new_manifest,
+            requirements_changed,
+        };
+        Ok(resolved)
     }
 
     pub fn resolve_and_download_versions(
@@ -177,7 +182,8 @@ where
         LocalPackages::from_manifest(&resolved.manifest).write_to_disc(paths)?;
 
         // Display the changes in versions to the user.
-        self.telemetry.resolved_package_versions(&resolved);
+        self.telemetry
+            .resolved_package_versions(&resolved.package_changes);
 
         // If requested to do so, check if there are major upgrades that could be performed with
         // more relaxed version requirements, and inform the user if so.

compiler-core/src/build/telemetry.rs

@@ -3,13 +3,16 @@ use std::{
     time::{Duration, Instant},
 };
 
-use crate::{Warning, manifest::Resolved};
+use crate::{
+    Warning,
+    manifest::{PackageChanges, Resolved},
+};
 
 pub trait Telemetry: Debug {
     fn waiting_for_build_directory_lock(&self);
     fn running(&self, name: &str);
     fn resolving_package_versions(&self);
-    fn resolved_package_versions(&self, resolved: &Resolved);
+    fn resolved_package_versions(&self, changes: &PackageChanges);
     fn downloading_package(&self, name: &str);
     fn packages_downloaded(&self, start: Instant, count: usize);
     fn compiled_package(&self, duration: Duration);
@@ -31,5 +34,5 @@ impl Telemetry for NullTelemetry {
     fn checked_package(&self, _duration: Duration) {}
     fn checking_package(&self, _name: &str) {}
     fn packages_downloaded(&self, _start: Instant, _count: usize) {}
-    fn resolved_package_versions(&self, _resolved: &Resolved) {}
+    fn resolved_package_versions(&self, _changes: &PackageChanges) {}
 }

compiler-core/src/manifest.rs

@@ -500,6 +500,12 @@ zzz = { version = "> 0.0.0" }
 #[derive(Debug)]
 pub struct Resolved {
     pub manifest: Manifest,
+    pub package_changes: PackageChanges,
+    pub requirements_changed: bool,
+}
+
+#[derive(Debug)]
+pub struct PackageChanges {
     pub added: Vec<(EcoString, Version)>,
     pub changed: Vec<Changed>,
     /// When updating git dependencies, it is possible to update to a newer commit
@@ -526,10 +532,7 @@ pub struct ChangedGit {
 
 impl Resolved {
     pub fn any_changes(&self) -> bool {
-        !self.added.is_empty()
-            || !self.changed.is_empty()
-            || !self.changed_git.is_empty()
-            || !self.removed.is_empty()
+        self.requirements_changed || self.package_changes.any_changes()
     }
 
     pub fn all_added(manifest: Manifest) -> Resolved {
@@ -540,26 +543,40 @@ impl Resolved {
             .collect();
         Self {
             manifest,
-            added,
-            changed: vec![],
-            changed_git: vec![],
-            removed: vec![],
+            requirements_changed: true,
+            package_changes: PackageChanges {
+                added,
+                changed: vec![],
+                changed_git: vec![],
+                removed: vec![],
+            },
         }
     }
 
     pub fn no_change(manifest: Manifest) -> Self {
         Self {
             manifest,
-            added: vec![],
-            changed: vec![],
-            changed_git: vec![],
-            removed: vec![],
+            requirements_changed: false,
+            package_changes: PackageChanges {
+                added: vec![],
+                changed: vec![],
+                changed_git: vec![],
+                removed: vec![],
+            },
         }
     }
+}
+
+impl PackageChanges {
+    pub fn any_changes(&self) -> bool {
+        !self.added.is_empty()
+            || !self.changed.is_empty()
+            || !self.changed_git.is_empty()
+            || !self.removed.is_empty()
+    }
 
-    /// Create a `Resolved`, comparing the old and new versions of the manifest to work out
-    /// if resolution resulted in any changes.
-    pub fn with_updates(old: &Manifest, new: Manifest) -> Self {
+    ///  Comparing the old and new versions of the manifest and determine the package changes
+    pub fn between_manifests(old: &Manifest, new: &Manifest) -> Self {
         let mut added = vec![];
         let mut changed = vec![];
         let mut changed_git = vec![];
@@ -613,7 +630,6 @@ impl Resolved {
         removed.extend(old.into_keys().cloned());
 
         Self {
-            manifest: new,
             added,
             changed,
             changed_git,
@@ -629,8 +645,8 @@ mod manifest_update_tests {
     use ecow::EcoString;
     use hexpm::version::Version;
 
-    use crate::manifest::{Base16Checksum, ManifestPackage, ManifestPackageSource};
-    use crate::manifest::{Changed, Manifest, Resolved};
+    use crate::manifest::{Base16Checksum, ManifestPackage, ManifestPackageSource, PackageChanges};
+    use crate::manifest::{Changed, Manifest};
 
     #[test]
     fn resolved_with_updated() {
@@ -669,21 +685,21 @@ mod manifest_update_tests {
             ],
         };
 
-        let mut resolved = Resolved::with_updates(&old, new);
-        resolved.added.sort();
-        resolved.changed.sort_by(|a, b| a.name.cmp(&b.name));
-        resolved.removed.sort();
+        let mut changes = PackageChanges::between_manifests(&old, &new);
+        changes.added.sort();
+        changes.changed.sort_by(|a, b| a.name.cmp(&b.name));
+        changes.removed.sort();
 
         assert_eq!(
-            resolved.added,
+            changes.added,
             vec![
                 ("new1".into(), Version::new(1, 0, 0)),
                 ("new2".into(), Version::new(2, 1, 0)),
             ]
         );
 
         assert_eq!(
-            resolved.changed,
+            changes.changed,
             vec![
                 Changed {
                     name: "changed1".into(),
@@ -699,7 +715,7 @@ mod manifest_update_tests {
         );
 
         assert_eq!(
-            resolved.removed,
+            changes.removed,
             vec![EcoString::from("removed1"), EcoString::from("removed2")]
         );
     }
@@ -731,9 +747,9 @@ mod manifest_update_tests {
             })],
         };
 
-        let resolved = Resolved::with_updates(&old, new);
-        assert!(resolved.changed.is_empty());
-        assert_eq!(resolved.removed, vec![name.clone()]);
-        assert_eq!(resolved.added, vec![(name.clone(), version.clone())]);
+        let changes = PackageChanges::between_manifests(&old, &new);
+        assert!(changes.changed.is_empty());
+        assert_eq!(changes.removed, vec![name.clone()]);
+        assert_eq!(changes.added, vec![(name.clone(), version.clone())]);
     }
 }