Skip to content

Commit

Permalink
Adopt CacheDiff (crate) over MetadataDiff (internal) (#370)
Browse files Browse the repository at this point in the history
* Upgrade Rust version

* Add CacheDiff to dependencies

* Import and implement CacheDiff derive macro

* Use CacheDiff implementation for MetadataDiff

The CacheDiff is a public crate while MetadataDiff was a temporary experimentation platform internal to this buildpack

* Update magic_migrate

Due to schneems/magic_migrate#7

* Introduce metadata v3

The idea is to centralize all OS distribution information into a single attribute.

It's not yet used.

* Use metadata v3 for bundle_install

The coming move to CacheDiff prefers if one attribute relates to one difference reason. V2 introduced a distribution name and distribution value which we later determined looked better if it's combined to look like a single attribute to the user "OS Distribution." This move brings the attributes inline with how they're compared, now both name and value are evaluated and emited as a single unit.

* Implement CacheDiff on bundle_install layer

(Tests do not compile yet)

* Update tests for CacheDiff change

* Move OS distribution into a type

* Implement v3 metadata with condensed OS distribution

The coming move to CacheDiff prefers if one attribute relates to one difference reason. V2 introduced a distribution name and distribution value which we later determined looked better if it's combined to look like a single attribute to the user "OS Distribution." This move brings the attributes inline with how they're compared, now both name and value are evaluated and emitted as a single unit.

* Fix tests due to V3 migration

* Implement CacheDiff for ruby_install_layer

Does not compile yet due to tests

* Fix test compilation

* Implement CacheDiff for shared tests

* Update shared interfaces to use CacheDiff instead of MetadataDiff

* Replace qualified path with use

* Remove unused internal trait MetadataDiff

* Use regex for integration test match

* Use diff method directly

In the refactor we had two `diff` methods, now there's only one so we can use it instead of having to reference the trait. This shaves off some lines of the total diff.

* Format use statements
  • Loading branch information
schneems authored Dec 13, 2024
1 parent b0064c1 commit d2fd043
Show file tree
Hide file tree
Showing 10 changed files with 237 additions and 158 deletions.
55 changes: 53 additions & 2 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ members = ["buildpacks/ruby", "commons"]

[workspace.package]
edition = "2021"
rust-version = "1.80"
rust-version = "1.82"

[workspace.lints.rust]
unreachable_pub = "warn"
Expand Down
1 change: 1 addition & 0 deletions buildpacks/ruby/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ ureq = { version = "2", default-features = false, features = ["tls"] }
url = "2"
magic_migrate = "0.2"
toml = "0.8"
cache_diff = { version = "1.0.0", features = ["bullet_stream"] }

[dev-dependencies]
libcnb-test = "=0.26.1"
20 changes: 4 additions & 16 deletions buildpacks/ruby/src/layers/bundle_download_layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@
//!
//! Installs a copy of `bundler` to the `<layer-dir>` with a bundler executable in
//! `<layer-dir>/bin`. Must run before [`crate.steps.bundle_install`].
use crate::layers::shared::{cached_layer_write_metadata, MetadataDiff};
use crate::layers::shared::cached_layer_write_metadata;
use crate::RubyBuildpack;
use crate::RubyBuildpackError;
use bullet_stream::state::SubBullet;
use bullet_stream::{style, Print};
use cache_diff::CacheDiff;
use commons::gemfile_lock::ResolvedBundlerVersion;
use fun_run::{self, CommandWithName};
use libcnb::data::layer_name;
Expand Down Expand Up @@ -57,22 +58,9 @@ try_migrate_deserializer_chain!(
chain: [MetadataV1],
);

impl MetadataDiff for Metadata {
fn diff(&self, other: &Self) -> Vec<String> {
let mut differences = Vec::new();
if self.version != other.version {
differences.push(format!(
"Bundler version ({old} to {now})",
old = style::value(other.version.to_string()),
now = style::value(self.version.to_string())
));
}
differences
}
}

#[derive(Deserialize, Serialize, Debug, Clone)]
#[derive(Deserialize, Serialize, Debug, Clone, CacheDiff)]
pub(crate) struct MetadataV1 {
#[cache_diff(rename = "Bundler version")]
pub(crate) version: ResolvedBundlerVersion,
}

Expand Down
116 changes: 59 additions & 57 deletions buildpacks/ruby/src/layers/bundle_install_layer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,12 @@
//! must be compiled and will then be invoked via FFI. These native extensions are
//! OS, Architecture, and Ruby version dependent. Due to this, when one of these changes
//! we must clear the cache and re-run `bundle install`.
use crate::layers::shared::{cached_layer_write_metadata, Meta, MetadataDiff};
use crate::target_id::{TargetId, TargetIdError};
use crate::layers::shared::{cached_layer_write_metadata, Meta};
use crate::target_id::{OsDistribution, TargetId, TargetIdError};
use crate::{BundleWithout, RubyBuildpack, RubyBuildpackError};
use bullet_stream::state::SubBullet;
use bullet_stream::{style, Print};
use cache_diff::CacheDiff;
use commons::{
display::SentenceList, gemfile_lock::ResolvedRubyVersion, metadata_digest::MetadataDigest,
};
Expand Down Expand Up @@ -116,51 +117,13 @@ pub(crate) fn handle(
Ok((bullet, layer_ref.read_env()?))
}

pub(crate) type Metadata = MetadataV2;
pub(crate) type Metadata = MetadataV3;
try_migrate_deserializer_chain!(
chain: [MetadataV1, MetadataV2],
chain: [MetadataV1, MetadataV2, MetadataV3],
error: MetadataMigrateError,
deserializer: toml::Deserializer::new,
);

impl MetadataDiff for Metadata {
fn diff(&self, old: &Self) -> Vec<String> {
let mut differences = Vec::new();
let Metadata {
distro_name,
distro_version,
cpu_architecture,
ruby_version,
force_bundle_install_key: _,
digest: _,
} = old;

if ruby_version != &self.ruby_version {
differences.push(format!(
"Ruby version ({old} to {now})",
old = style::value(ruby_version.to_string()),
now = style::value(self.ruby_version.to_string())
));
}
if distro_name != &self.distro_name || distro_version != &self.distro_version {
differences.push(format!(
"Distribution ({old} to {now})",
old = style::value(format!("{distro_name} {distro_version}")),
now = style::value(format!("{} {}", self.distro_name, self.distro_version))
));
}
if cpu_architecture != &self.cpu_architecture {
differences.push(format!(
"CPU architecture ({old} to {now})",
old = style::value(cpu_architecture),
now = style::value(&self.cpu_architecture)
));
}

differences
}
}

#[derive(Deserialize, Serialize, Debug, Clone, Eq, PartialEq)]
pub(crate) struct MetadataV1 {
pub(crate) stack: String,
Expand All @@ -176,6 +139,19 @@ pub(crate) struct MetadataV2 {
pub(crate) cpu_architecture: String,
pub(crate) ruby_version: ResolvedRubyVersion,
pub(crate) force_bundle_install_key: String,
pub(crate) digest: MetadataDigest, // Must be last for serde to be happy https://github.com/toml-rs/toml-rs/issues/142
}

#[derive(Deserialize, Serialize, Debug, Clone, Eq, PartialEq, CacheDiff)]
pub(crate) struct MetadataV3 {
#[cache_diff(rename = "OS Distribution")]
pub(crate) os_distribution: OsDistribution,
#[cache_diff(rename = "CPU Architecture")]
pub(crate) cpu_architecture: String,
#[cache_diff(rename = "Ruby version")]
pub(crate) ruby_version: ResolvedRubyVersion,
#[cache_diff(ignore)]
pub(crate) force_bundle_install_key: String,

/// A struct that holds the cryptographic hash of components that can
/// affect the result of `bundle install`. When these values do not
Expand All @@ -188,6 +164,7 @@ pub(crate) struct MetadataV2 {
/// This value is cached with metadata, so changing the struct
/// may cause metadata to be invalidated (and the cache cleared).
///
#[cache_diff(ignore)]
pub(crate) digest: MetadataDigest, // Must be last for serde to be happy https://github.com/toml-rs/toml-rs/issues/142
}

Expand Down Expand Up @@ -217,6 +194,23 @@ impl TryFrom<MetadataV1> for MetadataV2 {
}
}

impl TryFrom<MetadataV2> for MetadataV3 {
type Error = Infallible;

fn try_from(v2: MetadataV2) -> Result<Self, Self::Error> {
Ok(Self {
os_distribution: OsDistribution {
name: v2.distro_name,
version: v2.distro_version,
},
cpu_architecture: v2.cpu_architecture,
ruby_version: v2.ruby_version,
force_bundle_install_key: v2.force_bundle_install_key,
digest: v2.digest,
})
}
}

#[derive(Debug)]
enum InstallState {
/// Holds message indicating the reason why we want to run 'bundle install'
Expand Down Expand Up @@ -334,7 +328,7 @@ mod test {
use super::*;
use std::path::PathBuf;

/// `MetadataDiff` logic controls cache invalidation
/// `CacheDiff` logic controls cache invalidation
/// When the vec is empty the cache is kept, otherwise it is invalidated
#[test]
fn metadata_diff_messages() {
Expand All @@ -350,8 +344,10 @@ mod test {

let old = Metadata {
ruby_version: ResolvedRubyVersion("3.5.3".to_string()),
distro_name: "ubuntu".to_string(),
distro_version: "20.04".to_string(),
os_distribution: OsDistribution {
name: "ubuntu".to_string(),
version: "20.04".to_string(),
},
cpu_architecture: "amd64".to_string(),
force_bundle_install_key: FORCE_BUNDLE_INSTALL_CACHE_KEY.to_string(),
digest: MetadataDigest::new_env_files(
Expand All @@ -364,22 +360,24 @@ mod test {

let diff = Metadata {
ruby_version: ResolvedRubyVersion("3.5.5".to_string()),
distro_name: old.distro_name.clone(),
distro_version: old.distro_version.clone(),
os_distribution: old.os_distribution.clone(),
cpu_architecture: old.cpu_architecture.clone(),
force_bundle_install_key: old.force_bundle_install_key.clone(),
digest: old.digest.clone(),
}
.diff(&old);

assert_eq!(
diff.iter().map(strip_ansi).collect::<Vec<String>>(),
vec!["Ruby version (`3.5.3` to `3.5.5`)".to_string()]
);

let diff = Metadata {
ruby_version: old.ruby_version.clone(),
distro_name: "alpine".to_string(),
distro_version: "3.20.0".to_string(),
os_distribution: OsDistribution {
name: "alpine".to_string(),
version: "3.20.0".to_string(),
},
cpu_architecture: old.cpu_architecture.clone(),
force_bundle_install_key: old.force_bundle_install_key.clone(),
digest: old.digest.clone(),
Expand All @@ -388,21 +386,21 @@ mod test {

assert_eq!(
diff.iter().map(strip_ansi).collect::<Vec<String>>(),
vec!["Distribution (`ubuntu 20.04` to `alpine 3.20.0`)".to_string()]
vec!["OS Distribution (`ubuntu 20.04` to `alpine 3.20.0`)".to_string()]
);

let diff = Metadata {
ruby_version: old.ruby_version.clone(),
distro_name: old.distro_name.clone(),
distro_version: old.distro_version.clone(),
os_distribution: old.os_distribution.clone(),
cpu_architecture: "arm64".to_string(),
force_bundle_install_key: old.force_bundle_install_key.clone(),
digest: old.digest.clone(),
}
.diff(&old);

assert_eq!(
diff.iter().map(strip_ansi).collect::<Vec<String>>(),
vec!["CPU architecture (`amd64` to `arm64`)".to_string()]
vec!["CPU Architecture (`amd64` to `arm64`)".to_string()]
);
}

Expand Down Expand Up @@ -476,8 +474,10 @@ GEM_PATH=layer_path

let target_id = TargetId::from_stack("heroku-22").unwrap();
let metadata = Metadata {
distro_name: target_id.distro_name,
distro_version: target_id.distro_version,
os_distribution: OsDistribution {
name: target_id.distro_name.clone(),
version: target_id.distro_version.clone(),
},
cpu_architecture: target_id.cpu_architecture,
ruby_version: ResolvedRubyVersion(String::from("3.1.3")),
force_bundle_install_key: String::from("v1"),
Expand All @@ -492,12 +492,14 @@ GEM_PATH=layer_path
let gemfile_path = gemfile.display();
let toml_string = format!(
r#"
distro_name = "ubuntu"
distro_version = "22.04"
cpu_architecture = "amd64"
ruby_version = "3.1.3"
force_bundle_install_key = "v1"
[os_distribution]
name = "ubuntu"
version = "22.04"
[digest]
platform_env = "c571543beaded525b7ee46ceb0b42c0fb7b9f6bfc3a211b3bbcfe6956b69ace3"
Expand Down
Loading

0 comments on commit d2fd043

Please sign in to comment.