Fix user binstubs not on PATH and fix PATH order (#383)

* Introduce struct to store rename information

* Add helper to check if a layer exists or not

* Add ability to rename a layer

* Add tests

* Rename `ruby` to `binruby`

Note that this places `/layers/heroku_ruby/gems/bin` before `/layers/heroku_ruby/binruby` on the path. Related to, but doesn't entirely fix #380.

* Changelog

* Assert order of ruby/rake

Using `-a` also shows that there's more than one value present.

* Add failing test for `/workspace/bin` being on the path

* Add pretty assertions for nicer diff output

* Expected before actual

* Put user provided binstub dir in the front of the path

It's common and expected that Rails applications will include a `bin` directory containing "binstubs" of executables their app depends on. For example https://github.com/heroku/ruby-getting-started/tree/5e7ce01610a21cf9e5381daea66f79178e2b3c06/bin. They're largely used to ensure that bundler is invoked/used so that you can run `bin/rails` rather than needing to use `bundle exec rails`. However it's not strictly limited to only that.

This change: Adds the `bin` folder in the root of the workspace to the PATH and changes the layer to `venv` so it is loaded after other layers (and takes precedence in the case of a PATH prepend).

This fixes the previously committed failing test.

Close #380

* Changelog

* Clippy lints

* Integration test for build time PATH

* Clarify that `bundle exec` changes the PATH order

* Fix grammar

* Remove internal `bundle exec` calls

The Application Contract does not specify that commands such as `rake -P` will be called with `bundle exec` and the classic buildpack does not rely on `bundle exec` internally. This brings the CNB closer to parity with the classic buildpack.

In the container environment, the first gems on the PATH should be those installed by the buildpack, negating the strict need to call `bundle exec` as you would on a development machine.

Usually prepending a Ruby command with `bundle exec` will have no discernible difference for an application that's bug free. This is evidenced by all tests passing with this change. However someone can commit their own `bin/rake` or `bin/rails` and we should use this over the executable installed via `bundle install`.

* Make integration failures prettier

* Lint import order

* Move user binstubs to its own layer

At runtime, the alphabetical order of the layer name determines the order it is loaded. At build time, the order that the `env` variable is modified determines the order.

At both build and runtime we want the bin stubs to come first on the PATH when executing user defined code. This was already working for runtime, but wasn't for build time as the "gems" layer was being prepended to the path after the "venv" layer (because the `venv` layer was being defined first, last definition wins).

I originally tried to fix this by defining the PATH inside of the "gems" layer along with the gems path but ran into heroku/libcnb.rs#899. The libcnb.rs project loads the user defined PATH modification last, but I'm unclear if that's spec defined behavior or not https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths.

* Extract layer env modification from method

* Update commons changelog date

buildpacks/ruby/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+### Fixed
+
+- Executables from the applications `bin` directory will be placed on the path before dependencies installed via bundler ([#383](https://github.com/heroku/buildpacks-ruby/pull/383))
+- Binaries from user installed gems will be placed on the path before binaries that ship with Ruby ([#383](https://github.com/heroku/buildpacks-ruby/pull/383))
+
 ## [5.0.0] - 2024-12-17
 
 ### Changed

buildpacks/ruby/Cargo.toml

@@ -34,3 +34,4 @@ cache_diff = { version = "1.0.0", features = ["bullet_stream"] }
 
 [dev-dependencies]
 libcnb-test = "=0.26.1"
+pretty_assertions = "1.4.1"

buildpacks/ruby/src/layers/bundle_download_layer.rs

@@ -33,10 +33,27 @@ pub(crate) fn handle(
         launch: true,
     }
     .cached_layer(layer_name!("bundler"), context, metadata)?;
+
+    let layer_env = LayerEnv::new()
+        .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
+        .chainable_insert(
+            Scope::All,
+            ModificationBehavior::Prepend,
+            "PATH",
+            // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle
+            layer_ref.path().join("bin"),
+        )
+        .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":")
+        .chainable_insert(
+            Scope::All,
+            ModificationBehavior::Prepend,
+            "GEM_PATH", // Bundler is a gem too, allow it to be required
+            layer_ref.path(),
+        );
+    layer_ref.write_env(&layer_env)?;
     match &layer_ref.state {
         LayerState::Restored { cause } => {
             bullet = bullet.sub_bullet(cause);
-            Ok((bullet, layer_ref.read_env()?))
         }
         LayerState::Empty { cause } => {
             match cause {
@@ -46,12 +63,10 @@ pub(crate) fn handle(
                     bullet = bullet.sub_bullet(cause);
                 }
             }
-            let (bullet, layer_env) = download_bundler(bullet, env, metadata, &layer_ref.path())?;
-            layer_ref.write_env(&layer_env)?;
-
-            Ok((bullet, layer_ref.read_env()?))
+            bullet = download_bundler(bullet, env, metadata, &layer_ref.path())?;
         }
     }
+    Ok((bullet, layer_ref.read_env()?))
 }
 
 pub(crate) type Metadata = MetadataV1;
@@ -77,10 +92,9 @@ fn download_bundler(
     bullet: Print<SubBullet<Stdout>>,
     env: &Env,
     metadata: &Metadata,
-    path: &Path,
-) -> Result<(Print<SubBullet<Stdout>>, LayerEnv), RubyBuildpackError> {
-    let bin_dir = path.join("bin");
-    let gem_path = path;
+    gem_path: &Path,
+) -> Result<Print<SubBullet<Stdout>>, RubyBuildpackError> {
+    let bin_dir = gem_path.join("bin");
 
     let mut cmd = Command::new("gem");
     cmd.args(["install", "bundler"]);
@@ -104,23 +118,7 @@ fn download_bundler(
         .map_err(|error| fun_run::map_which_problem(error, cmd.mut_cmd(), env.get("PATH").cloned()))
         .map_err(RubyBuildpackError::GemInstallBundlerCommandError)?;
 
-    let layer_env = LayerEnv::new()
-        .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
-        .chainable_insert(
-            Scope::All,
-            ModificationBehavior::Prepend,
-            "PATH", // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle
-            bin_dir,
-        )
-        .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":")
-        .chainable_insert(
-            Scope::All,
-            ModificationBehavior::Prepend,
-            "GEM_PATH", // Bundler is a gem too, allow it to be required
-            gem_path,
-        );
-
-    Ok((timer.done(), layer_env))
+    Ok(timer.done())
 }
 
 #[cfg(test)]

buildpacks/ruby/src/layers/bundle_install_layer.rs

@@ -322,9 +322,9 @@ fn display_name(cmd: &mut Command, env: &Env) -> String {
 
 #[cfg(test)]
 mod test {
-    use bullet_stream::strip_ansi;
-
     use super::*;
+    use bullet_stream::strip_ansi;
+    use pretty_assertions::assert_eq;
     use std::path::PathBuf;
 
     /// `CacheDiff` logic controls cache invalidation

buildpacks/ruby/src/layers/metrics_agent_install.rs

@@ -152,10 +152,10 @@ fn write_execd_script(
     fs_err::write(
         &execd,
         format!(
-            r#"#!/usr/bin/env bash
+            r"#!/usr/bin/env bash
 
                {daemon} --log {log} --loop-path {run_loop} --agentmon {agentmon}
-              "#,
+              ",
             log = log.display(),
             daemon = daemon.display(),
             run_loop = run_loop.display(),

buildpacks/ruby/src/layers/ruby_install_layer.rs

@@ -20,7 +20,7 @@ use bullet_stream::state::SubBullet;
 use bullet_stream::Print;
 use cache_diff::CacheDiff;
 use commons::gemfile_lock::ResolvedRubyVersion;
-use commons::layer::diff_migrate::DiffMigrateLayer;
+use commons::layer::diff_migrate::{DiffMigrateLayer, LayerRename};
 use flate2::read::GzDecoder;
 use libcnb::data::layer_name;
 use libcnb::layer::{EmptyLayerCause, LayerState};
@@ -42,7 +42,14 @@ pub(crate) fn handle(
         build: true,
         launch: true,
     }
-    .cached_layer(layer_name!("ruby"), context, metadata)?;
+    .cached_layer_rename(
+        LayerRename {
+            to: layer_name!("binruby"),
+            from: vec![layer_name!("ruby")],
+        },
+        context,
+        metadata,
+    )?;
     match &layer_ref.state {
         LayerState::Restored { cause } => {
             bullet = bullet.sub_bullet(cause);

buildpacks/ruby/src/main.rs

@@ -11,9 +11,11 @@ use layers::{
 use libcnb::build::{BuildContext, BuildResult, BuildResultBuilder};
 use libcnb::data::build_plan::BuildPlanBuilder;
 use libcnb::data::launch::LaunchBuilder;
+use libcnb::data::layer_name;
 use libcnb::detect::{DetectContext, DetectResult, DetectResultBuilder};
 use libcnb::generic::{GenericMetadata, GenericPlatform};
-use libcnb::layer_env::Scope;
+use libcnb::layer::UncachedLayerDefinition;
+use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
 use libcnb::Platform;
 use libcnb::{buildpack_main, Buildpack};
 use std::io::stdout;
@@ -28,6 +30,8 @@ mod user_errors;
 
 #[cfg(test)]
 use libcnb_test as _;
+#[cfg(test)]
+use pretty_assertions as _;
 
 use clap as _;
 
@@ -218,6 +222,28 @@ impl Buildpack for RubyBuildpack {
             (bullet.done(), layer_env.apply(Scope::Build, &env))
         };
 
+        env = {
+            let user_binstubs = context.uncached_layer(
+                layer_name!("user_binstubs"),
+                UncachedLayerDefinition {
+                    build: true,
+                    launch: true,
+                },
+            )?;
+            user_binstubs.write_env(
+                LayerEnv::new()
+                    .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
+                    .chainable_insert(
+                        Scope::All,
+                        ModificationBehavior::Prepend,
+                        "PATH",
+                        context.app_dir.join("bin"),
+                    ),
+            )?;
+
+            user_binstubs.read_env()?.apply(Scope::Build, &env)
+        };
+
         // ## Detect gems
         let (mut build_output, gem_list, default_process) = {
             let bullet = build_output.bullet("Default process detection");

buildpacks/ruby/src/rake_task_detect.rs

@@ -29,10 +29,8 @@ pub(crate) fn call<T: IntoIterator<Item = (K, V)>, K: AsRef<OsStr>, V: AsRef<OsS
     envs: T,
     error_on_failure: bool,
 ) -> Result<(Print<SubBullet<Stdout>>, RakeDetect), CmdError> {
-    let mut cmd = Command::new("bundle");
-    cmd.args(["exec", "rake", "-P", "--trace"])
-        .env_clear()
-        .envs(envs);
+    let mut cmd = Command::new("rake");
+    cmd.args(["-P", "--trace"]).env_clear().envs(envs);
 
     let timer = bullet.start_timer(format!("Running {}", style::command(cmd.name())));
     let output = cmd.named_output().or_else(|error| {

buildpacks/ruby/src/steps/rake_assets_install.rs

@@ -20,7 +20,7 @@ pub(crate) fn rake_assets_install(
     let cases = asset_cases(rake_detect);
     let rake_assets_precompile = style::value("rake assets:precompile");
     let rake_assets_clean = style::value("rake assets:clean");
-    let rake_detect_cmd = style::value("bundle exec rake -P");
+    let rake_detect_cmd = style::value("rake -P");
 
     match cases {
         AssetCases::None => {
@@ -33,8 +33,8 @@ pub(crate) fn rake_assets_install(
                 format!("Compiling assets without cache (Clean task not found via {rake_detect_cmd})"),
             ).sub_bullet(format!("{help} Enable caching by ensuring {rake_assets_clean} is present when running the detect command locally"));
 
-            let mut cmd = Command::new("bundle");
-            cmd.args(["exec", "rake", "assets:precompile", "--trace"])
+            let mut cmd = Command::new("rake");
+            cmd.args(["assets:precompile", "--trace"])
                 .env_clear()
                 .envs(env);
 
@@ -79,16 +79,10 @@ pub(crate) fn rake_assets_install(
                 });
             }
 
-            let mut cmd = Command::new("bundle");
-            cmd.args([
-                "exec",
-                "rake",
-                "assets:precompile",
-                "assets:clean",
-                "--trace",
-            ])
-            .env_clear()
-            .envs(env);
+            let mut cmd = Command::new("rake");
+            cmd.args(["assets:precompile", "assets:clean", "--trace"])
+                .env_clear()
+                .envs(env);
 
             bullet
                 .stream_with(