Skip to content

Commit db2676d

Browse files
schneemsMalax
andauthored
Fix auto PATH ordering (#938)
* Failing test for #900 * Add a delimiter to the path Missing delimiter strikes again * 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 * Test Build and Launch scope * Expected before actual in assertion * Test LIBRARY_PATH ordering * Changelog * Doc whitespace and headers * 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. * Simplify layer_paths_come_before_manually_added_paths test * 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 ``` * Apply suggestions from code review Co-authored-by: Manuel Fuchs <manuel.fuchs@salesforce.com> Signed-off-by: Richard Schneeman <richard.schneeman+no-recruiters@gmail.com> --------- Signed-off-by: Richard Schneeman <richard.schneeman+no-recruiters@gmail.com> Co-authored-by: Manuel Fuchs <manuel.fuchs@salesforce.com>
1 parent b8d6835 commit db2676d

File tree

2 files changed

+61
-6
lines changed

2 files changed

+61
-6
lines changed

CHANGELOG.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
99

1010
## [Unreleased]
1111

12+
### Fixed
13+
14+
- libcnb:
15+
- Order of automatically applied environment variables by libcnb, such as `PATH=<layer>/bin`, now matches the upstream CNB lifecycle. ([#938](https://github.com/heroku/libcnb.rs/pull/938))
16+
1217

1318
## [0.29.0] - 2025-05-02
1419

libcnb/src/layer_env.rs

Lines changed: 56 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,14 @@ use std::path::Path;
2828
/// logic that uses the build tool to download dependencies. The download process does not need to
2929
/// know the layer name or any of the logic for constructing `PATH`.
3030
///
31-
/// # Applying the delta
31+
/// ## Applying the delta
32+
///
3233
/// `LayerEnv` is not a static set of environment variables, but a delta. Layers can modify existing
3334
/// variables by appending, prepending or setting variables only if they were not already defined. If you only need a
3435
/// static set of environment variables, see [`Env`].
3536
///
3637
/// To apply a `LayerEnv` delta to a given `Env`, use [`LayerEnv::apply`] like so:
38+
///
3739
/// ```
3840
/// use libcnb::layer_env::{LayerEnv, ModificationBehavior, Scope};
3941
/// use libcnb::Env;
@@ -51,16 +53,18 @@ use std::path::Path;
5153
/// assert_eq!(modified_env.get("VAR2").unwrap(), "previous-value");
5254
/// ```
5355
///
54-
/// # Implicit Entries
56+
/// ## Implicit Entries
57+
///
5558
/// Some directories in a layer directory are implicitly added to the layer environment if they
5659
/// exist. The prime example for this behaviour is the `bin` directory. If it exists, its path will
57-
/// be automatically appended to `PATH` using the operating systems path delimiter as the delimiter.
60+
/// be automatically added to `PATH` using the operating systems path delimiter as the delimiter.
5861
///
5962
/// A full list of these special directories can be found in the
6063
/// [Cloud Native Buildpack specification](https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths).
6164
///
6265
/// libcnb supports these, including all precedence and lifecycle rules, when a `LayerEnv` is read
6366
/// from disk:
67+
///
6468
/// ```
6569
/// use libcnb::layer_env::{LayerEnv, Scope};
6670
/// use std::fs;
@@ -139,8 +143,8 @@ impl LayerEnv {
139143
pub fn apply(&self, scope: Scope, env: &Env) -> Env {
140144
let deltas = match scope {
141145
Scope::All => vec![&self.all],
142-
Scope::Build => vec![&self.all, &self.build, &self.layer_paths_build],
143-
Scope::Launch => vec![&self.all, &self.launch, &self.layer_paths_launch],
146+
Scope::Build => vec![&self.layer_paths_build, &self.all, &self.build],
147+
Scope::Launch => vec![&self.layer_paths_launch, &self.all, &self.launch],
144148
Scope::Process(process) => {
145149
let mut process_deltas = vec![&self.all];
146150
if let Some(process_specific_delta) = self.process.get(&process) {
@@ -606,8 +610,8 @@ const PATH_LIST_SEPARATOR: &str = ";";
606610
mod tests {
607611
use std::cmp::Ordering;
608612
use std::collections::HashMap;
613+
use std::ffi::OsString;
609614
use std::fs;
610-
611615
use tempfile::tempdir;
612616

613617
use crate::layer_env::{Env, LayerEnv, ModificationBehavior, Scope};
@@ -899,6 +903,52 @@ mod tests {
899903
assert_eq!(env.get("PKG_CONFIG_PATH"), None);
900904
}
901905

906+
// https://github.com/heroku/libcnb.rs/issues/900
907+
#[test]
908+
fn layer_paths_come_before_manually_added_paths() {
909+
const TEST_ENV_VALUE: &str = "test-value";
910+
911+
let test_cases = [
912+
("bin", "PATH", Scope::Build),
913+
("bin", "PATH", Scope::Launch),
914+
("lib", "LIBRARY_PATH", Scope::Build),
915+
];
916+
917+
for (path, name, scope) in test_cases {
918+
// Construct test layer environment on disk
919+
let temp_dir = tempdir().unwrap();
920+
let layer_dir = temp_dir.path();
921+
922+
let absolute_path = layer_dir.join(path);
923+
fs::create_dir_all(&absolute_path).unwrap();
924+
925+
let mut layer_env = LayerEnv::new();
926+
layer_env.insert(
927+
scope.clone(),
928+
ModificationBehavior::Prepend,
929+
name,
930+
TEST_ENV_VALUE,
931+
);
932+
933+
layer_env.write_to_layer_dir(layer_dir).unwrap();
934+
935+
// Validate LayerEnv after reading it from disk
936+
let env = LayerEnv::read_from_layer_dir(layer_dir)
937+
.unwrap()
938+
.apply_to_empty(scope.clone());
939+
940+
let mut expected_env_value = OsString::new();
941+
expected_env_value.push(TEST_ENV_VALUE);
942+
expected_env_value.push(absolute_path.into_os_string());
943+
944+
assert_eq!(
945+
env.get(name),
946+
Some(&expected_env_value),
947+
"For ENV var `{name}` scope `{scope:?}`"
948+
);
949+
}
950+
}
951+
902952
#[test]
903953
fn read_from_layer_dir_layer_paths_build() {
904954
let temp_dir = tempdir().unwrap();

0 commit comments

Comments
 (0)