Fix user binstubs not on PATH and fix PATH order

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/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

@@ -28,6 +28,8 @@ mod user_errors;
 
 #[cfg(test)]
 use libcnb_test as _;
+#[cfg(test)]
+use pretty_assertions as _;
 
 use clap as _;
 

buildpacks/ruby/src/steps/default_env.rs

@@ -38,12 +38,20 @@ pub(crate) fn default_env(
         .to_string();
 
     let layer_ref = context.uncached_layer(
-        layer_name!("env_defaults"),
+        layer_name!("venv"),
         UncachedLayerDefinition {
             build: true,
             launch: true,
         },
     )?;
+    let update_env = LayerEnv::new()
+        .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":")
+        .chainable_insert(
+            Scope::All,
+            ModificationBehavior::Prepend,
+            "PATH",
+            context.app_dir.join("bin"),
+        );
     let env = layer_ref
         .write_env({
             [
@@ -57,7 +65,7 @@ pub(crate) fn default_env(
                 ("DISABLE_SPRING", "1"),
             ]
             .iter()
-            .fold(LayerEnv::new(), |layer_env, (name, value)| {
+            .fold(update_env, |layer_env, (name, value)| {
                 layer_env.chainable_insert(Scope::All, ModificationBehavior::Default, name, value)
             })
         })

buildpacks/ruby/tests/integration_test.rs

@@ -3,10 +3,14 @@
 // Required due to: https://github.com/rust-lang/rust-clippy/issues/11119
 #![allow(clippy::unwrap_used)]
 
+use indoc::{formatdoc, indoc};
 use libcnb_test::{
     assert_contains, assert_contains_match, assert_empty, BuildConfig, BuildpackReference,
     ContainerConfig, ContainerContext, TestRunner,
 };
+use pretty_assertions::assert_eq;
+use std::os::unix::fs::PermissionsExt;
+use std::path::{Path, PathBuf};
 use std::thread;
 use std::time::{Duration, Instant};
 use ureq::Response;
@@ -15,19 +19,18 @@ use ureq::Response;
 // - Cached data "stack" is preserved and will be successfully migrated to "targets"
 #[test]
 #[ignore = "integration test"]
-fn test_migrating_metadata() {
+fn test_migrating_metadata_or_layer_names() {
     // This test is a placeholder for when a change modifies metadata structures.
     // Remove the return and update the `buildpack-ruby` reference to the latest version.
     #![allow(unreachable_code)]
-    // Test v4.0.2 compatible with v4.0.1
-    return;
+    // Test v5.0.1 compatible with v5.0.0
 
     let builder = "heroku/builder:24";
     let app_dir = "tests/fixtures/default_ruby";
 
     TestRunner::default().build(
         BuildConfig::new(builder, app_dir).buildpacks([BuildpackReference::Other(
-            "docker://docker.io/heroku/buildpack-ruby:4.0.1".to_string(),
+            "docker://docker.io/heroku/buildpack-ruby:5.0.0".to_string(),
         )]),
         |context| {
             println!("{}", context.pack_stdout);
@@ -57,8 +60,20 @@ fn test_migrating_metadata() {
 #[test]
 #[ignore = "integration test"]
 fn test_default_app_ubuntu20() {
+    let temp = tempfile::tempdir().unwrap();
+    let app_dir = temp.path();
+
+    copy_dir_all(
+        PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+            .join("tests")
+            .join("fixtures")
+            .join("default_ruby"),
+        app_dir,
+    )
+    .unwrap();
+    let config = BuildConfig::new("heroku/builder:20", app_dir);
     TestRunner::default().build(
-        BuildConfig::new("heroku/builder:20", "tests/fixtures/default_ruby"),
+        config.clone(),
         |context| {
             println!("{}", context.pack_stdout);
             assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack");
@@ -67,6 +82,85 @@ fn test_default_app_ubuntu20() {
                 r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#);
 
             assert_contains!(context.pack_stdout, "Installing puma");
+
+        // Check that at run-time:
+        // - The correct env vars are set.
+        let command_output = context.run_shell_command(
+            indoc! {"
+                set -euo pipefail
+                printenv | sort | grep -vE '(_|HOME|HOSTNAME|OLDPWD|PWD|SHLVL|SECRET_KEY_BASE)='
+
+                # Output command + output to stdout
+                export BASH_XTRACEFD=1; set -o xtrace
+                which -a rake
+                which -a ruby
+            "}
+        );
+        assert_empty!(command_output.stderr);
+        assert_eq!(
+            formatdoc! {"
+                BUNDLE_BIN=/layers/heroku_ruby/gems/bin
+                BUNDLE_CLEAN=1
+                BUNDLE_DEPLOYMENT=1
+                BUNDLE_GEMFILE=/workspace/Gemfile
+                BUNDLE_PATH=/layers/heroku_ruby/gems
+                BUNDLE_WITHOUT=development:test
+                DISABLE_SPRING=1
+                GEM_PATH=/layers/heroku_ruby/gems:/layers/heroku_ruby/bundler
+                JRUBY_OPTS=-Xcompile.invokedynamic=false
+                LD_LIBRARY_PATH=/layers/heroku_ruby/binruby/lib
+                MALLOC_ARENA_MAX=2
+                PATH=/workspace/bin:/layers/heroku_ruby/bundler/bin:/layers/heroku_ruby/gems/bin:/layers/heroku_ruby/bundler/bin:/layers/heroku_ruby/binruby/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin
+                RACK_ENV=production
+                RAILS_ENV=production
+                RAILS_LOG_TO_STDOUT=enabled
+                RAILS_SERVE_STATIC_FILES=enabled
+                + which -a rake
+                /layers/heroku_ruby/gems/bin/rake
+                /layers/heroku_ruby/binruby/bin/rake
+                /usr/bin/rake
+                /bin/rake
+                + which -a ruby
+                /layers/heroku_ruby/binruby/bin/ruby
+                /usr/bin/ruby
+                /bin/ruby
+            "},
+            command_output.stdout,
+        );
+
+        fs_err::create_dir_all(app_dir.join("bin")).unwrap();
+        fs_err::write(app_dir.join("bin").join("rake"), formatdoc!{"
+            #!/usr/bin/env ruby
+            require_relative '../config/boot'
+            require 'rake'
+            Rake.application.run
+        "}).unwrap();
+        chmod_plus_x(&app_dir.join("bin").join("rake")).unwrap();
+
+        context.rebuild(config, |rebuild_context| {
+            println!("{}", rebuild_context.pack_stdout);
+            assert_contains!(rebuild_context.pack_stdout, "Skipping `bundle install` (no changes found in /workspace/Gemfile, /workspace/Gemfile.lock, or user configured environment variables)");
+
+            let command_output = rebuild_context.run_shell_command(
+                indoc! {"
+                    # Output command + output to stdout
+                    export BASH_XTRACEFD=1; set -o xtrace
+                    which -a rake
+                "}
+            );
+            assert_empty!(command_output.stderr);
+            assert_eq!(
+                formatdoc! {"
+                    + which -a rake
+                    /workspace/bin/rake
+                    /layers/heroku_ruby/gems/bin/rake
+                    /layers/heroku_ruby/binruby/bin/rake
+                    /usr/bin/rake
+                    /bin/rake
+                "},
+                command_output.stdout,
+            );
+        });
         },
     );
 }
@@ -318,3 +412,33 @@ fn amd_arm_builder_config(builder_name: &str, app_dir: &str) -> BuildConfig {
     };
     config
 }
+
+/// Sets file permissions on the given path to 7xx (similar to `chmod +x <path>`)
+///
+/// i.e. chmod +x will ensure that the first digit
+/// of the file permission is 7 on unix so if you pass
+/// in 0o455 it would be mutated to 0o755
+fn chmod_plus_x(path: &Path) -> Result<(), std::io::Error> {
+    let mut perms = fs_err::metadata(path)?.permissions();
+    let mut mode = perms.mode();
+    mode |= 0o700;
+    perms.set_mode(mode);
+
+    fs_err::set_permissions(path, perms)
+}
+
+fn copy_dir_all(src: impl AsRef<Path>, dst: impl AsRef<Path>) -> Result<(), std::io::Error> {
+    let src = src.as_ref();
+    let dst = dst.as_ref();
+    fs_err::create_dir_all(dst)?;
+    for entry in fs_err::read_dir(src)? {
+        let entry = entry?;
+        let ty = entry.file_type()?;
+        if ty.is_dir() {
+            copy_dir_all(entry.path(), dst.join(entry.file_name()))?;
+        } else {
+            fs_err::copy(entry.path(), dst.join(entry.file_name()))?;
+        }
+    }
+    Ok(())
+}

commons/CHANGELOG.md

@@ -2,6 +2,12 @@
 
 ### Added
 
+- Introduce `DiffMigrateLayer::cached_layer_rename` and `layer::diff_migrate::LayerRename` (https://github.com/heroku/buildpacks-ruby/pull/383)
+
+## 2024-01-08
+
+### Added
+
 - Introduced `layer::diff_migrate` and `DiffMigrateLayer` for public cache use (https://github.com/heroku/buildpacks-ruby/pull/376)
 
 ### Changed