From fd53d508e38bbead8e69bfcea191858a6a732418 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 12:11:47 -0600 Subject: [PATCH 01/23] Introduce struct to store rename information --- commons/src/layer/diff_migrate.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index 4476f6fd..20fd5dc2 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -130,6 +130,15 @@ impl DiffMigrateLayer { } } +/// Represents when we want to move contents from one (or more) layer names +/// +pub struct LayerRename { + /// The desired layer name + pub to: LayerName, + /// A list of prior, possibly layer names + pub from: Vec, +} + /// Standardizes formatting for layer cache clearing behavior /// /// If the diff is empty, there are no changes and the layer is kept and the old data is returned From 5aa139d14368b1858b00a72c71780252b31cf441 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 12:35:16 -0600 Subject: [PATCH 02/23] Add helper to check if a layer exists or not --- commons/src/layer/diff_migrate.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index 20fd5dc2..77e7fb0d 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -44,12 +44,16 @@ use crate::display::SentenceList; use cache_diff::CacheDiff; +use fs_err::PathExt; use libcnb::build::BuildContext; use libcnb::data::layer::LayerName; -use libcnb::layer::{CachedLayerDefinition, InvalidMetadataAction, LayerRef, RestoredLayerAction}; +use libcnb::layer::{ + CachedLayerDefinition, InvalidMetadataAction, LayerError, LayerRef, RestoredLayerAction, +}; use magic_migrate::TryMigrate; use serde::ser::Serialize; use std::fmt::Debug; +use std::path::PathBuf; #[cfg(test)] use bullet_stream as _; @@ -139,6 +143,21 @@ pub struct LayerRename { pub from: Vec, } +/// Returns Some(PathBuf) when the layer exists on disk +fn is_layer_on_disk( + layer_name: &LayerName, + context: &BuildContext, +) -> libcnb::Result, B::Error> +where + B: libcnb::Buildpack, +{ + let path = context.layers_dir.join(layer_name.as_str()); + + path.fs_err_try_exists() + .map_err(|error| libcnb::Error::LayerError(LayerError::IoError(error))) + .map(|exists| exists.then_some(path)) +} + /// Standardizes formatting for layer cache clearing behavior /// /// If the diff is empty, there are no changes and the layer is kept and the old data is returned From 734485d60cd8ad2db2c3aa1a3f837659077b4f3d Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 13:06:19 -0600 Subject: [PATCH 03/23] Add ability to rename a layer --- commons/src/layer/diff_migrate.rs | 51 +++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index 77e7fb0d..06e8911b 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -132,6 +132,57 @@ impl DiffMigrateLayer { layer_ref.write_metadata(metadata)?; Ok(layer_ref) } + + /// Renames cached layer while writing metadata to a layer + /// + /// When given a prior [`LayerRename::from`] that exists, but the [`LayerRename::to`] + /// does not, then the contents of the prior layer will be copied before being deleted. + /// + /// After that this function callse [`cached_layer`] on the new layer. + /// + /// # Panics + /// + /// This function should not panic unless there's an internal bug. + /// + /// # Errors + /// + /// Returns an error if libcnb cannot read or write the metadata. Or if + /// there's an error while copying from one path to another. + pub fn cached_layer_rename( + self, + layer_rename: LayerRename, + context: &BuildContext, + metadata: &M, + ) -> libcnb::Result, Meta>, B::Error> + where + B: libcnb::Buildpack, + M: CacheDiff + TryMigrate + Serialize + Debug + Clone, + { + let LayerRename { + to: to_layer, + from: prior_layers, + } = layer_rename; + + if let (Some(prior_dir), None) = ( + prior_layers + .iter() + .map(|layer_name| is_layer_on_disk(layer_name, context)) + .collect::>, _>>()? + .iter() + .find_map(std::borrow::ToOwned::to_owned), + is_layer_on_disk(&to_layer, context)?, + ) { + let to_dir = context.layers_dir.join(to_layer.as_str()); + std::fs::create_dir_all(&to_dir).map_err(LayerError::IoError)?; + std::fs::rename(&prior_dir, &to_dir).map_err(LayerError::IoError)?; + std::fs::rename( + prior_dir.with_extension("toml"), + to_dir.with_extension("toml"), + ) + .map_err(LayerError::IoError)?; + } + self.cached_layer(to_layer, context, metadata) + } } /// Represents when we want to move contents from one (or more) layer names From 8d46c1eee24ef00e8579df7c5191338e7ec45457 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 13:31:49 -0600 Subject: [PATCH 04/23] Add tests --- commons/src/layer/diff_migrate.rs | 94 +++++++++++++++++++++++++++++++ 1 file changed, 94 insertions(+) diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index 06e8911b..aaf398c7 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -360,6 +360,100 @@ mod tests { } } + #[test] + fn test_migrate_layer_name_works_if_prior_dir_does_not_exist() { + let temp = tempfile::tempdir().unwrap(); + let context = temp_build_context::( + temp.path(), + include_str!("../../../buildpacks/ruby/buildpack.toml"), + ); + + let result = DiffMigrateLayer { + build: true, + launch: true, + } + .cached_layer_rename( + LayerRename { + to: layer_name!("new"), + from: vec![layer_name!("does_not_exist")], + }, + &context, + &TestMetadata { + value: "hello".to_string(), + }, + ) + .unwrap(); + + assert!(matches!(result.state, LayerState::Empty { cause: _ })); + } + + #[test] + fn test_migrate_layer_name_copies_old_data() { + let temp = tempfile::tempdir().unwrap(); + let old_layer_name = layer_name!("old"); + let new_layer_name = layer_name!("new"); + let context = temp_build_context::( + temp.path(), + include_str!("../../../buildpacks/ruby/buildpack.toml"), + ); + + // First write + let result = DiffMigrateLayer { + build: true, + launch: true, + } + .cached_layer( + old_layer_name.clone(), + &context, + &TestMetadata { + value: "hello".to_string(), + }, + ) + .unwrap(); + + assert!(matches!( + result.state, + LayerState::Empty { + cause: EmptyLayerCause::NewlyCreated + } + )); + + assert!(context + .layers_dir + .join(old_layer_name.as_str()) + .fs_err_try_exists() + .unwrap()); + + assert!(!context + .layers_dir + .join(new_layer_name.as_str()) + .fs_err_try_exists() + .unwrap()); + + let result = DiffMigrateLayer { + build: true, + launch: true, + } + .cached_layer_rename( + LayerRename { + to: new_layer_name.clone(), + from: vec![old_layer_name], + }, + &context, + &TestMetadata { + value: "hello".to_string(), + }, + ) + .unwrap(); + + assert!(matches!(result.state, LayerState::Restored { cause: _ })); + assert!(context + .layers_dir + .join(new_layer_name.as_str()) + .fs_err_try_exists() + .unwrap()); + } + #[test] fn test_diff_migrate() { let temp = tempfile::tempdir().unwrap(); From 5674b00f1aa3129af0dea5714ae8f3cc82cf3e98 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 13:47:22 -0600 Subject: [PATCH 05/23] 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. --- .../ruby/src/layers/ruby_install_layer.rs | 11 +++++- buildpacks/ruby/tests/integration_test.rs | 39 +++++++++++++++++-- 2 files changed, 44 insertions(+), 6 deletions(-) diff --git a/buildpacks/ruby/src/layers/ruby_install_layer.rs b/buildpacks/ruby/src/layers/ruby_install_layer.rs index aad8b1f5..80dd7b46 100644 --- a/buildpacks/ruby/src/layers/ruby_install_layer.rs +++ b/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); diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index 1d05d524..eb509ffc 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -3,6 +3,7 @@ // 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, @@ -15,19 +16,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); @@ -67,6 +67,37 @@ 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)=' + "} + ); + assert_empty!(command_output.stderr); + assert_eq!( + command_output.stdout, + 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=/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 + "} + ); }, ); } From a91b1bfc92ef3a560f26612609c0145d6d7857b9 Mon Sep 17 00:00:00 2001 From: Schneems Date: Wed, 8 Jan 2025 14:03:08 -0600 Subject: [PATCH 06/23] Changelog --- buildpacks/ruby/CHANGELOG.md | 4 ++++ commons/CHANGELOG.md | 6 ++++++ docs/application_contract.md | 1 + 3 files changed, 11 insertions(+) diff --git a/buildpacks/ruby/CHANGELOG.md b/buildpacks/ruby/CHANGELOG.md index 3c0b9d7e..ad2f757d 100644 --- a/buildpacks/ruby/CHANGELOG.md +++ b/buildpacks/ruby/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- Binaries from user installed gems will be placed on the path before binaries that ship with Ruby ([#382](https://github.com/heroku/buildpacks-ruby/pull/382)) + ## [5.0.0] - 2024-12-17 ### Changed diff --git a/commons/CHANGELOG.md b/commons/CHANGELOG.md index dfa3109e..869e35f2 100644 --- a/commons/CHANGELOG.md +++ b/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/382) + +## 2024-01-08 + +### Added + - Introduced `layer::diff_migrate` and `DiffMigrateLayer` for public cache use (https://github.com/heroku/buildpacks-ruby/pull/376) ### Changed diff --git a/docs/application_contract.md b/docs/application_contract.md index effac1da..c263f374 100644 --- a/docs/application_contract.md +++ b/docs/application_contract.md @@ -80,5 +80,6 @@ Once an application has passed the detect phase, the build phase will execute to - `GEM_PATH=` - Tells Ruby where gems are located. - `MALLOC_ARENA_MAX=2` - Controls glibc memory allocation behavior with the goal of decreasing overall memory allocated by Ruby [details](https://devcenter.heroku.com/changelog-items/1683). - `PATH` - Various executables are installed and the `PATH` env var will be modified so they can be executed at the system level. This is mostly done via interfaces provided by `libcnb` and CNB layers rather than directly. + - Binaries from gems will take precedence over binaries that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that come with the compiled Ruby binary). - `RAILS_LOG_TO_STDOUT="enabled"` - Sets the default logging target to STDOUT for Rails 5+ apps. [details](https://blog.heroku.com/container_ready_rails_5) - `RAILS_SERVE_STATIC_FILES="enabled"` - Enables the `ActionDispatch::Static` middleware for Rails 5+ apps so that static files such as those in `public/assets` are served by the Ruby webserver such as Puma [details](https://blog.heroku.com/container_ready_rails_5). From a33dcbd8bf0c9cdd7770238689107f6983473764 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 14:01:42 -0600 Subject: [PATCH 07/23] Assert order of ruby/rake Using `-a` also shows that there's more than one value present. --- buildpacks/ruby/tests/integration_test.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index eb509ffc..dd5f1c94 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -74,6 +74,11 @@ fn test_default_app_ubuntu20() { 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); @@ -96,6 +101,15 @@ fn test_default_app_ubuntu20() { 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 "} ); }, From c378e9d547251c38867bf819f779ee73f72ad223 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 14:57:21 -0600 Subject: [PATCH 08/23] Add failing test for `/workspace/bin` being on the path --- buildpacks/ruby/tests/integration_test.rs | 82 ++++++++++++++++++++++- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index dd5f1c94..9028d214 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -8,6 +8,8 @@ use libcnb_test::{ assert_contains, assert_contains_match, assert_empty, BuildConfig, BuildpackReference, ContainerConfig, ContainerContext, TestRunner, }; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; use std::thread; use std::time::{Duration, Instant}; use ureq::Response; @@ -57,8 +59,20 @@ fn test_migrating_metadata_or_layer_names() { #[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"); @@ -96,7 +110,7 @@ fn test_default_app_ubuntu20() { JRUBY_OPTS=-Xcompile.invokedynamic=false LD_LIBRARY_PATH=/layers/heroku_ruby/binruby/lib MALLOC_ARENA_MAX=2 - PATH=/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 + 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 @@ -112,6 +126,40 @@ fn test_default_app_ubuntu20() { /bin/ruby "} ); + + 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!( + command_output.stdout, + formatdoc! {" + + which -a rake + /workspace/bin/rake + /layers/heroku_ruby/gems/bin/rake + /layers/heroku_ruby/binruby/bin/rake + /usr/bin/rake + /bin/rake + "} + ); + }); }, ); } @@ -363,3 +411,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 `) +/// +/// 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, dst: impl AsRef) -> 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(()) +} From 90b16fc639c0036e618ae50a76cfb50a0ff7bff3 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 15:05:44 -0600 Subject: [PATCH 09/23] Add pretty assertions for nicer diff output --- Cargo.lock | 1 + buildpacks/ruby/Cargo.toml | 1 + buildpacks/ruby/tests/integration_test.rs | 1 + 3 files changed, 3 insertions(+) diff --git a/Cargo.lock b/Cargo.lock index fb0687db..609c89da 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -608,6 +608,7 @@ dependencies = [ "libcnb-test", "libherokubuildpack", "magic_migrate", + "pretty_assertions", "rand", "regex", "serde", diff --git a/buildpacks/ruby/Cargo.toml b/buildpacks/ruby/Cargo.toml index 98ee266d..372f9590 100644 --- a/buildpacks/ruby/Cargo.toml +++ b/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" diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index 9028d214..aa10b451 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -8,6 +8,7 @@ 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; From 3d36826dfcc23f6829f296f9138d01985b5a2adb Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 15:20:49 -0600 Subject: [PATCH 10/23] Expected before actual --- buildpacks/ruby/tests/integration_test.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index aa10b451..efb092af 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -98,7 +98,6 @@ fn test_default_app_ubuntu20() { ); assert_empty!(command_output.stderr); assert_eq!( - command_output.stdout, formatdoc! {" BUNDLE_BIN=/layers/heroku_ruby/gems/bin BUNDLE_CLEAN=1 @@ -125,7 +124,8 @@ fn test_default_app_ubuntu20() { /layers/heroku_ruby/binruby/bin/ruby /usr/bin/ruby /bin/ruby - "} + "}, + command_output.stdout, ); fs_err::create_dir_all(app_dir.join("bin")).unwrap(); @@ -150,7 +150,6 @@ fn test_default_app_ubuntu20() { ); assert_empty!(command_output.stderr); assert_eq!( - command_output.stdout, formatdoc! {" + which -a rake /workspace/bin/rake @@ -158,7 +157,8 @@ fn test_default_app_ubuntu20() { /layers/heroku_ruby/binruby/bin/rake /usr/bin/rake /bin/rake - "} + "}, + command_output.stdout, ); }); }, From 0fe9ee290030c3a944e0a09eaab99540f6840375 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 15:27:45 -0600 Subject: [PATCH 11/23] 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 --- buildpacks/ruby/src/steps/default_env.rs | 12 ++++++++++-- docs/application_contract.md | 3 ++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/buildpacks/ruby/src/steps/default_env.rs b/buildpacks/ruby/src/steps/default_env.rs index d8f3849e..1b66a358 100644 --- a/buildpacks/ruby/src/steps/default_env.rs +++ b/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) }) }) diff --git a/docs/application_contract.md b/docs/application_contract.md index c263f374..4c8c6bba 100644 --- a/docs/application_contract.md +++ b/docs/application_contract.md @@ -80,6 +80,7 @@ Once an application has passed the detect phase, the build phase will execute to - `GEM_PATH=` - Tells Ruby where gems are located. - `MALLOC_ARENA_MAX=2` - Controls glibc memory allocation behavior with the goal of decreasing overall memory allocated by Ruby [details](https://devcenter.heroku.com/changelog-items/1683). - `PATH` - Various executables are installed and the `PATH` env var will be modified so they can be executed at the system level. This is mostly done via interfaces provided by `libcnb` and CNB layers rather than directly. - - Binaries from gems will take precedence over binaries that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that come with the compiled Ruby binary). + - Executables in the application `bin` directory will take precedence over gem installed executables. + - Executables from gems will take precedence over executables that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that come with the compiled Ruby binary). - `RAILS_LOG_TO_STDOUT="enabled"` - Sets the default logging target to STDOUT for Rails 5+ apps. [details](https://blog.heroku.com/container_ready_rails_5) - `RAILS_SERVE_STATIC_FILES="enabled"` - Enables the `ActionDispatch::Static` middleware for Rails 5+ apps so that static files such as those in `public/assets` are served by the Ruby webserver such as Puma [details](https://blog.heroku.com/container_ready_rails_5). From 3ed9c1763e0dffb88e7f6e9046d47c68a36fcd95 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 15:31:07 -0600 Subject: [PATCH 12/23] Changelog --- buildpacks/ruby/CHANGELOG.md | 3 ++- commons/CHANGELOG.md | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/buildpacks/ruby/CHANGELOG.md b/buildpacks/ruby/CHANGELOG.md index ad2f757d..bdd72d76 100644 --- a/buildpacks/ruby/CHANGELOG.md +++ b/buildpacks/ruby/CHANGELOG.md @@ -9,7 +9,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- Binaries from user installed gems will be placed on the path before binaries that ship with Ruby ([#382](https://github.com/heroku/buildpacks-ruby/pull/382)) +- 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 diff --git a/commons/CHANGELOG.md b/commons/CHANGELOG.md index 869e35f2..e046228c 100644 --- a/commons/CHANGELOG.md +++ b/commons/CHANGELOG.md @@ -2,7 +2,7 @@ ### Added -- Introduce `DiffMigrateLayer::cached_layer_rename` and `layer::diff_migrate::LayerRename` (https://github.com/heroku/buildpacks-ruby/pull/382) +- Introduce `DiffMigrateLayer::cached_layer_rename` and `layer::diff_migrate::LayerRename` (https://github.com/heroku/buildpacks-ruby/pull/383) ## 2024-01-08 From 2bec8ef5f6715114e50ece68976cf12fb16a5985 Mon Sep 17 00:00:00 2001 From: Schneems Date: Thu, 9 Jan 2025 15:35:43 -0600 Subject: [PATCH 13/23] Clippy lints --- buildpacks/ruby/src/layers/metrics_agent_install.rs | 4 ++-- buildpacks/ruby/src/main.rs | 2 ++ buildpacks/ruby/tests/integration_test.rs | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) diff --git a/buildpacks/ruby/src/layers/metrics_agent_install.rs b/buildpacks/ruby/src/layers/metrics_agent_install.rs index 2c289194..9ced9209 100644 --- a/buildpacks/ruby/src/layers/metrics_agent_install.rs +++ b/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(), diff --git a/buildpacks/ruby/src/main.rs b/buildpacks/ruby/src/main.rs index 4663e905..0717d357 100644 --- a/buildpacks/ruby/src/main.rs +++ b/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 _; diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index efb092af..0860be92 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -430,7 +430,7 @@ fn chmod_plus_x(path: &Path) -> Result<(), std::io::Error> { fn copy_dir_all(src: impl AsRef, dst: impl AsRef) -> Result<(), std::io::Error> { let src = src.as_ref(); let dst = dst.as_ref(); - fs_err::create_dir_all(&dst)?; + fs_err::create_dir_all(dst)?; for entry in fs_err::read_dir(src)? { let entry = entry?; let ty = entry.file_type()?; From b9fd9c493ee6ce35661e8caaa2afafbc41b586ef Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 12:29:47 -0600 Subject: [PATCH 14/23] Integration test for build time PATH --- buildpacks/ruby/tests/integration_test.rs | 71 +++++++++++++++++++++-- 1 file changed, 66 insertions(+), 5 deletions(-) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index 0860be92..e78ecbcd 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -9,6 +9,7 @@ use libcnb_test::{ ContainerConfig, ContainerContext, TestRunner, }; use pretty_assertions::assert_eq; +use regex::Regex; use std::os::unix::fs::PermissionsExt; use std::path::{Path, PathBuf}; use std::thread; @@ -59,6 +60,7 @@ fn test_migrating_metadata_or_layer_names() { #[test] #[ignore = "integration test"] +#[allow(clippy::too_many_lines)] fn test_default_app_ubuntu20() { let temp = tempfile::tempdir().unwrap(); let app_dir = temp.path(); @@ -129,17 +131,76 @@ fn test_default_app_ubuntu20() { ); fs_err::create_dir_all(app_dir.join("bin")).unwrap(); - fs_err::write(app_dir.join("bin").join("rake"), formatdoc!{" + fs_err::write(app_dir.join("bin").join("rake"), formatdoc!{r#" #!/usr/bin/env ruby - require_relative '../config/boot' - require 'rake' - Rake.application.run - "}).unwrap(); + # frozen_string_literal: true + + # + # This file was generated by Bundler. + # + # The application 'rake' is installed as part of a gem, and + # this file is here to facilitate running it. + # + + ENV["BUNDLE_GEMFILE"] ||= File.expand_path("../Gemfile", __dir__) + + bundle_binstub = File.expand_path("bundle", __dir__) + + if File.file?(bundle_binstub) + if File.read(bundle_binstub, 300).include?("This file was generated by Bundler") + load(bundle_binstub) + else + abort("Your `bin/bundle` was not generated by Bundler, so this binstub cannot run. + Replace `bin/bundle` by running `bundle binstubs bundler --force`, then run this command again.") + end + end + + require "rubygems" + require "bundler/setup" + + load Gem.bin_path("rake", "rake") + "#}).unwrap(); chmod_plus_x(&app_dir.join("bin").join("rake")).unwrap(); + fs_err::write(app_dir.join("Rakefile"), r#" + task "assets:precompile" do + puts "Inspecting ruby via rake assets:precompile" + puts "==========================================" + run!("echo $PATH") + run!("which -a rake") + run!("which -a ruby") + end + + def run!(cmd) + puts "$ #{cmd}" + output = `#{cmd} 2>&1` + raise "Command #{cmd} failed with output #{output}" unless $?.success? + puts output + end + "#).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)"); + assert_contains!( + Regex::new(r"/layers/heroku_ruby/gems/ruby/\d+\.\d+\.\d+/bin").unwrap().replace_all(&rebuild_context.pack_stdout, "/layers/heroku_ruby/gems/ruby//bin") + , r" + Inspecting ruby via rake assets:precompile + ========================================== + $ echo $PATH + /layers/heroku_ruby/gems/ruby//bin:/workspace/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 + $ which -a rake + /layers/heroku_ruby/gems/ruby//bin/rake + /workspace/bin/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 +".trim()); let command_output = rebuild_context.run_shell_command( indoc! {" From 86bc51396c28051744e8e6f30338a49234184149 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 12:30:14 -0600 Subject: [PATCH 15/23] Clarify that `bundle exec` changes the PATH order --- docs/application_contract.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/application_contract.md b/docs/application_contract.md index 4c8c6bba..b92b4a05 100644 --- a/docs/application_contract.md +++ b/docs/application_contract.md @@ -80,7 +80,7 @@ Once an application has passed the detect phase, the build phase will execute to - `GEM_PATH=` - Tells Ruby where gems are located. - `MALLOC_ARENA_MAX=2` - Controls glibc memory allocation behavior with the goal of decreasing overall memory allocated by Ruby [details](https://devcenter.heroku.com/changelog-items/1683). - `PATH` - Various executables are installed and the `PATH` env var will be modified so they can be executed at the system level. This is mostly done via interfaces provided by `libcnb` and CNB layers rather than directly. - - Executables in the application `bin` directory will take precedence over gem installed executables. + - Executables in the application `bin` directory will take precedence over gem installed executables. Note that some commands like `bundle exec` may alter the `PATH` to change this order. - Executables from gems will take precedence over executables that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that come with the compiled Ruby binary). - `RAILS_LOG_TO_STDOUT="enabled"` - Sets the default logging target to STDOUT for Rails 5+ apps. [details](https://blog.heroku.com/container_ready_rails_5) - `RAILS_SERVE_STATIC_FILES="enabled"` - Enables the `ActionDispatch::Static` middleware for Rails 5+ apps so that static files such as those in `public/assets` are served by the Ruby webserver such as Puma [details](https://blog.heroku.com/container_ready_rails_5). From 12cd6fd6a3affe660b1f6621a23c1b3790a3eb91 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 12:33:19 -0600 Subject: [PATCH 16/23] Fix grammar --- docs/application_contract.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/application_contract.md b/docs/application_contract.md index b92b4a05..62e2369b 100644 --- a/docs/application_contract.md +++ b/docs/application_contract.md @@ -81,6 +81,6 @@ Once an application has passed the detect phase, the build phase will execute to - `MALLOC_ARENA_MAX=2` - Controls glibc memory allocation behavior with the goal of decreasing overall memory allocated by Ruby [details](https://devcenter.heroku.com/changelog-items/1683). - `PATH` - Various executables are installed and the `PATH` env var will be modified so they can be executed at the system level. This is mostly done via interfaces provided by `libcnb` and CNB layers rather than directly. - Executables in the application `bin` directory will take precedence over gem installed executables. Note that some commands like `bundle exec` may alter the `PATH` to change this order. - - Executables from gems will take precedence over executables that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that come with the compiled Ruby binary). + - Executables from gems will take precedence over executables that ship with Ruby (for example `rake` installed from `bundle install` should be loaded before `rake` that comes with the compiled Ruby binary). - `RAILS_LOG_TO_STDOUT="enabled"` - Sets the default logging target to STDOUT for Rails 5+ apps. [details](https://blog.heroku.com/container_ready_rails_5) - `RAILS_SERVE_STATIC_FILES="enabled"` - Enables the `ActionDispatch::Static` middleware for Rails 5+ apps so that static files such as those in `public/assets` are served by the Ruby webserver such as Puma [details](https://blog.heroku.com/container_ready_rails_5). From 7c149df122676f1b174ff61fd46e9c0944d59e87 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 12:48:44 -0600 Subject: [PATCH 17/23] 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`. --- buildpacks/ruby/src/rake_task_detect.rs | 6 ++---- .../ruby/src/steps/rake_assets_install.rs | 20 +++++++------------ 2 files changed, 9 insertions(+), 17 deletions(-) diff --git a/buildpacks/ruby/src/rake_task_detect.rs b/buildpacks/ruby/src/rake_task_detect.rs index 2d287673..b19ed35a 100644 --- a/buildpacks/ruby/src/rake_task_detect.rs +++ b/buildpacks/ruby/src/rake_task_detect.rs @@ -29,10 +29,8 @@ pub(crate) fn call, K: AsRef, V: AsRef Result<(Print>, 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| { diff --git a/buildpacks/ruby/src/steps/rake_assets_install.rs b/buildpacks/ruby/src/steps/rake_assets_install.rs index 58938d35..9f8cf604 100644 --- a/buildpacks/ruby/src/steps/rake_assets_install.rs +++ b/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( From 7471a4805ffeb82369111a3ab830bdd37ce7d889 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 14:01:09 -0600 Subject: [PATCH 18/23] Make integration failures prettier --- buildpacks/ruby/tests/integration_test.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index e78ecbcd..d4b95219 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -164,11 +164,11 @@ fn test_default_app_ubuntu20() { fs_err::write(app_dir.join("Rakefile"), r#" task "assets:precompile" do - puts "Inspecting ruby via rake assets:precompile" - puts "==========================================" + puts "START RAKE TEST OUTPUT" run!("echo $PATH") run!("which -a rake") run!("which -a ruby") + puts "END RAKE TEST OUTPUT" end def run!(cmd) @@ -179,14 +179,13 @@ fn test_default_app_ubuntu20() { end "#).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)"); - assert_contains!( - Regex::new(r"/layers/heroku_ruby/gems/ruby/\d+\.\d+\.\d+/bin").unwrap().replace_all(&rebuild_context.pack_stdout, "/layers/heroku_ruby/gems/ruby//bin") - , r" - Inspecting ruby via rake assets:precompile - ========================================== + let rake_output = Regex::new(r"(?sm)START RAKE TEST OUTPUT\n(.*)END RAKE TEST OUTPUT").unwrap().captures(&rebuild_context.pack_stdout).and_then(|captures| captures.get(1).map(|m| m.as_str().to_string())).unwrap(); + assert_eq!( + r" $ echo $PATH /layers/heroku_ruby/gems/ruby//bin:/workspace/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 $ which -a rake @@ -200,7 +199,9 @@ fn test_default_app_ubuntu20() { /layers/heroku_ruby/binruby/bin/ruby /usr/bin/ruby /bin/ruby -".trim()); + ".trim(), + Regex::new(r"/layers/heroku_ruby/gems/ruby/\d+\.\d+\.\d+/bin").unwrap().replace_all(&rake_output, "/layers/heroku_ruby/gems/ruby//bin").trim() +); let command_output = rebuild_context.run_shell_command( indoc! {" From 24a34bd4bce17a2f0220686d5dbe4f6813868cb7 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 14:04:17 -0600 Subject: [PATCH 19/23] Lint import order --- buildpacks/ruby/src/layers/bundle_install_layer.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/buildpacks/ruby/src/layers/bundle_install_layer.rs b/buildpacks/ruby/src/layers/bundle_install_layer.rs index 17f955ad..060a14b5 100644 --- a/buildpacks/ruby/src/layers/bundle_install_layer.rs +++ b/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 From c91cee5716633d768e0abadc8f2d14549fc25b38 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 14:25:19 -0600 Subject: [PATCH 20/23] 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 https://github.com/heroku/libcnb.rs/issues/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. --- buildpacks/ruby/src/main.rs | 26 +++++++++++++++++++++++- buildpacks/ruby/src/steps/default_env.rs | 12 ++--------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/buildpacks/ruby/src/main.rs b/buildpacks/ruby/src/main.rs index 0717d357..9ff4f2f5 100644 --- a/buildpacks/ruby/src/main.rs +++ b/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; @@ -220,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"); diff --git a/buildpacks/ruby/src/steps/default_env.rs b/buildpacks/ruby/src/steps/default_env.rs index 1b66a358..d8f3849e 100644 --- a/buildpacks/ruby/src/steps/default_env.rs +++ b/buildpacks/ruby/src/steps/default_env.rs @@ -38,20 +38,12 @@ pub(crate) fn default_env( .to_string(); let layer_ref = context.uncached_layer( - layer_name!("venv"), + layer_name!("env_defaults"), 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({ [ @@ -65,7 +57,7 @@ pub(crate) fn default_env( ("DISABLE_SPRING", "1"), ] .iter() - .fold(update_env, |layer_env, (name, value)| { + .fold(LayerEnv::new(), |layer_env, (name, value)| { layer_env.chainable_insert(Scope::All, ModificationBehavior::Default, name, value) }) }) From b6820b6f713da8cd54e5f80ba7bd663788923222 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 16:44:45 -0600 Subject: [PATCH 21/23] Spike: Auto Layer Ordering Prefix layers with sequential numbers starting at `0001_`. To support this it assumes any layers on disk with a number prefix is equivalent i.e. changing the order of layers will not invalidate the cache. This has the benefit that: - Layers can be named something semantic without side effects. - Build and launch layer behavior is guaranteed to be the same (provided `read_env` is called and applied for every layer in main). --- commons/Cargo.toml | 5 ++ commons/src/cache/app_cache.rs | 18 +++++-- commons/src/layer.rs | 1 + commons/src/layer/diff_migrate.rs | 80 +++++++++++++++++++++---------- commons/src/layer/order.rs | 68 ++++++++++++++++++++++++++ 5 files changed, 144 insertions(+), 28 deletions(-) create mode 100644 commons/src/layer/order.rs diff --git a/commons/Cargo.toml b/commons/Cargo.toml index bbfb0d44..2d433f6d 100644 --- a/commons/Cargo.toml +++ b/commons/Cargo.toml @@ -38,3 +38,8 @@ libcnb-test = "=0.26.1" pretty_assertions = "1" toml = "0.8" bullet_stream = "0.3.0" + +[features] +# Auto name layers in the order of their creation so that launch and build are resolved in the same order +auto_layer_ordering = [] +default = ["auto_layer_ordering"] diff --git a/commons/src/cache/app_cache.rs b/commons/src/cache/app_cache.rs index 395aa138..7c20ddef 100644 --- a/commons/src/cache/app_cache.rs +++ b/commons/src/cache/app_cache.rs @@ -1,5 +1,6 @@ use crate::cache::clean::{lru_clean, FilesWithSize}; use crate::cache::{CacheConfig, CacheError, KeepPath}; +use crate::layer::order::ordered_layer_name; use byte_unit::{AdjustedByte, Byte, UnitType}; use fs_extra::dir::CopyOptions; use libcnb::build::BuildContext; @@ -387,9 +388,15 @@ fn create_layer_name(app_root: &Path, path: &Path) -> Result>() .join("_"); - format!("cache_{name}") + let layer_name: LayerName = format!("cache_{name}") .parse() - .map_err(CacheError::InvalidLayerName) + .map_err(CacheError::InvalidLayerName)?; + + if cfg!(feature = "auto_layer_ordering") { + Ok(ordered_layer_name(layer_name)) + } else { + Ok(layer_name) + } } /// Determines if a cache directory in a layer previously existed or not. @@ -420,6 +427,8 @@ fn is_empty_dir(path: &Path) -> bool { #[cfg(test)] mod tests { + use crate::layer::order::strip_order_prefix; + use super::*; use filetime::FileTime; use libcnb::data::layer_name; @@ -429,7 +438,10 @@ mod tests { fn test_to_layer_name() { let dir = PathBuf::from_str("muh_base").unwrap(); let layer = create_layer_name(&dir, &dir.join("my").join("input")).unwrap(); - assert_eq!(layer_name!("cache_my_input"), layer); + assert_eq!( + layer_name!("cache_my_input").as_str(), + &strip_order_prefix(layer.as_str()) + ); } #[test] diff --git a/commons/src/layer.rs b/commons/src/layer.rs index 4ec3cade..759af313 100644 --- a/commons/src/layer.rs +++ b/commons/src/layer.rs @@ -1,6 +1,7 @@ mod configure_env_layer; mod default_env_layer; pub mod diff_migrate; +pub(crate) mod order; #[deprecated(note = "Use the struct layer API in the latest libcnb.rs instead")] pub use self::configure_env_layer::ConfigureEnvLayer; diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index aaf398c7..b7c1ee73 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -43,6 +43,8 @@ #![doc = include_str!("fixtures/metadata_migration_example.md")] use crate::display::SentenceList; +use crate::layer::order::contains_entry_with_name_or_pattern; +use crate::layer::order::ordered_layer_name; use cache_diff::CacheDiff; use fs_err::PathExt; use libcnb::build::BuildContext; @@ -53,7 +55,7 @@ use libcnb::layer::{ use magic_migrate::TryMigrate; use serde::ser::Serialize; use std::fmt::Debug; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; #[cfg(test)] use bullet_stream as _; @@ -120,6 +122,19 @@ impl DiffMigrateLayer { B: libcnb::Buildpack, M: CacheDiff + TryMigrate + Serialize + Debug + Clone, { + let layer_name = if cfg!(feature = "auto_layer_ordering") { + let layer_name = ordered_layer_name(layer_name); + + // Move NNNN_ or `` to `NNNN_` + if let Some(prior) = layer_to_path(context, &layer_name)? { + move_layer_path(&prior, &context.layers_dir.join(layer_name.as_str())) + .map_err(LayerError::IoError)?; + } + layer_name + } else { + layer_name + }; + let layer_ref = context.cached_layer( layer_name, CachedLayerDefinition { @@ -166,25 +181,28 @@ impl DiffMigrateLayer { if let (Some(prior_dir), None) = ( prior_layers .iter() - .map(|layer_name| is_layer_on_disk(layer_name, context)) + .map(|layer_name| layer_to_path(context, layer_name)) .collect::>, _>>()? .iter() .find_map(std::borrow::ToOwned::to_owned), - is_layer_on_disk(&to_layer, context)?, + layer_to_path(context, &to_layer)?, ) { - let to_dir = context.layers_dir.join(to_layer.as_str()); - std::fs::create_dir_all(&to_dir).map_err(LayerError::IoError)?; - std::fs::rename(&prior_dir, &to_dir).map_err(LayerError::IoError)?; - std::fs::rename( - prior_dir.with_extension("toml"), - to_dir.with_extension("toml"), - ) - .map_err(LayerError::IoError)?; + move_layer_path(&prior_dir, &context.layers_dir.join(to_layer.as_str())) + .map_err(LayerError::IoError)?; } self.cached_layer(to_layer, context, metadata) } } +fn move_layer_path(from_dir: &Path, to_dir: &Path) -> Result<(), std::io::Error> { + std::fs::create_dir_all(to_dir)?; + std::fs::rename(from_dir, to_dir)?; + std::fs::rename( + from_dir.with_extension("toml"), + to_dir.with_extension("toml"), + ) +} + /// Represents when we want to move contents from one (or more) layer names /// pub struct LayerRename { @@ -195,18 +213,24 @@ pub struct LayerRename { } /// Returns Some(PathBuf) when the layer exists on disk -fn is_layer_on_disk( - layer_name: &LayerName, +/// +/// Is aware of auto layer-ordering and will convert `0001_my_layer` => `my_layer` and vise-versa +fn layer_to_path( context: &BuildContext, + layer_name: &LayerName, ) -> libcnb::Result, B::Error> where B: libcnb::Buildpack, { - let path = context.layers_dir.join(layer_name.as_str()); - - path.fs_err_try_exists() - .map_err(|error| libcnb::Error::LayerError(LayerError::IoError(error))) - .map(|exists| exists.then_some(path)) + if cfg!(feature = "auto_layer_ordering") { + contains_entry_with_name_or_pattern(&context.layers_dir, layer_name.as_str()) + .map_err(|error| libcnb::Error::LayerError(LayerError::IoError(error))) + } else { + let path = context.layers_dir.join(layer_name.as_str()); + path.fs_err_try_exists() + .map_err(|error| libcnb::Error::LayerError(LayerError::IoError(error))) + .map(|exists| exists.then_some(path)) + } } /// Standardizes formatting for layer cache clearing behavior @@ -292,6 +316,7 @@ where /// /// - Will only ever contain metadata when the cache is retained. /// - Will contain a message when the cache is cleared, describing why it was cleared. +#[derive(Debug)] pub enum Meta { Message(String), Data(M), @@ -322,6 +347,7 @@ mod tests { use libcnb::layer::{EmptyLayerCause, InvalidMetadataAction, LayerState, RestoredLayerAction}; use magic_migrate::{migrate_toml_chain, try_migrate_deserializer_chain, Migrate, TryMigrate}; use std::convert::Infallible; + /// Struct for asserting the behavior of `CacheBuddy` #[derive(Debug, serde::Serialize, serde::Deserialize, Clone)] #[serde(deny_unknown_fields)] @@ -418,9 +444,9 @@ mod tests { } )); - assert!(context - .layers_dir - .join(old_layer_name.as_str()) + assert!(layer_to_path(&context, &old_layer_name) + .unwrap() + .unwrap() .fs_err_try_exists() .unwrap()); @@ -446,10 +472,14 @@ mod tests { ) .unwrap(); - assert!(matches!(result.state, LayerState::Restored { cause: _ })); - assert!(context - .layers_dir - .join(new_layer_name.as_str()) + assert!( + matches!(result.state, LayerState::Restored { cause: _ }), + "Does not match {:?}", + result.state + ); + assert!(layer_to_path(&context, &new_layer_name) + .unwrap() + .unwrap() .fs_err_try_exists() .unwrap()); } diff --git a/commons/src/layer/order.rs b/commons/src/layer/order.rs new file mode 100644 index 00000000..410897d7 --- /dev/null +++ b/commons/src/layer/order.rs @@ -0,0 +1,68 @@ +use libcnb::data::layer::LayerName; +use std::path::{Path, PathBuf}; +use std::sync::atomic::{AtomicUsize, Ordering}; + +/// As layers are created this is incremented +static LAYER_COUNT: AtomicUsize = AtomicUsize::new(1); +/// Formatting for number of leanding zeros. Max number of layers is 1024 (as of 2025). Four digits allows +/// one buildpack to meet or exceed that value +static DIGITS: usize = 4; + +fn prefix(value: usize) -> String { + format!("{value:0DIGITS$}_") +} + +fn next_count() -> usize { + LAYER_COUNT.fetch_add(1, Ordering::Acquire) +} + +/// Removes the `NNNN_` prefix if there is one +pub(crate) fn strip_order_prefix(name: &str) -> String { + let re = regex::Regex::new(&format!("^\\d{{{DIGITS}}}_")) + .expect("internal code bugs caught by unit tests"); + re.replace(name, "").to_string() +} + +/// Searches the given dir for an entry with the exact name or any NNNN_ prefix and returns Ok(Some(PathBuf)) +pub(crate) fn contains_entry_with_name_or_pattern( + dir: &Path, + name: &str, +) -> Result, std::io::Error> { + let name = strip_order_prefix(name); + + let pattern = format!("^\\d{{{DIGITS}}}_{}$", regex::escape(&name)); + let re = regex::Regex::new(&pattern).expect("internal error if this fails to compile"); + + for entry in fs_err::read_dir(dir)?.flatten() { + if let Some(file_name) = entry.file_name().to_str() { + if file_name == name || re.is_match(file_name) { + return Ok(Some(entry.path().clone())); + } + } + } + + Ok(None) +} + +/// Gets and increments the next name +/// +/// # Panics +/// +/// Assumes that prepending a value to a valid layer name is a valid operation +#[must_use] +pub(crate) fn ordered_layer_name(name: LayerName) -> LayerName { + let prefix = prefix(next_count()); + prefix_layer_name(&prefix, name) +} + +/// # Panics +/// +/// Assumes that prepending a value to a valid layer name is a valid operation +#[must_use] +#[allow(clippy::needless_pass_by_value)] +fn prefix_layer_name(prefix: impl AsRef, name: LayerName) -> LayerName { + let prefix = prefix.as_ref(); + format!("{prefix}{}", name.as_str()) + .parse() + .expect("Prepending to a valid layer name is valid") +} From 9c399c5218614cc1c4d36914bc88a261739d5cc1 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 17:18:48 -0600 Subject: [PATCH 22/23] Update integration tests --- buildpacks/ruby/tests/integration_test.rs | 42 +++++++++++------------ 1 file changed, 21 insertions(+), 21 deletions(-) diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index d4b95219..8e6d39f1 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -81,7 +81,7 @@ fn test_default_app_ubuntu20() { assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack"); assert_contains!( context.pack_stdout, - 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`"#); + r#"`BUNDLE_BIN="/layers/heroku_ruby/0003_gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/0003_gems" BUNDLE_WITHOUT="development:test" bundle install`"#); assert_contains!(context.pack_stdout, "Installing puma"); @@ -101,29 +101,29 @@ fn test_default_app_ubuntu20() { assert_empty!(command_output.stderr); assert_eq!( formatdoc! {" - BUNDLE_BIN=/layers/heroku_ruby/gems/bin + BUNDLE_BIN=/layers/heroku_ruby/0003_gems/bin BUNDLE_CLEAN=1 BUNDLE_DEPLOYMENT=1 BUNDLE_GEMFILE=/workspace/Gemfile - BUNDLE_PATH=/layers/heroku_ruby/gems + BUNDLE_PATH=/layers/heroku_ruby/0003_gems BUNDLE_WITHOUT=development:test DISABLE_SPRING=1 - GEM_PATH=/layers/heroku_ruby/gems:/layers/heroku_ruby/bundler + GEM_PATH=/layers/heroku_ruby/0003_gems:/layers/heroku_ruby/0002_bundler JRUBY_OPTS=-Xcompile.invokedynamic=false - LD_LIBRARY_PATH=/layers/heroku_ruby/binruby/lib + LD_LIBRARY_PATH=/layers/heroku_ruby/0001_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 + PATH=/workspace/bin:/layers/heroku_ruby/0002_bundler/bin:/layers/heroku_ruby/0003_gems/bin:/layers/heroku_ruby/0002_bundler/bin:/layers/heroku_ruby/0001_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 + /layers/heroku_ruby/0003_gems/bin/rake + /layers/heroku_ruby/0001_binruby/bin/rake /usr/bin/rake /bin/rake + which -a ruby - /layers/heroku_ruby/binruby/bin/ruby + /layers/heroku_ruby/0001_binruby/bin/ruby /usr/bin/ruby /bin/ruby "}, @@ -187,20 +187,20 @@ fn test_default_app_ubuntu20() { assert_eq!( r" $ echo $PATH - /layers/heroku_ruby/gems/ruby//bin:/workspace/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 + /layers/heroku_ruby/0003_gems/ruby//bin:/workspace/bin:/layers/heroku_ruby/0003_gems/bin:/layers/heroku_ruby/0002_bundler/bin:/layers/heroku_ruby/0001_binruby/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin $ which -a rake - /layers/heroku_ruby/gems/ruby//bin/rake + /layers/heroku_ruby/0003_gems/ruby//bin/rake /workspace/bin/rake - /layers/heroku_ruby/gems/bin/rake - /layers/heroku_ruby/binruby/bin/rake + /layers/heroku_ruby/0003_gems/bin/rake + /layers/heroku_ruby/0001_binruby/bin/rake /usr/bin/rake /bin/rake $ which -a ruby - /layers/heroku_ruby/binruby/bin/ruby + /layers/heroku_ruby/0001_binruby/bin/ruby /usr/bin/ruby /bin/ruby ".trim(), - Regex::new(r"/layers/heroku_ruby/gems/ruby/\d+\.\d+\.\d+/bin").unwrap().replace_all(&rake_output, "/layers/heroku_ruby/gems/ruby//bin").trim() + Regex::new(r"/layers/heroku_ruby/0003_gems/ruby/\d+\.\d+\.\d+/bin").unwrap().replace_all(&rake_output, "/layers/heroku_ruby/0003_gems/ruby//bin").trim() ); let command_output = rebuild_context.run_shell_command( @@ -215,8 +215,8 @@ fn test_default_app_ubuntu20() { formatdoc! {" + which -a rake /workspace/bin/rake - /layers/heroku_ruby/gems/bin/rake - /layers/heroku_ruby/binruby/bin/rake + /layers/heroku_ruby/0003_gems/bin/rake + /layers/heroku_ruby/0001_binruby/bin/rake /usr/bin/rake /bin/rake "}, @@ -237,7 +237,7 @@ fn test_default_app_ubuntu22() { assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack"); assert_contains!( context.pack_stdout, - 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`"#); + r#"`BUNDLE_BIN="/layers/heroku_ruby/0003_gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/0003_gems" BUNDLE_WITHOUT="development:test" bundle install`"#); assert_contains!(context.pack_stdout, "Installing puma"); }, @@ -256,7 +256,7 @@ fn test_default_app_latest_distro() { assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack"); assert_contains!( context.pack_stdout, - 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`"#); + r#"`BUNDLE_BIN="/layers/heroku_ruby/0003_gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/0003_gems" BUNDLE_WITHOUT="development:test" bundle install`"#); assert_contains!(context.pack_stdout, "Installing puma"); @@ -337,7 +337,7 @@ DEPENDENCIES assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack"); assert_contains!( context.pack_stdout, - 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`"# + r#"`BUNDLE_BIN="/layers/heroku_ruby/0003_gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/0003_gems" BUNDLE_WITHOUT="development:test" bundle install`"# ); assert_contains!(context.pack_stdout, "Ruby version `3.1.4-jruby-9.4.8.0` from `Gemfile.lock`"); }); @@ -358,7 +358,7 @@ fn test_ruby_app_with_yarn_app() { assert_contains!(context.pack_stdout, "# Heroku Ruby Buildpack"); assert_contains!( context.pack_stdout, - 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`"#); + r#"`BUNDLE_BIN="/layers/heroku_ruby/0003_gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/0003_gems" BUNDLE_WITHOUT="development:test" bundle install`"#); } ); } From 9e171528ff1c87888e17349007307918aaeade18 Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 10 Jan 2025 17:52:51 -0600 Subject: [PATCH 23/23] Always write/update env even when loading from cache --- .../ruby/src/layers/bundle_download_layer.rs | 52 ++++++++++--------- 1 file changed, 28 insertions(+), 24 deletions(-) diff --git a/buildpacks/ruby/src/layers/bundle_download_layer.rs b/buildpacks/ruby/src/layers/bundle_download_layer.rs index ea3e91b5..cfed3739 100644 --- a/buildpacks/ruby/src/layers/bundle_download_layer.rs +++ b/buildpacks/ruby/src/layers/bundle_download_layer.rs @@ -36,6 +36,8 @@ pub(crate) fn handle( match &layer_ref.state { LayerState::Restored { cause } => { bullet = bullet.sub_bullet(cause); + + layer_ref.write_env(layer_env(&layer_ref.path()))?; Ok((bullet, layer_ref.read_env()?)) } LayerState::Empty { cause } => { @@ -46,9 +48,9 @@ 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)?; + let bullet = download_bundler(bullet, env, metadata, &layer_ref.path())?; + layer_ref.write_env(layer_env(&layer_ref.path()))?; Ok((bullet, layer_ref.read_env()?)) } } @@ -73,15 +75,33 @@ pub(crate) enum MetadataError { // Update if migrating between a metadata version can error } +fn layer_env(gem_path: &Path) -> LayerEnv { + let bin_dir = gem_path.join("bin"); + + 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, + ) +} + fn download_bundler( bullet: Print>, env: &Env, metadata: &Metadata, - path: &Path, -) -> Result<(Print>, LayerEnv), RubyBuildpackError> { - let bin_dir = path.join("bin"); - let gem_path = path; - + gem_path: &Path, +) -> Result>, RubyBuildpackError> { + let bin_dir = gem_path.join("bin"); let mut cmd = Command::new("gem"); cmd.args(["install", "bundler"]); cmd.args(["--version", &metadata.version.to_string()]) // Specify exact version to install @@ -104,23 +124,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)]