From ef8a7cf9b3cb75f3e8c3a07ce85ccf2b837c8482 Mon Sep 17 00:00:00 2001 From: daimond113 Date: Wed, 30 Apr 2025 20:02:41 +0200 Subject: [PATCH] fix: rewrite dependency type system Previously, a naive dependency type was used. This had 2 fields in a dependency graph node: resolved_ty and is_peer. These fields were incorrect, since the dependency can be dependended on as multiple types. Now, the dependency type is stored in the dependencies fields. This change makes old lockfiles incompatible since the old format had data loss when it comes to dependency types. Fixes #33. --- CHANGELOG.md | 1 + src/cli/commands/publish.rs | 5 +- src/cli/install.rs | 59 +++++++++++++++++- src/download_and_link.rs | 4 +- src/graph.rs | 7 +-- src/linking/mod.rs | 2 +- src/lockfile.rs | 120 +++--------------------------------- src/resolver.rs | 39 ++---------- 8 files changed, 81 insertions(+), 156 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2b7302b..71418aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - Download engines in install step rather than lazily by @daimond113 +- Rewrite dependency type system to solve multiple issues by @daimond113 ### Performance - Remove unnecessary `Arc`s from codebase by @daimond113 diff --git a/src/cli/commands/publish.rs b/src/cli/commands/publish.rs index ee9c36c..bb11ade 100644 --- a/src/cli/commands/publish.rs +++ b/src/cli/commands/publish.rs @@ -172,7 +172,10 @@ impl PublishCommand { Ok::<_, anyhow::Error>( target.build_files().is_none() - && !matches!(node.resolved_ty, DependencyType::Dev), + && node + .direct + .as_ref() + .is_some_and(|(_, _, ty)| *ty != DependencyType::Dev), ) } }) diff --git a/src/cli/install.rs b/src/cli/install.rs index 65b7221..49a2507 100644 --- a/src/cli/install.rs +++ b/src/cli/install.rs @@ -53,7 +53,7 @@ impl DownloadAndLinkHooks for InstallHooks { let aliases = graph .iter() .flat_map(|(_, node)| node.node.dependencies.iter()) - .filter_map(|(id, alias)| binary_packages.contains(id).then_some(alias.as_str())) + .filter_map(|(id, (alias, _))| binary_packages.contains(id).then_some(alias.as_str())) .chain( graph .iter() @@ -202,6 +202,8 @@ pub async fn install( .await .context("failed to build dependency graph")?; + check_peers_satisfied(&graph); + let mut tasks = graph .iter() .filter_map(|(id, node)| { @@ -537,3 +539,58 @@ pub fn print_package_diff(prefix: &str, old_graph: &DependencyGraph, new_graph: println!(); } } + +pub fn check_peers_satisfied(graph: &DependencyGraph) { + for (id, node) in graph { + let Some((alias, _, _)) = &node.direct else { + continue; + }; + + let mut queue = node + .dependencies + .iter() + .map(|(dep_id, (dep_alias, dep_ty))| (vec![(id, alias)], (dep_id, dep_alias), *dep_ty)) + .collect::>(); + + while let Some((path, (dep_id, dep_alias), dep_ty)) = queue.pop() { + if dep_ty == DependencyType::Peer { + let mut iter = path + .iter() + .map(|(id, _)| id) + .rev() + // skip our parent since we're always going to be descendants of it + .skip(1) + .take(2); + + let satisfied = if iter.len() > 0 { + iter.any(|id| graph[id].dependencies.contains_key(dep_id)) + } else { + graph.get(dep_id).is_some_and(|node| node.direct.is_some()) + }; + + if !satisfied { + eprintln!( + "{WARN_PREFIX}: peer dependency {}>{dep_alias} is not satisfied", + path.iter() + .map(|(_, alias)| alias.as_str()) + .collect::>() + .join(">"), + ); + } + } + + queue.extend(graph[dep_id].dependencies.iter().map( + |(inner_dep_id, (inner_dep_alias, inner_dep_ty))| { + ( + path.iter() + .copied() + .chain(std::iter::once((dep_id, dep_alias))) + .collect(), + (inner_dep_id, inner_dep_alias), + *inner_dep_ty, + ) + }, + )); + } + } +} diff --git a/src/download_and_link.rs b/src/download_and_link.rs index 92d8888..6d2f85a 100644 --- a/src/download_and_link.rs +++ b/src/download_and_link.rs @@ -268,7 +268,9 @@ impl Project { let mut queue = graph .iter() .filter(|(_, node)| { - node.direct.is_some() && install_dependencies_mode.fits(node.resolved_ty) + node.direct + .as_ref() + .is_some_and(|(_, _, ty)| install_dependencies_mode.fits(*ty)) }) .collect::>(); diff --git a/src/graph.rs b/src/graph.rs index b326abc..d5807bd 100644 --- a/src/graph.rs +++ b/src/graph.rs @@ -25,12 +25,7 @@ pub struct DependencyGraphNode { pub direct: Option<(Alias, DependencySpecifiers, DependencyType)>, /// The dependencies of the package #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub dependencies: BTreeMap, - /// The resolved (transformed, for example Peer -> Standard) type of the dependency - pub resolved_ty: DependencyType, - /// Whether the resolved type should be Peer if this isn't depended on - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub is_peer: bool, + pub dependencies: BTreeMap, /// The package reference pub pkg_ref: PackageRefs, } diff --git a/src/linking/mod.rs b/src/linking/mod.rs index da04291..5d1ac44 100644 --- a/src/linking/mod.rs +++ b/src/linking/mod.rs @@ -307,7 +307,7 @@ impl Project { (container_folder, base_folder) }; - for (dep_id, dep_alias) in &node.node.dependencies { + for (dep_id, (dep_alias, _)) in &node.node.dependencies { let dep_id = dep_id.clone(); let dep_alias = dep_alias.clone(); let dep_node = graph.get(&dep_id).cloned(); diff --git a/src/lockfile.rs b/src/lockfile.rs index 653f1fa..c455488 100644 --- a/src/lockfile.rs +++ b/src/lockfile.rs @@ -3,15 +3,15 @@ use crate::{ graph::DependencyGraph, manifest::{overrides::OverrideKey, target::TargetKind}, names::PackageName, - source::{ids::PackageId, specifiers::DependencySpecifiers}, + source::specifiers::DependencySpecifiers, }; use relative_path::RelativePathBuf; use semver::Version; use serde::{Deserialize, Serialize}; -use std::{collections::BTreeMap, fmt::Debug}; +use std::collections::BTreeMap; /// The current format of the lockfile -pub const CURRENT_FORMAT: usize = 1; +pub const CURRENT_FORMAT: usize = 2; /// A lockfile #[derive(Serialize, Deserialize, Debug, Clone)] @@ -47,120 +47,12 @@ pub fn parse_lockfile(lockfile: &str) -> Result { - let this = match toml::from_str(lockfile) { - Ok(lockfile) => return Ok(lockfile), - Err(e) => match toml::from_str::(lockfile) { - Ok(this) => this, - Err(_) => return Err(errors::ParseLockfileError::De(e)), - }, - }; - - Ok(Lockfile { - name: this.name, - version: this.version, - target: this.target, - overrides: this.overrides, - workspace: this.workspace, - graph: this - .graph - .into_iter() - .flat_map(|(name, versions)| { - versions.into_iter().map(move |(version, node)| { - ( - PackageId(name.clone(), version), - crate::graph::DependencyGraphNode { - direct: node.node.direct, - dependencies: node - .node - .dependencies - .into_iter() - .map(|(name, (version, alias))| { - (PackageId(name, version), alias) - }) - .collect(), - resolved_ty: node.node.resolved_ty, - is_peer: node.node.is_peer, - pkg_ref: node.node.pkg_ref, - }, - ) - }) - }) - .collect(), - }) - } CURRENT_FORMAT => toml::de::from_str(lockfile).map_err(Into::into), + format if format < CURRENT_FORMAT => Err(errors::ParseLockfileError::TooOld(format)), format => Err(errors::ParseLockfileError::TooNew(format)), } } -/// Lockfile v0 -pub mod v0 { - use crate::{ - manifest::{ - overrides::OverrideKey, - target::{Target, TargetKind}, - Alias, DependencyType, - }, - names::{PackageName, PackageNames}, - source::{ids::VersionId, refs::PackageRefs, specifiers::DependencySpecifiers}, - }; - use relative_path::RelativePathBuf; - use semver::Version; - use serde::{Deserialize, Serialize}; - use std::collections::BTreeMap; - - /// A dependency graph node - #[derive(Serialize, Deserialize, Debug, Clone)] - pub struct DependencyGraphNode { - /// The alias, specifier, and original (as in the manifest) type for the dependency, if it is a direct dependency (i.e. used by the current project) - #[serde(default, skip_serializing_if = "Option::is_none")] - pub direct: Option<(Alias, DependencySpecifiers, DependencyType)>, - /// The dependencies of the package - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub dependencies: BTreeMap, - /// The resolved (transformed, for example Peer -> Standard) type of the dependency - pub resolved_ty: DependencyType, - /// Whether the resolved type should be Peer if this isn't depended on - #[serde(default, skip_serializing_if = "std::ops::Not::not")] - pub is_peer: bool, - /// The package reference - pub pkg_ref: PackageRefs, - } - - /// A downloaded dependency graph node, i.e. a `DependencyGraphNode` with a `Target` - #[derive(Serialize, Deserialize, Debug, Clone)] - pub struct DownloadedDependencyGraphNode { - /// The target of the package - pub target: Target, - /// The node - #[serde(flatten)] - pub node: DependencyGraphNode, - } - - /// A lockfile - #[derive(Serialize, Deserialize, Debug, Clone)] - pub struct Lockfile { - /// The name of the package - pub name: PackageName, - /// The version of the package - pub version: Version, - /// The target of the package - pub target: TargetKind, - /// The overrides of the package - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub overrides: BTreeMap, - - /// The workspace members - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub workspace: BTreeMap>, - - /// The graph of dependencies - #[serde(default, skip_serializing_if = "BTreeMap::is_empty")] - pub graph: BTreeMap>, - } -} - /// Errors that can occur when working with lockfiles pub mod errors { use thiserror::Error; @@ -173,6 +65,10 @@ pub mod errors { #[error("lockfile format {} is too new. newest supported format: {}", .0, super::CURRENT_FORMAT)] TooNew(usize), + /// The lockfile format is too old + #[error("lockfile format {} is too old. manual deletion is required. current format: {}", .0, super::CURRENT_FORMAT)] + TooOld(usize), + /// Deserializing the lockfile failed #[error("deserializing the lockfile failed")] De(#[from] toml::de::Error), diff --git a/src/resolver.rs b/src/resolver.rs index 7d60b14..2051600 100644 --- a/src/resolver.rs +++ b/src/resolver.rs @@ -114,7 +114,9 @@ impl Project { let mut queue = node .dependencies .iter() - .map(|(id, dep_alias)| (id, vec![alias.to_string(), dep_alias.to_string()])) + .map(|(id, (dep_alias, _))| { + (id, vec![alias.to_string(), dep_alias.to_string()]) + }) .collect::>(); while let Some((dep_id, path)) = queue.pop_front() { @@ -135,7 +137,7 @@ impl Project { dep_node .dependencies .iter() - .map(|(id, alias)| { + .map(|(id, (alias, _))| { ( id, path.iter() @@ -270,19 +272,12 @@ impl Project { ))); }; - let resolved_ty = if (is_published_package || depth == 0) && ty == DependencyType::Peer - { - DependencyType::Standard - } else { - ty - }; - if let Some(dependant_id) = dependant { graph .get_mut(&dependant_id) .expect("dependant package not found in graph") .dependencies - .insert(package_id.clone(), alias.clone()); + .insert(package_id.clone(), (alias.clone(), ty)); } let pkg_ref = &resolved[package_id.version_id()]; @@ -296,14 +291,6 @@ impl Project { ); } - if already_resolved.resolved_ty == DependencyType::Peer { - already_resolved.resolved_ty = resolved_ty; - } - - if ty == DependencyType::Peer && depth == 0 { - already_resolved.is_peer = true; - } - if already_resolved.direct.is_none() && depth == 0 { already_resolved.direct = Some((alias.clone(), specifier.clone(), ty)); } @@ -315,12 +302,6 @@ impl Project { direct: (depth == 0).then(|| (alias.clone(), specifier.clone(), ty)), pkg_ref: pkg_ref.clone(), dependencies: Default::default(), - resolved_ty, - is_peer: if depth == 0 { - false - } else { - ty == DependencyType::Peer - }, }; insert_node( &mut graph, @@ -388,16 +369,6 @@ impl Project { .await?; } - for (id, node) in &mut graph { - if node.is_peer && node.direct.is_none() { - node.resolved_ty = DependencyType::Peer; - } - - if node.resolved_ty == DependencyType::Peer { - tracing::warn!("peer dependency {id} was not resolved"); - } - } - Ok(graph) } }