Change Layer interface from &self to &mut self

This Commit is an experiment to see the effects of moving the interface for `Layer` to be mutable. 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.

## Alternatives


### Alternatives: Consuming interface

Originally, I wanted to make these consuming interfaces, but to do that, we would have to change to taking a `Box<Self>` as you cannot use `Self` directly in a trait:

```
    fn create(
        self: Box<Self>,
        context: &BuildContext<Self::Buildpack>,
        layer_path: &Path,
    ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error>;

```

We could explore going down that path. This change was easier to make to start the discussion.

### Alternatives: Mutable and immutable layer traits

Provide a `Layer` and `MutLayer` trait to let developers pick what they want. You would also need a `context.handle_mut_layer` function as well.

It's nice in that it gives devs the flexibility to choose whether they need that mutability. They could start with `Layer` and change to `MutLayer` only if needed.

The downside is that maintaining duplicate logic that implements both would be annoying. There's no good way to express "this could be mutable, or not, I don't care which" To Rust.


### Alternatives: Exploded layer API

The current layer implementation works as a state machine. You put state in (values in the struct that implements Layer). Libcnb uses that input along with the prior run's metadata, and the existing_layer_strategy is used to determine the cache state (keep or clear) and which function should be called (create or update).

For workflows suited to this "simple input equals simple output," it works well. In other cases, where the developer might be doing something surprising or unusual (example, https://github.com/heroku/buildpacks-ruby/blob/c9ee5ff29890d30f2ccb23f850ed5274c11b06de/buildpacks/ruby/src/layers/bundle_install_layer.rs#L192-L198) it requires the developer to squeeze their use case into the state machine concept. A mutable logger is one such concept that is currently impossible to represent using the existing layer signature.

When I wrote a competing CNB library in Ruby, I didn't try to bundle metadata, caching logic, disk state, and env vars together in one thing. Instead, I separated them to be acted on individually. I'm linking them here for reference. I do NOT suggest we try to duplicate these interfaces:

- Metadata: https://github.com/heroku/buildpacks-ruby/blob/22d489df6ad59c82220e9e3801bbb399afd78882/lib/heroku_buildpack_ruby/metadata/cnb.rb
- Env: https://github.com/heroku/buildpacks-ruby/blob/22d489df6ad59c82220e9e3801bbb399afd78882/lib/heroku_buildpack_ruby/env_proxy/default.rb

This approach was flawed and could be better. However, it presents what an inverted or exploded layer API could look like. Instead of trying to provide a transactional state machine view into a layer, we could expose primitives and let users build their own. Or possibly provide several different approaches to working with layers:

- Individual functions (i.e., inverted/exploded API)
- Current state machine Layer interface
- Some other access patterns we've yet to explore.

For that last bullet point, we should invest in more specific and opinionated solutions to individual use cases instead of a singular pattern. One example is cache invalidation. I've said for a long time that a proc macro that compares structs would be useful for general cache invalidation. It doesn't even have to be tied to CNB cache invalidation.

Ultimately, I eventually want more than what we've currently got. I've found ways to hack around some limitations and provide varying interfaces:

- Pre-built Layer for ONLY setting env vars: https://github.com/heroku/buildpacks-ruby/blob/c9ee5ff29890d30f2ccb23f850ed5274c11b06de/commons/src/layer/configure_env_layer.rs
- Exploded "give me a path" Layer for use in caching public assets https://github.com/heroku/buildpacks-ruby/blob/c9ee5ff29890d30f2ccb23f850ed5274c11b06de/commons/src/cache/in_app_dir_cache_layer.rs

I've not made a PR using an alternative Layer API because I've not managed to produce one that I like, and others might want something different. I don't know if anyone has been able to produce a good sketch of what that "something different" could look like.

## Implementation and analysis

I changed the `layer/public_interface.rs` and then fixed the cascading failures. It was very straightforward, and there was no change in logic needed (just changes in mutability). It's not a backward-compatible change, but the update will be easy.

CHANGELOG.md

@@ -21,6 +21,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - `libcnb-cargo`:
   - No longer outputs paths for non-libcnb.rs and non-meta buildpacks. ([#657](https://github.com/heroku/libcnb.rs/pull/657))
   - Build output for humans changed slightly, output intended for machines/scripting didn't change. ([#657](https://github.com/heroku/libcnb.rs/pull/657))
+- `libcnb`:
+  - The `Layer` trait interface now takes a mutable reference. Buildpack authors will need to change `&self` to `&mut self` for all their layers. ([#669](https://github.com/heroku/libcnb.rs/pull/669))
 
 ## [0.14.0] - 2023-08-18
 

examples/execd/src/layer.rs

@@ -21,7 +21,7 @@ impl Layer for ExecDLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {

examples/ruby-sample/src/layers/bundler.rs

@@ -31,7 +31,7 @@ impl Layer for BundlerLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         context: &BuildContext<Self::Buildpack>,
         layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, RubyBuildpackError> {
@@ -69,7 +69,7 @@ impl Layer for BundlerLayer {
     }
 
     fn existing_layer_strategy(
-        &self,
+        &mut self,
         context: &BuildContext<Self::Buildpack>,
         layer: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, RubyBuildpackError> {
@@ -85,7 +85,7 @@ impl Layer for BundlerLayer {
     }
 
     fn update(
-        &self,
+        &mut self,
         context: &BuildContext<Self::Buildpack>,
         layer: &LayerData<Self::Metadata>,
     ) -> Result<LayerResult<Self::Metadata>, RubyBuildpackError> {

examples/ruby-sample/src/layers/ruby.rs

@@ -23,7 +23,7 @@ impl Layer for RubyLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         context: &BuildContext<Self::Buildpack>,
         layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, RubyBuildpackError> {

libcnb/src/build.rs

@@ -85,7 +85,7 @@ impl<B: Buildpack + ?Sized> BuildContext<B> {
     /// #    }
     /// #
     ///     fn create(
-    ///         &self,
+    ///         &mut self,
     ///         context: &BuildContext<Self::Buildpack>,
     ///         layer_path: &Path,
     ///     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {

libcnb/src/layer/handling.rs

@@ -21,10 +21,10 @@ use std::path::{Path, PathBuf};
 pub(crate) fn handle_layer<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
     context: &BuildContext<B>,
     layer_name: LayerName,
-    layer: L,
+    mut layer: L,
 ) -> Result<LayerData<L::Metadata>, HandleLayerErrorOrBuildpackError<B::Error>> {
     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<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
             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<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
 fn handle_create_layer<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
     context: &BuildContext<B>,
     layer_name: &LayerName,
-    layer: &L,
+    layer: &mut L,
 ) -> Result<LayerData<L::Metadata>, HandleLayerErrorOrBuildpackError<B::Error>> {
     let layer_dir = context.layers_dir.join(layer_name.as_str());
 
@@ -138,7 +140,7 @@ fn handle_create_layer<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
 fn handle_update_layer<B: Buildpack + ?Sized, L: Layer<Buildpack = B>>(
     context: &BuildContext<B>,
     layer_data: &LayerData<L::Metadata>,
-    layer: &L,
+    layer: &mut L,
 ) -> Result<LayerData<L::Metadata>, HandleLayerErrorOrBuildpackError<B::Error>> {
     let layer_result = layer
         .update(context, layer_data)

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<Self::Buildpack>,
         layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::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<Self::Buildpack>,
         layer_data: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::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<Self::Buildpack>,
         layer_data: &LayerData<Self::Metadata>,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {

libcnb/src/layer/tests.rs

@@ -61,7 +61,7 @@ impl Layer for TestLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -79,15 +79,15 @@ impl Layer for TestLayer {
     }
 
     fn existing_layer_strategy(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_data: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {
         Ok(self.existing_layer_strategy)
     }
 
     fn update(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         layer_data: &LayerData<Self::Metadata>,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -749,7 +749,7 @@ fn default_layer_method_implementations() {
         }
 
         fn create(
-            &self,
+            &mut self,
             _context: &BuildContext<Self::Buildpack>,
             _layer_path: &Path,
         ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::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"),
@@ -837,7 +837,7 @@ fn layer_env_read_write() {
         }
 
         fn create(
-            &self,
+            &mut self,
             _context: &BuildContext<Self::Buildpack>,
             _layer_path: &Path,
         ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -847,7 +847,7 @@ fn layer_env_read_write() {
         }
 
         fn existing_layer_strategy(
-            &self,
+            &mut self,
             _context: &BuildContext<Self::Buildpack>,
             layer_data: &LayerData<Self::Metadata>,
         ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {
@@ -857,7 +857,7 @@ fn layer_env_read_write() {
         }
 
         fn update(
-            &self,
+            &mut self,
             _context: &BuildContext<Self::Buildpack>,
             layer_data: &LayerData<Self::Metadata>,
         ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {

test-buildpacks/readonly-layer-files/src/layer.rs

@@ -24,7 +24,7 @@ impl Layer for TestLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -42,7 +42,7 @@ impl Layer for TestLayer {
     }
 
     fn existing_layer_strategy(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_data: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {

test-buildpacks/sbom/src/test_layer.rs

@@ -22,7 +22,7 @@ impl Layer for TestLayer {
     }
 
     fn create(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -43,15 +43,15 @@ impl Layer for TestLayer {
     }
 
     fn existing_layer_strategy(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_data: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {
         Ok(ExistingLayerStrategy::Update)
     }
 
     fn update(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_data: &LayerData<Self::Metadata>,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {

test-buildpacks/sbom/src/test_layer_2.rs

@@ -22,7 +22,7 @@ impl Layer for TestLayer2 {
     }
 
     fn create(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_path: &Path,
     ) -> Result<LayerResult<Self::Metadata>, <Self::Buildpack as Buildpack>::Error> {
@@ -43,7 +43,7 @@ impl Layer for TestLayer2 {
     }
 
     fn existing_layer_strategy(
-        &self,
+        &mut self,
         _context: &BuildContext<Self::Buildpack>,
         _layer_data: &LayerData<Self::Metadata>,
     ) -> Result<ExistingLayerStrategy, <Self::Buildpack as Buildpack>::Error> {