From 4452d3aa492f1c17303ddbbf718946f23ce72682 Mon Sep 17 00:00:00 2001 From: Richard Schneeman Date: Mon, 13 Jan 2025 16:03:19 -0600 Subject: [PATCH] Fix user binstubs not on PATH and fix PATH order (#383) * Introduce struct to store rename information * Add helper to check if a layer exists or not * Add ability to rename a layer * Add tests * Rename `ruby` to `binruby` Note that this places `/layers/heroku_ruby/gems/bin` before `/layers/heroku_ruby/binruby` on the path. Related to, but doesn't entirely fix #380. * Changelog * Assert order of ruby/rake Using `-a` also shows that there's more than one value present. * Add failing test for `/workspace/bin` being on the path * Add pretty assertions for nicer diff output * Expected before actual * Put user provided binstub dir in the front of the path It's common and expected that Rails applications will include a `bin` directory containing "binstubs" of executables their app depends on. For example https://github.com/heroku/ruby-getting-started/tree/5e7ce01610a21cf9e5381daea66f79178e2b3c06/bin. They're largely used to ensure that bundler is invoked/used so that you can run `bin/rails` rather than needing to use `bundle exec rails`. However it's not strictly limited to only that. This change: Adds the `bin` folder in the root of the workspace to the PATH and changes the layer to `venv` so it is loaded after other layers (and takes precedence in the case of a PATH prepend). This fixes the previously committed failing test. Close #380 * Changelog * Clippy lints * Integration test for build time PATH * Clarify that `bundle exec` changes the PATH order * Fix grammar * Remove internal `bundle exec` calls The Application Contract does not specify that commands such as `rake -P` will be called with `bundle exec` and the classic buildpack does not rely on `bundle exec` internally. This brings the CNB closer to parity with the classic buildpack. In the container environment, the first gems on the PATH should be those installed by the buildpack, negating the strict need to call `bundle exec` as you would on a development machine. Usually prepending a Ruby command with `bundle exec` will have no discernible difference for an application that's bug free. This is evidenced by all tests passing with this change. However someone can commit their own `bin/rake` or `bin/rails` and we should use this over the executable installed via `bundle install`. * Make integration failures prettier * Lint import order * Move user binstubs to its own layer At runtime, the alphabetical order of the layer name determines the order it is loaded. At build time, the order that the `env` variable is modified determines the order. At both build and runtime we want the bin stubs to come first on the PATH when executing user defined code. This was already working for runtime, but wasn't for build time as the "gems" layer was being prepended to the path after the "venv" layer (because the `venv` layer was being defined first, last definition wins). I originally tried to fix this by defining the PATH inside of the "gems" layer along with the gems path but ran into 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. * Extract layer env modification from method * Update commons changelog date --- Cargo.lock | 1 + buildpacks/ruby/CHANGELOG.md | 5 + buildpacks/ruby/Cargo.toml | 1 + .../ruby/src/layers/bundle_download_layer.rs | 50 +++-- .../ruby/src/layers/bundle_install_layer.rs | 4 +- .../ruby/src/layers/metrics_agent_install.rs | 4 +- .../ruby/src/layers/ruby_install_layer.rs | 11 +- buildpacks/ruby/src/main.rs | 28 ++- buildpacks/ruby/src/rake_task_detect.rs | 6 +- .../ruby/src/steps/rake_assets_install.rs | 20 +- buildpacks/ruby/tests/integration_test.rs | 196 +++++++++++++++++- commons/CHANGELOG.md | 8 + commons/src/layer/diff_migrate.rs | 175 +++++++++++++++- docs/application_contract.md | 2 + 14 files changed, 455 insertions(+), 56 deletions(-) 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..2ba953c2 100644 --- a/buildpacks/ruby/src/layers/bundle_download_layer.rs +++ b/buildpacks/ruby/src/layers/bundle_download_layer.rs @@ -33,10 +33,27 @@ pub(crate) fn handle( launch: true, } .cached_layer(layer_name!("bundler"), context, metadata)?; + + let layer_env = LayerEnv::new() + .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":") + .chainable_insert( + Scope::All, + ModificationBehavior::Prepend, + "PATH", + // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle + layer_ref.path().join("bin"), + ) + .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":") + .chainable_insert( + Scope::All, + ModificationBehavior::Prepend, + "GEM_PATH", // Bundler is a gem too, allow it to be required + layer_ref.path(), + ); + layer_ref.write_env(&layer_env)?; match &layer_ref.state { LayerState::Restored { cause } => { bullet = bullet.sub_bullet(cause); - Ok((bullet, layer_ref.read_env()?)) } LayerState::Empty { cause } => { match cause { @@ -46,12 +63,10 @@ pub(crate) fn handle( bullet = bullet.sub_bullet(cause); } } - let (bullet, layer_env) = download_bundler(bullet, env, metadata, &layer_ref.path())?; - layer_ref.write_env(&layer_env)?; - - Ok((bullet, layer_ref.read_env()?)) + bullet = download_bundler(bullet, env, metadata, &layer_ref.path())?; } } + Ok((bullet, layer_ref.read_env()?)) } pub(crate) type Metadata = MetadataV1; @@ -77,10 +92,9 @@ fn download_bundler( bullet: Print>, 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"]); @@ -104,23 +118,7 @@ fn download_bundler( .map_err(|error| fun_run::map_which_problem(error, cmd.mut_cmd(), env.get("PATH").cloned())) .map_err(RubyBuildpackError::GemInstallBundlerCommandError)?; - let layer_env = LayerEnv::new() - .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "PATH", ":") - .chainable_insert( - Scope::All, - ModificationBehavior::Prepend, - "PATH", // Ensure this path comes before default bundler that ships with ruby, don't rely on the lifecycle - bin_dir, - ) - .chainable_insert(Scope::All, ModificationBehavior::Delimiter, "GEM_PATH", ":") - .chainable_insert( - Scope::All, - ModificationBehavior::Prepend, - "GEM_PATH", // Bundler is a gem too, allow it to be required - gem_path, - ); - - Ok((timer.done(), layer_env)) + Ok(timer.done()) } #[cfg(test)] 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..d4b95219 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,9 +60,22 @@ 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"); @@ -67,6 +84,145 @@ fn test_default_app_ubuntu20() { r#"`BUNDLE_BIN="/layers/heroku_ruby/gems/bin" BUNDLE_CLEAN="1" BUNDLE_DEPLOYMENT="1" BUNDLE_GEMFILE="/workspace/Gemfile" BUNDLE_PATH="/layers/heroku_ruby/gems" BUNDLE_WITHOUT="development:test" bundle install`"#); assert_contains!(context.pack_stdout, "Installing puma"); + + // Check that at run-time: + // - The correct env vars are set. + let command_output = context.run_shell_command( + indoc! {" + set -euo pipefail + printenv | sort | grep -vE '(_|HOME|HOSTNAME|OLDPWD|PWD|SHLVL|SECRET_KEY_BASE)=' + + # Output command + output to stdout + export BASH_XTRACEFD=1; set -o xtrace + which -a rake + which -a ruby + "} + ); + assert_empty!(command_output.stderr); + assert_eq!( + formatdoc! {" + BUNDLE_BIN=/layers/heroku_ruby/gems/bin + BUNDLE_CLEAN=1 + BUNDLE_DEPLOYMENT=1 + BUNDLE_GEMFILE=/workspace/Gemfile + BUNDLE_PATH=/layers/heroku_ruby/gems + BUNDLE_WITHOUT=development:test + DISABLE_SPRING=1 + GEM_PATH=/layers/heroku_ruby/gems:/layers/heroku_ruby/bundler + JRUBY_OPTS=-Xcompile.invokedynamic=false + LD_LIBRARY_PATH=/layers/heroku_ruby/binruby/lib + MALLOC_ARENA_MAX=2 + PATH=/workspace/bin:/layers/heroku_ruby/bundler/bin:/layers/heroku_ruby/gems/bin:/layers/heroku_ruby/bundler/bin:/layers/heroku_ruby/binruby/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin + RACK_ENV=production + RAILS_ENV=production + RAILS_LOG_TO_STDOUT=enabled + RAILS_SERVE_STATIC_FILES=enabled + + which -a rake + /layers/heroku_ruby/gems/bin/rake + /layers/heroku_ruby/binruby/bin/rake + /usr/bin/rake + /bin/rake + + which -a ruby + /layers/heroku_ruby/binruby/bin/ruby + /usr/bin/ruby + /bin/ruby + "}, + command_output.stdout, + ); + + fs_err::create_dir_all(app_dir.join("bin")).unwrap(); + fs_err::write(app_dir.join("bin").join("rake"), formatdoc!{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/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(), + 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! {" + # Output command + output to stdout + export BASH_XTRACEFD=1; set -o xtrace + which -a rake + "} + ); + assert_empty!(command_output.stderr); + assert_eq!( + formatdoc! {" + + which -a rake + /workspace/bin/rake + /layers/heroku_ruby/gems/bin/rake + /layers/heroku_ruby/binruby/bin/rake + /usr/bin/rake + /bin/rake + "}, + command_output.stdout, + ); + }); }, ); } @@ -318,3 +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..c7e17924 100644 --- a/commons/CHANGELOG.md +++ b/commons/CHANGELOG.md @@ -1,5 +1,13 @@ # Changelog for commons features +## 2024-01-13 + +### 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) diff --git a/commons/src/layer/diff_migrate.rs b/commons/src/layer/diff_migrate.rs index 4476f6fd..aaf398c7 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 _; @@ -128,6 +132,81 @@ 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 +/// +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 +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 @@ -281,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(); 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).