From 18aeaaac24a3b3152a4f0647b616b06cf87c0099 Mon Sep 17 00:00:00 2001 From: Richard Schneeman Date: Thu, 7 Sep 2023 14:08:54 -0500 Subject: [PATCH] Change Layer interface from `&self` to `&mut self` The primary driver for wanting to mutate a layer is a desire for stateful logging. I hit this in the consuming state-machine interface, and previously, Josh hit it in a logging interface designed to track indentation level. Upside: - Developers are now allowed to do whatever they want within a layer - It's more technically correct to require a single reference for these functions. With the prior interface, it would be theoretically possible to execute the same Layer in parallel (though unlikely) - We can now change things between function calls. Downside: - Mutable self means there's no guarantee something didn't change between function calls. --- CHANGELOG.md | 5 + examples/execd/src/layer.rs | 2 +- examples/ruby-sample/src/layers/bundler.rs | 121 ++++++++++++++++++ examples/ruby-sample/src/layers/ruby.rs | 61 +++++++++ libcnb/src/build.rs | 2 +- libcnb/src/layer/handling.rs | 14 +- libcnb/src/layer/public_interface.rs | 6 +- libcnb/src/layer/tests.rs | 16 +-- .../readonly-layer-files/src/layer.rs | 4 +- test-buildpacks/sbom/src/test_layer.rs | 6 +- test-buildpacks/sbom/src/test_layer_2.rs | 4 +- 11 files changed, 215 insertions(+), 26 deletions(-) create mode 100644 examples/ruby-sample/src/layers/bundler.rs create mode 100644 examples/ruby-sample/src/layers/ruby.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index db3a9478..5e832bca 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- `libcnb`: + - Change Layer interface from `&self` to `&mut self`. ([#669](https://github.com/heroku/libcnb.rs/pull/669)) + ## [0.17.0] - 2023-12-06 diff --git a/examples/execd/src/layer.rs b/examples/execd/src/layer.rs index 6c2fea3f..fbf4a124 100644 --- a/examples/execd/src/layer.rs +++ b/examples/execd/src/layer.rs @@ -21,7 +21,7 @@ impl Layer for ExecDLayer { } fn create( - &self, + &mut self, _context: &BuildContext, _layer_path: &Path, ) -> Result, ::Error> { diff --git a/examples/ruby-sample/src/layers/bundler.rs b/examples/ruby-sample/src/layers/bundler.rs new file mode 100644 index 00000000..49f5d189 --- /dev/null +++ b/examples/ruby-sample/src/layers/bundler.rs @@ -0,0 +1,121 @@ +use crate::RubyBuildpack; +use crate::{util, RubyBuildpackError}; +use libcnb::build::BuildContext; +use libcnb::data::layer_content_metadata::LayerTypes; +use libcnb::layer::{ExistingLayerStrategy, Layer, LayerData, LayerResult, LayerResultBuilder}; +use libcnb::Env; +use serde::Deserialize; +use serde::Serialize; +use std::path::Path; +use std::process::Command; + +#[derive(Deserialize, Serialize, Debug, Clone)] +pub(crate) struct BundlerLayerMetadata { + gemfile_lock_checksum: String, +} + +pub(crate) struct BundlerLayer { + pub ruby_env: Env, +} + +impl Layer for BundlerLayer { + type Buildpack = RubyBuildpack; + type Metadata = BundlerLayerMetadata; + + fn types(&self) -> LayerTypes { + LayerTypes { + build: true, + launch: true, + cache: true, + } + } + + fn create( + &mut self, + context: &BuildContext, + layer_path: &Path, + ) -> Result, RubyBuildpackError> { + println!("---> Installing bundler"); + + util::run_simple_command( + Command::new("gem") + .args(["install", "bundler", "--force"]) + .envs(&self.ruby_env), + RubyBuildpackError::GemInstallBundlerCommandError, + RubyBuildpackError::GemInstallBundlerUnexpectedExitStatus, + )?; + + println!("---> Installing gems"); + + util::run_simple_command( + Command::new("bundle") + .args([ + "install", + "--path", + layer_path.to_str().unwrap(), + "--binstubs", + layer_path.join("bin").to_str().unwrap(), + ]) + .envs(&self.ruby_env), + RubyBuildpackError::BundleInstallCommandError, + RubyBuildpackError::BundleInstallUnexpectedExitStatus, + )?; + + LayerResultBuilder::new(BundlerLayerMetadata { + gemfile_lock_checksum: util::sha256_checksum(context.app_dir.join("Gemfile.lock")) + .map_err(RubyBuildpackError::CouldNotGenerateChecksum)?, + }) + .build() + } + + fn existing_layer_strategy( + &mut self, + context: &BuildContext, + layer: &LayerData, + ) -> Result { + util::sha256_checksum(context.app_dir.join("Gemfile.lock")) + .map_err(RubyBuildpackError::CouldNotGenerateChecksum) + .map(|checksum| { + if checksum == layer.content_metadata.metadata.gemfile_lock_checksum { + ExistingLayerStrategy::Keep + } else { + ExistingLayerStrategy::Update + } + }) + } + + fn update( + &mut self, + context: &BuildContext, + layer: &LayerData, + ) -> Result, RubyBuildpackError> { + println!("---> Reusing gems"); + + util::run_simple_command( + Command::new("bundle") + .args(["config", "--local", "path", layer.path.to_str().unwrap()]) + .envs(&self.ruby_env), + RubyBuildpackError::BundleConfigCommandError, + RubyBuildpackError::BundleConfigUnexpectedExitStatus, + )?; + + util::run_simple_command( + Command::new("bundle") + .args([ + "config", + "--local", + "bin", + layer.path.join("bin").as_path().to_str().unwrap(), + ]) + .envs(&self.ruby_env), + RubyBuildpackError::BundleConfigCommandError, + RubyBuildpackError::BundleConfigUnexpectedExitStatus, + )?; + + LayerResultBuilder::new(BundlerLayerMetadata { + gemfile_lock_checksum: util::sha256_checksum(context.app_dir.join("Gemfile.lock")) + .map_err(RubyBuildpackError::CouldNotGenerateChecksum)?, + }) + .build() + } +} diff --git a/examples/ruby-sample/src/layers/ruby.rs b/examples/ruby-sample/src/layers/ruby.rs new file mode 100644 index 00000000..26ee25fe --- /dev/null +++ b/examples/ruby-sample/src/layers/ruby.rs @@ -0,0 +1,61 @@ +use crate::util; +use crate::{RubyBuildpack, RubyBuildpackError}; +use libcnb::build::BuildContext; +use libcnb::data::layer_content_metadata::LayerTypes; +use libcnb::generic::GenericMetadata; +use libcnb::layer::{Layer, LayerResult, LayerResultBuilder}; +use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope}; +use std::path::Path; +use tempfile::NamedTempFile; + +pub(crate) struct RubyLayer; + +impl Layer for RubyLayer { + type Buildpack = RubyBuildpack; + type Metadata = GenericMetadata; + + fn types(&self) -> LayerTypes { + LayerTypes { + build: true, + launch: true, + cache: false, + } + } + + fn create( + &mut self, + context: &BuildContext, + layer_path: &Path, + ) -> Result, RubyBuildpackError> { + println!("---> Download and extracting Ruby"); + + let ruby_tgz = + NamedTempFile::new().map_err(RubyBuildpackError::CouldNotCreateTemporaryFile)?; + + util::download( + &context.buildpack_descriptor.metadata.ruby_url, + ruby_tgz.path(), + ) + .map_err(RubyBuildpackError::RubyDownloadError)?; + + util::untar(ruby_tgz.path(), layer_path).map_err(RubyBuildpackError::RubyUntarError)?; + + LayerResultBuilder::new(GenericMetadata::default()) + .env( + LayerEnv::new() + .chainable_insert( + Scope::All, + ModificationBehavior::Prepend, + "PATH", + context.app_dir.join(".gem/ruby/2.6.6/bin"), + ) + .chainable_insert( + Scope::All, + ModificationBehavior::Prepend, + "LD_LIBRARY_PATH", + layer_path, + ), + ) + .build() + } +} diff --git a/libcnb/src/build.rs b/libcnb/src/build.rs index 49785511..13d17fae 100644 --- a/libcnb/src/build.rs +++ b/libcnb/src/build.rs @@ -85,7 +85,7 @@ impl BuildContext { /// # } /// # /// fn create( - /// &self, + /// &mut self, /// context: &BuildContext, /// layer_path: &Path, /// ) -> Result, ::Error> { diff --git a/libcnb/src/layer/handling.rs b/libcnb/src/layer/handling.rs index 2a696b7d..0e0d9b71 100644 --- a/libcnb/src/layer/handling.rs +++ b/libcnb/src/layer/handling.rs @@ -21,10 +21,10 @@ use std::path::{Path, PathBuf}; pub(crate) fn handle_layer>( context: &BuildContext, layer_name: LayerName, - layer: L, + mut layer: L, ) -> Result, HandleLayerErrorOrBuildpackError> { match read_layer(&context.layers_dir, &layer_name) { - Ok(None) => handle_create_layer(context, &layer_name, &layer), + Ok(None) => handle_create_layer(context, &layer_name, &mut layer), Ok(Some(layer_data)) => { let existing_layer_strategy = layer .existing_layer_strategy(context, &layer_data) @@ -33,9 +33,11 @@ pub(crate) fn handle_layer>( match existing_layer_strategy { ExistingLayerStrategy::Recreate => { delete_layer(&context.layers_dir, &layer_name)?; - handle_create_layer(context, &layer_name, &layer) + handle_create_layer(context, &layer_name, &mut layer) + } + ExistingLayerStrategy::Update => { + handle_update_layer(context, &layer_data, &mut layer) } - ExistingLayerStrategy::Update => handle_update_layer(context, &layer_data, &layer), ExistingLayerStrategy::Keep => { // We need to rewrite the metadata even if we just want to keep the layer around // since cached layers are restored without their types, causing the layer to be @@ -106,7 +108,7 @@ pub(crate) fn handle_layer>( fn handle_create_layer>( context: &BuildContext, layer_name: &LayerName, - layer: &L, + layer: &mut L, ) -> Result, HandleLayerErrorOrBuildpackError> { let layer_dir = context.layers_dir.join(layer_name.as_str()); @@ -138,7 +140,7 @@ fn handle_create_layer>( fn handle_update_layer>( context: &BuildContext, layer_data: &LayerData, - layer: &L, + layer: &mut L, ) -> Result, HandleLayerErrorOrBuildpackError> { let layer_result = layer .update(context, layer_data) diff --git a/libcnb/src/layer/public_interface.rs b/libcnb/src/layer/public_interface.rs index 658b1610..5f25e72d 100644 --- a/libcnb/src/layer/public_interface.rs +++ b/libcnb/src/layer/public_interface.rs @@ -45,7 +45,7 @@ pub trait Layer { /// # Implementation Requirements /// Implementations **MUST NOT** write to any other location than `layer_path`. fn create( - &self, + &mut self, context: &BuildContext, layer_path: &Path, ) -> Result, ::Error>; @@ -74,7 +74,7 @@ pub trait Layer { /// # Implementation Requirements /// Implementations **MUST NOT** modify the file-system. fn existing_layer_strategy( - &self, + &mut self, context: &BuildContext, layer_data: &LayerData, ) -> Result::Error> { @@ -100,7 +100,7 @@ pub trait Layer { /// # Implementation Requirements /// Implementations **MUST NOT** write to any other location than `layer_path`. fn update( - &self, + &mut self, context: &BuildContext, layer_data: &LayerData, ) -> Result, ::Error> { diff --git a/libcnb/src/layer/tests.rs b/libcnb/src/layer/tests.rs index cbaa170f..81fa4489 100644 --- a/libcnb/src/layer/tests.rs +++ b/libcnb/src/layer/tests.rs @@ -61,7 +61,7 @@ impl Layer for TestLayer { } fn create( - &self, + &mut self, _context: &BuildContext, layer_path: &Path, ) -> Result, ::Error> { @@ -79,7 +79,7 @@ impl Layer for TestLayer { } fn existing_layer_strategy( - &self, + &mut self, _context: &BuildContext, _layer_data: &LayerData, ) -> Result::Error> { @@ -87,7 +87,7 @@ impl Layer for TestLayer { } fn update( - &self, + &mut self, _context: &BuildContext, layer_data: &LayerData, ) -> Result, ::Error> { @@ -749,7 +749,7 @@ fn default_layer_method_implementations() { } fn create( - &self, + &mut self, _context: &BuildContext, _layer_path: &Path, ) -> Result, ::Error> { @@ -770,7 +770,7 @@ fn default_layer_method_implementations() { let temp_dir = tempdir().unwrap(); let context = build_context(&temp_dir); let layer_name = random_layer_name(); - let simple_layer = SimpleLayer; + let mut simple_layer = SimpleLayer; let simple_layer_metadata = SimpleLayerMetadata { field_one: String::from("value one"), @@ -838,7 +838,7 @@ fn layer_env_read_write() { } fn create( - &self, + &mut self, _context: &BuildContext, _layer_path: &Path, ) -> Result, ::Error> { @@ -848,7 +848,7 @@ fn layer_env_read_write() { } fn existing_layer_strategy( - &self, + &mut self, _context: &BuildContext, layer_data: &LayerData, ) -> Result::Error> { @@ -858,7 +858,7 @@ fn layer_env_read_write() { } fn update( - &self, + &mut self, _context: &BuildContext, layer_data: &LayerData, ) -> Result, ::Error> { diff --git a/test-buildpacks/readonly-layer-files/src/layer.rs b/test-buildpacks/readonly-layer-files/src/layer.rs index 0f6b631c..555ff54a 100644 --- a/test-buildpacks/readonly-layer-files/src/layer.rs +++ b/test-buildpacks/readonly-layer-files/src/layer.rs @@ -24,7 +24,7 @@ impl Layer for TestLayer { } fn create( - &self, + &mut self, _context: &BuildContext, layer_path: &Path, ) -> Result, ::Error> { @@ -42,7 +42,7 @@ impl Layer for TestLayer { } fn existing_layer_strategy( - &self, + &mut self, _context: &BuildContext, _layer_data: &LayerData, ) -> Result::Error> { diff --git a/test-buildpacks/sbom/src/test_layer.rs b/test-buildpacks/sbom/src/test_layer.rs index 8ebdde2b..ce4154cb 100644 --- a/test-buildpacks/sbom/src/test_layer.rs +++ b/test-buildpacks/sbom/src/test_layer.rs @@ -22,7 +22,7 @@ impl Layer for TestLayer { } fn create( - &self, + &mut self, _context: &BuildContext, _layer_path: &Path, ) -> Result, ::Error> { @@ -43,7 +43,7 @@ impl Layer for TestLayer { } fn existing_layer_strategy( - &self, + &mut self, _context: &BuildContext, _layer_data: &LayerData, ) -> Result::Error> { @@ -51,7 +51,7 @@ impl Layer for TestLayer { } fn update( - &self, + &mut self, _context: &BuildContext, _layer_data: &LayerData, ) -> Result, ::Error> { diff --git a/test-buildpacks/sbom/src/test_layer_2.rs b/test-buildpacks/sbom/src/test_layer_2.rs index 2ce8bc0f..bf244321 100644 --- a/test-buildpacks/sbom/src/test_layer_2.rs +++ b/test-buildpacks/sbom/src/test_layer_2.rs @@ -22,7 +22,7 @@ impl Layer for TestLayer2 { } fn create( - &self, + &mut self, _context: &BuildContext, _layer_path: &Path, ) -> Result, ::Error> { @@ -43,7 +43,7 @@ impl Layer for TestLayer2 { } fn existing_layer_strategy( - &self, + &mut self, _context: &BuildContext, _layer_data: &LayerData, ) -> Result::Error> {