diff --git a/CHANGELOG.md b/CHANGELOG.md index 638b38b925b..d28f6ab9929 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,6 +48,11 @@ ([fruno](https://github.com/fruno-bulax/)) +- Fixed two bugs that made gleam not update the manifest correctly, causing + it to hit hex for version resolution on every operation and quickly reach + request limits in large projects. + ([fruno](https://github.com/fruno-bulax/)) + - The performance of `==` and `!=` has been improved for single-variant custom types when compiling to JavaScript. This was done by generating comparison code specific to the custom type rather than using the generic equality check diff --git a/compiler-cli/src/cli.rs b/compiler-cli/src/cli.rs index fb9b3aa74be..d8e5de034e6 100644 --- a/compiler-cli/src/cli.rs +++ b/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); } } diff --git a/compiler-cli/src/dependencies.rs b/compiler-cli/src/dependencies.rs index db576cbd5df..a35d48b1ae5 100644 --- a/compiler-cli/src/dependencies.rs +++ b/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(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. diff --git a/compiler-cli/src/dependencies/dependency_manager.rs b/compiler-cli/src/dependencies/dependency_manager.rs index 954f15c71e7..bb4dc8bd69a 100644 --- a/compiler-cli/src/dependencies/dependency_manager.rs +++ b/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. diff --git a/compiler-core/src/build/telemetry.rs b/compiler-core/src/build/telemetry.rs index d20d5194393..02344ef523d 100644 --- a/compiler-core/src/build/telemetry.rs +++ b/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) {} } diff --git a/compiler-core/src/manifest.rs b/compiler-core/src/manifest.rs index 920afb99262..6b2d28b7a67 100644 --- a/compiler-core/src/manifest.rs +++ b/compiler-core/src/manifest.rs @@ -215,6 +215,23 @@ pub enum ManifestPackageSource { Local { path: Utf8PathBuf }, // should be the canonical path } +impl ManifestPackageSource { + pub fn kind(&self) -> ManifestPackageSourceKind { + match self { + ManifestPackageSource::Hex { .. } => ManifestPackageSourceKind::Hex, + ManifestPackageSource::Git { .. } => ManifestPackageSourceKind::Git, + ManifestPackageSource::Local { .. } => ManifestPackageSourceKind::Local, + } + } +} + +#[derive(Debug, PartialEq, Eq, Clone, Copy)] +pub enum ManifestPackageSourceKind { + Hex, + Git, + Local, +} + fn sorted_vec(value: &[T], serializer: S) -> Result where S: serde::Serializer, @@ -483,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, /// When updating git dependencies, it is possible to update to a newer commit @@ -509,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 { @@ -523,29 +543,44 @@ 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![], + }, } } +} - /// 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 { +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() + } + + /// Compare 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![]; + let mut removed = vec![]; let mut old: HashMap<_, _> = old .packages .iter() @@ -554,6 +589,11 @@ impl Resolved { for new in &new.packages { match old.remove(&new.name) { + // If the kind of source changed, the packages bear essentially no connection + Some(old) if new.source.kind() != old.source.kind() => { + removed.push(old.name.clone()); + added.push((new.name.clone(), new.version.clone())); + } Some(old) if old.version == new.version => match (&old.source, &new.source) { ( ManifestPackageSource::Git { @@ -587,10 +627,9 @@ impl Resolved { } } - let removed = old.into_keys().cloned().collect(); + removed.extend(old.into_keys().cloned()); Self { - manifest: new, added, changed, changed_git, @@ -599,76 +638,118 @@ impl Resolved { } } -#[test] -fn resolved_with_updated() { - use crate::manifest::{Base16Checksum, ManifestPackage, ManifestPackageSource}; - - let package = |name: &str, version| ManifestPackage { - name: EcoString::from(name), - version, - build_tools: vec![], - otp_app: None, - requirements: vec![], - source: ManifestPackageSource::Hex { - outer_checksum: Base16Checksum(vec![]), - }, - }; - - let old = Manifest { - requirements: HashMap::new(), - packages: vec![ - package("unchanged1", Version::new(3, 0, 0)), - package("unchanged2", Version::new(0, 1, 0)), - package("changed1", Version::new(3, 0, 0)), - package("changed2", Version::new(0, 1, 0)), - package("removed1", Version::new(10, 0, 0)), - package("removed2", Version::new(20, 1, 0)), - ], - }; - - let new = Manifest { - requirements: HashMap::new(), - packages: vec![ - package("new1", Version::new(1, 0, 0)), - package("new2", Version::new(2, 1, 0)), - package("unchanged1", Version::new(3, 0, 0)), - package("unchanged2", Version::new(0, 1, 0)), - package("changed1", Version::new(5, 0, 0)), - package("changed2", Version::new(3, 0, 0)), - ], - }; - - 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(); - - assert_eq!( - resolved.added, - vec![ - ("new1".into(), Version::new(1, 0, 0)), - ("new2".into(), Version::new(2, 1, 0)), - ] - ); - - assert_eq!( - resolved.changed, - vec![ - Changed { - name: "changed1".into(), - old: Version::new(3, 0, 0), - new: Version::new(5, 0, 0) - }, - Changed { - name: "changed2".into(), - old: Version::new(0, 1, 0), - new: Version::new(3, 0, 0) +#[cfg(test)] +mod manifest_update_tests { + use std::collections::HashMap; + + use ecow::EcoString; + use hexpm::version::Version; + + use crate::manifest::{Base16Checksum, ManifestPackage, ManifestPackageSource, PackageChanges}; + use crate::manifest::{Changed, Manifest}; + + #[test] + fn resolved_with_updated() { + let package = |name: &str, version| ManifestPackage { + name: EcoString::from(name), + version, + build_tools: vec![], + otp_app: None, + requirements: vec![], + source: ManifestPackageSource::Hex { + outer_checksum: Base16Checksum(vec![]), }, - ] - ); + }; + + let old = Manifest { + requirements: HashMap::new(), + packages: vec![ + package("unchanged1", Version::new(3, 0, 0)), + package("unchanged2", Version::new(0, 1, 0)), + package("changed1", Version::new(3, 0, 0)), + package("changed2", Version::new(0, 1, 0)), + package("removed1", Version::new(10, 0, 0)), + package("removed2", Version::new(20, 1, 0)), + ], + }; + + let new = Manifest { + requirements: HashMap::new(), + packages: vec![ + package("new1", Version::new(1, 0, 0)), + package("new2", Version::new(2, 1, 0)), + package("unchanged1", Version::new(3, 0, 0)), + package("unchanged2", Version::new(0, 1, 0)), + package("changed1", Version::new(5, 0, 0)), + package("changed2", Version::new(3, 0, 0)), + ], + }; - assert_eq!( - resolved.removed, - vec![EcoString::from("removed1"), EcoString::from("removed2")] - ); + 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!( + changes.added, + vec![ + ("new1".into(), Version::new(1, 0, 0)), + ("new2".into(), Version::new(2, 1, 0)), + ] + ); + + assert_eq!( + changes.changed, + vec![ + Changed { + name: "changed1".into(), + old: Version::new(3, 0, 0), + new: Version::new(5, 0, 0) + }, + Changed { + name: "changed2".into(), + old: Version::new(0, 1, 0), + new: Version::new(3, 0, 0) + }, + ] + ); + + assert_eq!( + changes.removed, + vec![EcoString::from("removed1"), EcoString::from("removed2")] + ); + } + + #[test] + fn resolved_with_source_type_change() { + let name = EcoString::from("wibble"); + let version = Version::new(1, 0, 0); + let package = |source| ManifestPackage { + name: name.clone(), + version: version.clone(), + build_tools: vec![], + otp_app: None, + requirements: vec![], + source, + }; + + let old = Manifest { + requirements: HashMap::new(), + packages: vec![package(ManifestPackageSource::Local { + path: "wibble".into(), + })], + }; + + let new = Manifest { + requirements: HashMap::new(), + packages: vec![package(ManifestPackageSource::Hex { + outer_checksum: Base16Checksum(vec![]), + })], + }; + + 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())]); + } }