From 43a63d9187104e623be624db342f42d65bd21d0a Mon Sep 17 00:00:00 2001 From: Schneems Date: Mon, 21 Apr 2025 20:01:13 -0500 Subject: [PATCH 01/12] Failing test for #900 --- libcnb/src/layer_env.rs | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index f04e26f2..b6c3a684 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -606,6 +606,7 @@ const PATH_LIST_SEPARATOR: &str = ";"; mod tests { use std::cmp::Ordering; use std::collections::HashMap; + use std::ffi::{OsStr, OsString}; use std::fs; use tempfile::tempdir; @@ -899,6 +900,35 @@ mod tests { assert_eq!(env.get("PKG_CONFIG_PATH"), None); } + #[test] + fn bin_on_path_last() { + let temp_dir = tempdir().unwrap(); + let layer_dir = temp_dir.path(); + + // https://github.com/heroku/libcnb.rs/issues/900 + fs::create_dir_all(layer_dir.join("bin")).unwrap(); + fs::create_dir_all(layer_dir.join("bada/bing")).unwrap(); + + let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); + layer_env.insert( + Scope::Build, + ModificationBehavior::Prepend, + "PATH", + layer_dir.join("bada/bing").as_os_str(), + ); + + layer_env.write_to_layer_dir(layer_dir).unwrap(); + let env = layer_env.apply_to_empty(Scope::Build); + assert_eq!( + env.get("PATH").unwrap(), + &[layer_dir.join("bada/bing"), layer_dir.join("bin")] + .map(|dir| dir.as_os_str().to_owned()) + .into_iter() + .collect::>() + .join(OsStr::new(":")) + ); + } + #[test] fn read_from_layer_dir_layer_paths_build() { let temp_dir = tempdir().unwrap(); From 2e85625471516375f3c117e513dbb123c510a8dc Mon Sep 17 00:00:00 2001 From: Schneems Date: Mon, 28 Apr 2025 10:40:14 -0500 Subject: [PATCH 02/12] Add a delimiter to the path Missing delimiter strikes again --- libcnb/src/layer_env.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index b6c3a684..82451626 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -910,6 +910,7 @@ mod tests { fs::create_dir_all(layer_dir.join("bada/bing")).unwrap(); let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); + layer_env.insert(Scope::Build, ModificationBehavior::Delimiter, "PATH", ":"); layer_env.insert( Scope::Build, ModificationBehavior::Prepend, From 75fd5e5300ad8aed4f5e799dafe69d215ec8a82e Mon Sep 17 00:00:00 2001 From: Schneems Date: Mon, 28 Apr 2025 20:54:06 -0500 Subject: [PATCH 03/12] Fix auto PATH ordering In #900, I observed that the path of a layer's `./bin` dir (if present) is appended to any explicitly added paths by CNB lifecycle. Libcnb instead prepends the path. This is fixed to match the upstream implementation by changing the order of path evaluation. Close #900 --- libcnb/src/layer_env.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index 82451626..c386268e 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -139,8 +139,8 @@ impl LayerEnv { pub fn apply(&self, scope: Scope, env: &Env) -> Env { let deltas = match scope { Scope::All => vec![&self.all], - Scope::Build => vec![&self.all, &self.build, &self.layer_paths_build], - Scope::Launch => vec![&self.all, &self.launch, &self.layer_paths_launch], + Scope::Build => vec![&self.layer_paths_build, &self.all, &self.build], + Scope::Launch => vec![&self.layer_paths_launch, &self.all, &self.launch], Scope::Process(process) => { let mut process_deltas = vec![&self.all]; if let Some(process_specific_delta) = self.process.get(&process) { From de93ecb854871469582ac046ce58234eee2943e0 Mon Sep 17 00:00:00 2001 From: Schneems Date: Mon, 28 Apr 2025 21:05:50 -0500 Subject: [PATCH 04/12] Test Build and Launch scope --- libcnb/src/layer_env.rs | 48 ++++++++++++++++++++++------------------- 1 file changed, 26 insertions(+), 22 deletions(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index c386268e..eccf32e8 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -901,33 +901,37 @@ mod tests { } #[test] - fn bin_on_path_last() { + fn layer_paths_come_before_manually_added_paths() { let temp_dir = tempdir().unwrap(); let layer_dir = temp_dir.path(); // https://github.com/heroku/libcnb.rs/issues/900 fs::create_dir_all(layer_dir.join("bin")).unwrap(); - fs::create_dir_all(layer_dir.join("bada/bing")).unwrap(); - - let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); - layer_env.insert(Scope::Build, ModificationBehavior::Delimiter, "PATH", ":"); - layer_env.insert( - Scope::Build, - ModificationBehavior::Prepend, - "PATH", - layer_dir.join("bada/bing").as_os_str(), - ); - - layer_env.write_to_layer_dir(layer_dir).unwrap(); - let env = layer_env.apply_to_empty(Scope::Build); - assert_eq!( - env.get("PATH").unwrap(), - &[layer_dir.join("bada/bing"), layer_dir.join("bin")] - .map(|dir| dir.as_os_str().to_owned()) - .into_iter() - .collect::>() - .join(OsStr::new(":")) - ); + fs::create_dir_all(layer_dir.join("explicit_path")).unwrap(); + + // TODO: Determine desired behavior of Scope::All + for scope in [Scope::Build, Scope::Launch] { + let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); + layer_env.insert(scope.clone(), ModificationBehavior::Delimiter, "PATH", ":"); + layer_env.insert( + scope.clone(), + ModificationBehavior::Prepend, + "PATH", + layer_dir.join("explicit_path").as_os_str(), + ); + + layer_env.write_to_layer_dir(layer_dir).unwrap(); + let env = layer_env.apply_to_empty(scope.clone()); + assert_eq!( + env.get("PATH").unwrap(), + &[layer_dir.join("explicit_path"), layer_dir.join("bin")] + .map(|dir| dir.as_os_str().to_owned()) + .into_iter() + .collect::>() + .join(OsStr::new(":")), + "PATH was not prepended correctly for scope: `{scope:?}`" + ); + } } #[test] From 6b02e81b16041008484461933898d6ab860cb9ca Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 29 Apr 2025 09:09:11 -0500 Subject: [PATCH 05/12] Expected before actual in assertion --- libcnb/src/layer_env.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index eccf32e8..a5a78619 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -923,12 +923,12 @@ mod tests { layer_env.write_to_layer_dir(layer_dir).unwrap(); let env = layer_env.apply_to_empty(scope.clone()); assert_eq!( - env.get("PATH").unwrap(), &[layer_dir.join("explicit_path"), layer_dir.join("bin")] .map(|dir| dir.as_os_str().to_owned()) .into_iter() .collect::>() .join(OsStr::new(":")), + env.get("PATH").unwrap(), "PATH was not prepended correctly for scope: `{scope:?}`" ); } From 699fb6f45bc19622f8264184339785934187196d Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 29 Apr 2025 09:15:47 -0500 Subject: [PATCH 06/12] Test LIBRARY_PATH ordering --- libcnb/src/layer_env.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index a5a78619..1458fc07 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -907,8 +907,10 @@ mod tests { // https://github.com/heroku/libcnb.rs/issues/900 fs::create_dir_all(layer_dir.join("bin")).unwrap(); + fs::create_dir_all(layer_dir.join("lib")).unwrap(); fs::create_dir_all(layer_dir.join("explicit_path")).unwrap(); + // Test Build and Launch PATH // TODO: Determine desired behavior of Scope::All for scope in [Scope::Build, Scope::Launch] { let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); @@ -932,6 +934,32 @@ mod tests { "PATH was not prepended correctly for scope: `{scope:?}`" ); } + + // Test Build LIBRARY_PATH + let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); + layer_env.insert( + Scope::Build, + ModificationBehavior::Delimiter, + "LIBRARY_PATH", + ":", + ); + layer_env.insert( + Scope::Build, + ModificationBehavior::Prepend, + "LIBRARY_PATH", + layer_dir.join("explicit_path").as_os_str(), + ); + + layer_env.write_to_layer_dir(layer_dir).unwrap(); + let env = layer_env.apply_to_empty(Scope::Build); + assert_eq!( + &[layer_dir.join("explicit_path"), layer_dir.join("lib")] + .map(|dir| dir.as_os_str().to_owned()) + .into_iter() + .collect::>() + .join(OsStr::new(":")), + env.get("LIBRARY_PATH").unwrap() + ); } #[test] From 0c9d14d178adeaeeed5a5c535dcf168a66b9ee10 Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 29 Apr 2025 09:19:47 -0500 Subject: [PATCH 07/12] Changelog --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4740b410..1ab6d6c8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed + +- libcnb: + - Order of automatically applied environment variables by libcnb, such as `PATH=/bin`, now matches the upstream CNB lifecycle (Close [#900](https://github.com/heroku/libcnb.rs/issues/900)). ([#938](https://github.com/heroku/libcnb.rs/pull/938)) + ## [0.28.1] - 2025-03-25 From c64e8bc6feb859e2ef235c77ef6c77ef7fd8cbbd Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 13 May 2025 18:31:44 -0500 Subject: [PATCH 08/12] Doc whitespace and headers --- libcnb/src/layer_env.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index 1458fc07..004bd96e 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -28,12 +28,14 @@ use std::path::Path; /// logic that uses the build tool to download dependencies. The download process does not need to /// know the layer name or any of the logic for constructing `PATH`. /// -/// # Applying the delta +/// ## Applying the delta +/// /// `LayerEnv` is not a static set of environment variables, but a delta. Layers can modify existing /// variables by appending, prepending or setting variables only if they were not already defined. If you only need a /// static set of environment variables, see [`Env`]. /// /// To apply a `LayerEnv` delta to a given `Env`, use [`LayerEnv::apply`] like so: +/// /// ``` /// use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope}; /// use libcnb::Env; @@ -51,7 +53,8 @@ use std::path::Path; /// assert_eq!(modified_env.get("VAR2").unwrap(), "previous-value"); /// ``` /// -/// # Implicit Entries +/// ## Implicit Entries +/// /// Some directories in a layer directory are implicitly added to the layer environment if they /// exist. The prime example for this behaviour is the `bin` directory. If it exists, its path will /// be automatically appended to `PATH` using the operating systems path delimiter as the delimiter. @@ -61,6 +64,7 @@ use std::path::Path; /// /// libcnb supports these, including all precedence and lifecycle rules, when a `LayerEnv` is read /// from disk: +/// /// ``` /// use libcnb::layer_env::{LayerEnv, Scope}; /// use std::fs; From bc1a98d5bcb35a7bb5d9498106aba48c4c3ac6a5 Mon Sep 17 00:00:00 2001 From: Schneems Date: Tue, 13 May 2025 18:34:04 -0500 Subject: [PATCH 09/12] Update docs s/append/prepend/ The docs stated that we're appending the layer-bin path. With this change we're now prepending it to align with lifecycle. --- libcnb/src/layer_env.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index 004bd96e..f0fe9d9a 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -57,7 +57,7 @@ use std::path::Path; /// /// Some directories in a layer directory are implicitly added to the layer environment if they /// exist. The prime example for this behaviour is the `bin` directory. If it exists, its path will -/// be automatically appended to `PATH` using the operating systems path delimiter as the delimiter. +/// be automatically prepended to `PATH` using the operating systems path delimiter as the delimiter. /// /// A full list of these special directories can be found in the /// [Cloud Native Buildpack specification](https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths). From cf6b9f063cbca2b35f575c0432ee59f0853321a2 Mon Sep 17 00:00:00 2001 From: Manuel Fuchs Date: Thu, 15 May 2025 13:34:39 +0200 Subject: [PATCH 10/12] Simplify layer_paths_come_before_manually_added_paths test --- libcnb/src/layer_env.rs | 79 +++++++++++++++-------------------------- 1 file changed, 29 insertions(+), 50 deletions(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index f0fe9d9a..8a2824b1 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -610,9 +610,8 @@ const PATH_LIST_SEPARATOR: &str = ";"; mod tests { use std::cmp::Ordering; use std::collections::HashMap; - use std::ffi::{OsStr, OsString}; + use std::ffi::OsString; use std::fs; - use tempfile::tempdir; use crate::layer_env::{Env, LayerEnv, ModificationBehavior, Scope}; @@ -904,66 +903,46 @@ mod tests { assert_eq!(env.get("PKG_CONFIG_PATH"), None); } + // https://github.com/heroku/libcnb.rs/issues/900 #[test] fn layer_paths_come_before_manually_added_paths() { - let temp_dir = tempdir().unwrap(); - let layer_dir = temp_dir.path(); + const TEST_ENV_VALUE: &str = "test-value"; - // https://github.com/heroku/libcnb.rs/issues/900 - fs::create_dir_all(layer_dir.join("bin")).unwrap(); - fs::create_dir_all(layer_dir.join("lib")).unwrap(); - fs::create_dir_all(layer_dir.join("explicit_path")).unwrap(); + let test_cases = [ + ("bin", "PATH", Scope::Build), + ("bin", "PATH", Scope::Launch), + ("lib", "LIBRARY_PATH", Scope::Build), + ]; - // Test Build and Launch PATH - // TODO: Determine desired behavior of Scope::All - for scope in [Scope::Build, Scope::Launch] { - let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); - layer_env.insert(scope.clone(), ModificationBehavior::Delimiter, "PATH", ":"); + for (path, name, scope) in test_cases { + // Construct test layer environment on disk + let temp_dir = tempdir().unwrap(); + let layer_dir = temp_dir.path(); + + let absolute_path = layer_dir.join(path); + fs::create_dir_all(&absolute_path).unwrap(); + + let mut layer_env = LayerEnv::new(); layer_env.insert( scope.clone(), ModificationBehavior::Prepend, - "PATH", - layer_dir.join("explicit_path").as_os_str(), + name, + TEST_ENV_VALUE, ); layer_env.write_to_layer_dir(layer_dir).unwrap(); - let env = layer_env.apply_to_empty(scope.clone()); - assert_eq!( - &[layer_dir.join("explicit_path"), layer_dir.join("bin")] - .map(|dir| dir.as_os_str().to_owned()) - .into_iter() - .collect::>() - .join(OsStr::new(":")), - env.get("PATH").unwrap(), - "PATH was not prepended correctly for scope: `{scope:?}`" - ); - } - // Test Build LIBRARY_PATH - let mut layer_env = LayerEnv::read_from_layer_dir(layer_dir).unwrap(); - layer_env.insert( - Scope::Build, - ModificationBehavior::Delimiter, - "LIBRARY_PATH", - ":", - ); - layer_env.insert( - Scope::Build, - ModificationBehavior::Prepend, - "LIBRARY_PATH", - layer_dir.join("explicit_path").as_os_str(), - ); + // Validate LayerEnv after reading it from disk + let env = LayerEnv::read_from_layer_dir(layer_dir) + .unwrap() + .apply_to_empty(scope); - layer_env.write_to_layer_dir(layer_dir).unwrap(); - let env = layer_env.apply_to_empty(Scope::Build); - assert_eq!( - &[layer_dir.join("explicit_path"), layer_dir.join("lib")] - .map(|dir| dir.as_os_str().to_owned()) - .into_iter() - .collect::>() - .join(OsStr::new(":")), - env.get("LIBRARY_PATH").unwrap() - ); + let mut expected_env_value = OsString::new(); + expected_env_value.push(TEST_ENV_VALUE); + expected_env_value.push(absolute_path.into_os_string()); + + assert_eq!(env.get(name), Some(&expected_env_value)); + } } #[test] From aef08b661609e32bc4efc79d0717e90c23712d2b Mon Sep 17 00:00:00 2001 From: Schneems Date: Fri, 16 May 2025 12:16:35 -0500 Subject: [PATCH 11/12] Emit scope and path on failure Before: ``` thread 'layer_env::tests::layer_paths_come_before_manually_added_paths' panicked at libcnb/src/layer_env.rs:944:13: assertion `left == right` failed left: Some("/var/folders/yr/yytf3z3n3q336f1tj2b2j0gw0000gn/T/.tmpKPAfq0/bin:test-value") right: Some("test-value/var/folders/yr/yytf3z3n3q336f1tj2b2j0gw0000gn/T/.tmpKPAfq0/bin") ``` After: ``` thread 'layer_env::tests::layer_paths_come_before_manually_added_paths' panicked at libcnb/src/layer_env.rs:944:13: assertion `left == right` failed: For ENV var `PATH` scope `Build` left: Some("/var/folders/yr/yytf3z3n3q336f1tj2b2j0gw0000gn/T/.tmpaCmToR/bin:test-value") right: Some("test-value/var/folders/yr/yytf3z3n3q336f1tj2b2j0gw0000gn/T/.tmpaCmToR/bin") note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace ``` --- libcnb/src/layer_env.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index 8a2824b1..5d83d1b5 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -935,13 +935,17 @@ mod tests { // Validate LayerEnv after reading it from disk let env = LayerEnv::read_from_layer_dir(layer_dir) .unwrap() - .apply_to_empty(scope); + .apply_to_empty(scope.clone()); let mut expected_env_value = OsString::new(); expected_env_value.push(TEST_ENV_VALUE); expected_env_value.push(absolute_path.into_os_string()); - assert_eq!(env.get(name), Some(&expected_env_value)); + assert_eq!( + env.get(name), + Some(&expected_env_value), + "For ENV var `{name}` scope `{scope:?}`" + ); } } From 93b6e72db5382984dcc8478552f1c6274a51ddde Mon Sep 17 00:00:00 2001 From: Richard Schneeman Date: Fri, 16 May 2025 12:17:47 -0500 Subject: [PATCH 12/12] Apply suggestions from code review Co-authored-by: Manuel Fuchs Signed-off-by: Richard Schneeman --- CHANGELOG.md | 2 +- libcnb/src/layer_env.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1ab6d6c8..bf638af6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - libcnb: - - Order of automatically applied environment variables by libcnb, such as `PATH=/bin`, now matches the upstream CNB lifecycle (Close [#900](https://github.com/heroku/libcnb.rs/issues/900)). ([#938](https://github.com/heroku/libcnb.rs/pull/938)) + - Order of automatically applied environment variables by libcnb, such as `PATH=/bin`, now matches the upstream CNB lifecycle. ([#938](https://github.com/heroku/libcnb.rs/pull/938)) ## [0.28.1] - 2025-03-25 diff --git a/libcnb/src/layer_env.rs b/libcnb/src/layer_env.rs index 5d83d1b5..1db4672b 100644 --- a/libcnb/src/layer_env.rs +++ b/libcnb/src/layer_env.rs @@ -57,7 +57,7 @@ use std::path::Path; /// /// Some directories in a layer directory are implicitly added to the layer environment if they /// exist. The prime example for this behaviour is the `bin` directory. If it exists, its path will -/// be automatically prepended to `PATH` using the operating systems path delimiter as the delimiter. +/// be automatically added to `PATH` using the operating systems path delimiter as the delimiter. /// /// A full list of these special directories can be found in the /// [Cloud Native Buildpack specification](https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths).