Skip to content

Commit

Permalink
chore(lockfiles) avoid hand written reflection (#6143)
Browse files Browse the repository at this point in the history
### Description

As part of DYF I wanted to see if I could get rid of the
`global_changes_key` part of the `Lockfile` trait by leveraging `Any`.
Using `Any` has two benefits:
- It doesn't depend on uniqueness of the magic string chosen to be part
of the buffer that forms the key. (We've [already almost had an
issue](#5934 (comment)))
- It doesn't require any allocations in order to perform this comparison

This gets us back to how the Go code used to work where we would take a
pointer to a `Lockfile` interface and attempt to downcast it to the same
concrete type before comparing fields.

### Testing Instructions

Verifying that `--filter=[HEAD^1]` works as expected:
 - `npx create-turbo@latest test pnpm`
- `cd test && pnpm rm turbo && git add . && git commit -m 'rm turbo'`
(Remove local version of turbo so we can use global dev version easily)
- `cd apps/web && pnpm add is-number && cd ../../ && git add . && git
commit -m 'add is-number'`
- `turbo_dev build --filter='[HEAD^1]' --experimental-rust-codepath`
should only run `build` for `web`

Closes TURBO-1443

---------

Co-authored-by: Chris Olszewski <Chris Olszewski>
  • Loading branch information
chris-olszewski authored Oct 18, 2023
1 parent 66446da commit ef2b800
Show file tree
Hide file tree
Showing 8 changed files with 72 additions and 69 deletions.
2 changes: 1 addition & 1 deletion crates/turborepo-lib/src/engine/builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -436,7 +436,7 @@ mod test {
unreachable!()
}

fn global_change_key(&self) -> Vec<u8> {
fn global_change(&self, _other: &dyn Lockfile) -> bool {
unreachable!()
}
}
Expand Down
9 changes: 3 additions & 6 deletions crates/turborepo-lib/src/package_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -327,10 +327,7 @@ impl PackageGraph {

let closures = turborepo_lockfiles::all_transitive_closures(previous, external_deps)?;

let current_key = current.global_change_key();
let previous_key = previous.global_change_key();

let global_change = current_key != previous_key;
let global_change = current.global_change(previous);

let changed = if global_change {
None
Expand Down Expand Up @@ -549,8 +546,8 @@ mod test {
unreachable!("lockfile encoding not necessary for package graph construction")
}

fn global_change_key(&self) -> Vec<u8> {
unreachable!()
fn global_change(&self, _other: &dyn Lockfile) -> bool {
unreachable!("global change detection not necessary for package graph construction")
}
}

Expand Down
23 changes: 9 additions & 14 deletions crates/turborepo-lockfiles/src/berry/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ mod resolution;
mod ser;

use std::{
any::Any,
collections::{HashMap, HashSet},
iter,
};
Expand All @@ -13,7 +14,6 @@ use de::SemverString;
use identifiers::{Descriptor, Locator};
use protocol_resolver::DescriptorResolver;
use serde::{Deserialize, Serialize};
use serde_json::json;
use thiserror::Error;
use turbopath::RelativeUnixPathBuf;

Expand Down Expand Up @@ -548,19 +548,14 @@ impl Lockfile for BerryLockfile {
Ok(patches)
}

fn global_change_key(&self) -> Vec<u8> {
let mut buf = vec![b'b', b'e', b'r', b'r', b'y', 0];

serde_json::to_writer(
&mut buf,
&json!({
"version": &self.data.metadata.version,
"cache_key": &self.data.metadata.cache_key,
}),
)
.expect("writing to Vec cannot fail");

buf
fn global_change(&self, other: &dyn Lockfile) -> bool {
let any_other = other as &dyn Any;
if let Some(other) = any_other.downcast_ref::<Self>() {
self.data.metadata.version != other.data.metadata.version
|| self.data.metadata.cache_key != other.data.metadata.cache_key
} else {
true
}
}
}

Expand Down
9 changes: 6 additions & 3 deletions crates/turborepo-lockfiles/src/bun/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::str::FromStr;
use std::{any::Any, str::FromStr};

use serde::Deserialize;

Expand Down Expand Up @@ -109,8 +109,11 @@ impl Lockfile for BunLockfile {
Err(crate::Error::Bun(Error::NotImplemented()))
}

fn global_change_key(&self) -> Vec<u8> {
vec![b'b', b'u', b'n', 0]
fn global_change(&self, other: &dyn Lockfile) -> bool {
let any_other = other as &dyn Any;
// Downcast returns none if the concrete type doesn't match
// if the types don't match then we changed package managers
any_other.downcast_ref::<Self>().is_none()
}
}

Expand Down
17 changes: 8 additions & 9 deletions crates/turborepo-lockfiles/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
#![feature(trait_upcasting)]
#![deny(clippy::all)]

mod berry;
Expand All @@ -7,7 +8,10 @@ mod npm;
mod pnpm;
mod yarn1;

use std::collections::{HashMap, HashSet};
use std::{
any::Any,
collections::{HashMap, HashSet},
};

pub use berry::{Error as BerryError, *};
pub use bun::BunLockfile;
Expand All @@ -27,7 +31,7 @@ pub struct Package {
// This trait will only be used when migrating the Go lockfile implementations
// to Rust. Once the migration is complete we will leverage petgraph for doing
// our graph calculations.
pub trait Lockfile: Send + Sync {
pub trait Lockfile: Send + Sync + Any {
// Given a workspace, a package it imports and version returns the key, resolved
// version, and if it was found
fn resolve_package(
Expand All @@ -53,13 +57,8 @@ pub trait Lockfile: Send + Sync {
Ok(Vec::new())
}

/// Present a global change key which is compared against two lockfiles
///
/// Impl notes: please prefix this key with some magic identifier
/// to prevent clashes. we are not worried about inter-version
/// compatibility so these keys don't need to be stable. They are
/// ephemeral.
fn global_change_key(&self) -> Vec<u8>;
/// Determine if there's a global change between two lockfiles
fn global_change(&self, other: &dyn Lockfile) -> bool;
}

/// Takes a lockfile, and a map of workspace directory paths -> (package name,
Expand Down
25 changes: 10 additions & 15 deletions crates/turborepo-lockfiles/src/npm.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use std::collections::HashMap;
use std::{any::Any, collections::HashMap};

use serde::{Deserialize, Serialize};
use serde_json::{json, Value};
use serde_json::Value;

use super::{Error, Lockfile, Package};

Expand Down Expand Up @@ -136,19 +136,14 @@ impl Lockfile for NpmLockfile {
Ok(serde_json::to_vec_pretty(&self)?)
}

fn global_change_key(&self) -> Vec<u8> {
let mut buf = vec![b'n', b'p', b'm', 0];

serde_json::to_writer(
&mut buf,
&json!({
"requires": &self.other.get("requires"),
"version": &self.lockfile_version,
}),
)
.expect("writing to Vec cannot fail");

buf
fn global_change(&self, other: &dyn Lockfile) -> bool {
let any_other = other as &dyn Any;
if let Some(other) = any_other.downcast_ref::<Self>() {
self.lockfile_version != other.lockfile_version
|| self.other.get("requires") != other.other.get("requires")
} else {
true
}
}
}

Expand Down
47 changes: 29 additions & 18 deletions crates/turborepo-lockfiles/src/pnpm/data.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use std::borrow::Cow;
use std::{any::Any, borrow::Cow, collections::BTreeMap};

use serde::{Deserialize, Serialize};
use serde_json::json;
use turbopath::RelativeUnixPathBuf;

use super::{dep_path::DepPath, Error, LockfileVersion};
Expand Down Expand Up @@ -252,6 +251,27 @@ impl PnpmLockfile {
}
Ok(pruned_patches)
}

// Create a projection of all fields in the lockfile that could affect all
// workspaces
fn global_fields(&self) -> GlobalFields {
GlobalFields {
version: &self.lockfile_version.version,
checksum: self.package_extensions_checksum.as_deref(),
overrides: self.overrides.as_ref(),
patched_dependencies: self.patched_dependencies.as_ref(),
settings: self.settings.as_ref(),
}
}
}

#[derive(Debug, PartialEq, Eq)]
struct GlobalFields<'a> {
version: &'a str,
checksum: Option<&'a str>,
overrides: Option<&'a BTreeMap<String, String>>,
patched_dependencies: Option<&'a BTreeMap<String, PatchFile>>,
settings: Option<&'a LockfileSettings>,
}

impl crate::Lockfile for PnpmLockfile {
Expand Down Expand Up @@ -401,22 +421,13 @@ impl crate::Lockfile for PnpmLockfile {
Ok(patches)
}

fn global_change_key(&self) -> Vec<u8> {
let mut buf = vec![b'p', b'n', b'p', b'm', 0];

serde_json::to_writer(
&mut buf,
&json!({
"version": self.lockfile_version.version,
"checksum": self.package_extensions_checksum,
"overrides": self.overrides,
"patched_deps": self.patched_dependencies,
"settings": self.settings,
}),
)
.expect("writing to Vec cannot fail");

buf
fn global_change(&self, other: &dyn crate::Lockfile) -> bool {
let any_other = other as &dyn Any;
if let Some(other) = any_other.downcast_ref::<Self>() {
self.global_fields() != other.global_fields()
} else {
true
}
}
}

Expand Down
9 changes: 6 additions & 3 deletions crates/turborepo-lockfiles/src/yarn1/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use std::str::FromStr;
use std::{any::Any, str::FromStr};

use serde::Deserialize;

Expand Down Expand Up @@ -108,8 +108,11 @@ impl Lockfile for Yarn1Lockfile {
Ok(self.to_string().into_bytes())
}

fn global_change_key(&self) -> Vec<u8> {
vec![b'y', b'a', b'r', b'n', 0]
fn global_change(&self, other: &dyn Lockfile) -> bool {
let any_other = other as &dyn Any;
// Downcast returns none if the concrete type doesn't match
// if the types don't match then we changed package managers
any_other.downcast_ref::<Self>().is_none()
}
}

Expand Down

0 comments on commit ef2b800

Please sign in to comment.