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/CHANGELOG.md b/buildpacks/ruby/CHANGELOG.md index 3c0b9d7e..bdd72d76 100644 --- a/buildpacks/ruby/CHANGELOG.md +++ b/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 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/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)] 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 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/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/src/main.rs b/buildpacks/ruby/src/main.rs index 4663e905..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; @@ -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"); 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( diff --git a/buildpacks/ruby/tests/integration_test.rs b/buildpacks/ruby/tests/integration_test.rs index 1d05d524..8e6d39f1 100644 --- a/buildpacks/ruby/tests/integration_test.rs +++ b/buildpacks/ruby/tests/integration_test.rs @@ -3,10 +3,15 @@ // 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 regex::Regex; +use std::os::unix::fs::PermissionsExt; +use std::path::{Path, PathBuf}; use std::thread; use std::time::{Duration, Instant}; use ureq::Response; @@ -15,19 +20,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); @@ -56,17 +60,169 @@ fn test_migrating_metadata() { #[test] #[ignore = "integration test"] +#[allow(clippy::too_many_lines)] 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"); 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"); + + // 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/0003_gems/bin + BUNDLE_CLEAN=1 + BUNDLE_DEPLOYMENT=1 + BUNDLE_GEMFILE=/workspace/Gemfile + BUNDLE_PATH=/layers/heroku_ruby/0003_gems + BUNDLE_WITHOUT=development:test + DISABLE_SPRING=1 + GEM_PATH=/layers/heroku_ruby/0003_gems:/layers/heroku_ruby/0002_bundler + JRUBY_OPTS=-Xcompile.invokedynamic=false + LD_LIBRARY_PATH=/layers/heroku_ruby/0001_binruby/lib + MALLOC_ARENA_MAX=2 + 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/0003_gems/bin/rake + /layers/heroku_ruby/0001_binruby/bin/rake + /usr/bin/rake + /bin/rake + + which -a ruby + /layers/heroku_ruby/0001_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!{r#" + #!/usr/bin/env ruby + # 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 "START RAKE TEST OUTPUT" + run!("echo $PATH") + run!("which -a rake") + run!("which -a ruby") + puts "END RAKE TEST OUTPUT" + 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)"); + 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/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/0003_gems/ruby//bin/rake + /workspace/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/0001_binruby/bin/ruby + /usr/bin/ruby + /bin/ruby + ".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( + 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/0003_gems/bin/rake + /layers/heroku_ruby/0001_binruby/bin/rake + /usr/bin/rake + /bin/rake + "}, + command_output.stdout, + ); + }); }, ); } @@ -81,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"); }, @@ -100,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"); @@ -181,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`"); }); @@ -202,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`"#); } ); } @@ -318,3 +474,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(()) +} diff --git a/commons/CHANGELOG.md b/commons/CHANGELOG.md index dfa3109e..e046228c 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/383) + +## 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/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 4476f6fd..b7c1ee73 100644 --- a/commons/src/layer/diff_migrate.rs +++ b/commons/src/layer/diff_migrate.rs @@ -43,13 +43,19 @@ #![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; 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::{Path, PathBuf}; #[cfg(test)] use bullet_stream as _; @@ -116,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 { @@ -128,6 +147,90 @@ 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| layer_to_path(context, layer_name)) + .collect::>, _>>()? + .iter() + .find_map(std::borrow::ToOwned::to_owned), + layer_to_path(context, &to_layer)?, + ) { + 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 { + /// The desired layer name + pub to: LayerName, + /// A list of prior, possibly layer names + pub from: Vec, +} + +/// Returns Some(PathBuf) when the layer exists on disk +/// +/// 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, +{ + 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 @@ -213,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), @@ -243,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)] @@ -281,6 +386,104 @@ 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!(layer_to_path(&context, &old_layer_name) + .unwrap() + .unwrap() + .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: _ }), + "Does not match {:?}", + result.state + ); + assert!(layer_to_path(&context, &new_layer_name) + .unwrap() + .unwrap() + .fs_err_try_exists() + .unwrap()); + } + #[test] fn test_diff_migrate() { let temp = tempfile::tempdir().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") +} diff --git a/docs/application_contract.md b/docs/application_contract.md index effac1da..62e2369b 100644 --- a/docs/application_contract.md +++ b/docs/application_contract.md @@ -80,5 +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. 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 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).