Skip to content

Change default PATH delimiter #945

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
schneems wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

Change default PATH delimiter #945

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

- `libcnb`
- The default delimiter for PATH based `LayerEnv` operations is now colon (`:`) for unix and semi-colon (`;`) for windows rather than an empty string. Developers relying on this behavior should use an explicit empty delimiter.
This value is subject to change if the [upstream spec changes the default value](https://github.com/buildpacks/spec/issues/285). ([#945](https://github.com/heroku/libcnb.rs/pull/945))

## [0.29.0] - 2025-05-02

Expand Down
16 changes: 10 additions & 6 deletions libcnb/src/layer_env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use std::path::Path;
/// 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;
Expand All @@ -47,7 +48,7 @@ use std::path::Path;
/// env.insert("VAR2", "previous-value");
///
/// let modified_env = layer_env.apply(Scope::Build, &env);
/// assert_eq!(modified_env.get("VAR").unwrap(), "foobar");
/// assert_eq!(modified_env.get("VAR").unwrap(), "foo:bar");
/// assert_eq!(modified_env.get("VAR2").unwrap(), "previous-value");
/// ```
///
Expand Down Expand Up @@ -132,7 +133,7 @@ impl LayerEnv {
/// env.insert("VAR2", "previous-value");
///
/// let modified_env = layer_env.apply(Scope::Build, &env);
/// assert_eq!(modified_env.get("VAR").unwrap(), "foobar");
/// assert_eq!(modified_env.get("VAR").unwrap(), "foo:bar");
/// assert_eq!(modified_env.get("VAR2").unwrap(), "previous-value");
/// ```
#[must_use]
Expand Down Expand Up @@ -482,7 +483,7 @@ impl LayerEnvDelta {
self.entries
.get(&(ModificationBehavior::Delimiter, key.into()))
.cloned()
.unwrap_or_default()
.unwrap_or(OsString::from(PATH_LIST_SEPARATOR))
Copy link
Copy Markdown
Contributor Author

@schneems schneems May 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem - This code is used in two ways: Writing env var changes to CNB layers and reading env var changes to CNB layers.

We can make assumptions or change defaults about the writing because we "own" the interface and expectations there. However, we cannot change expectations about reading as we don't know who wrote it. We could work through/around this decoupling this logic somehow either on write or read.

For example, on read if the delim doesn't exist, deserialize that as an explicit "" (empty string) until the upstream spec is updated, and never write a layer without a .delim (by defaulting to ;).

}

fn read_from_env_dir(path: impl AsRef<Path>) -> Result<Self, std::io::Error> {
Expand Down Expand Up @@ -614,8 +615,11 @@ mod tests {

use super::LayerEnvDelta;

/// Direct port of a test from the reference lifecycle implementation:
/// Port of a test from the reference lifecycle implementation:
/// See: <https://github.com/buildpacks/lifecycle/blob/a7428a55c2a14d8a37e84285b95dc63192e3264e/env/env_test.go#L105-L154>
///
/// Modifications:
/// - Default delimiter is now based on OS instead of an empty string
#[test]
fn reference_impl_env_files_have_a_suffix_it_performs_the_matching_action() {
let temp_dir = tempdir().unwrap();
Expand Down Expand Up @@ -660,7 +664,7 @@ mod tests {

assert_eq!(
vec![
("VAR_APPEND", "value-append-origvalue-append"),
("VAR_APPEND", "value-append-orig:value-append"),
(
"VAR_APPEND_DELIM",
"value-append-delim-orig[]value-append-delim"
Expand All @@ -671,7 +675,7 @@ mod tests {
("VAR_DEFAULT_NEW", "value-default"),
("VAR_OVERRIDE", "value-override"),
("VAR_OVERRIDE_NEW", "value-override"),
("VAR_PREPEND", "value-prependvalue-prepend-orig"),
("VAR_PREPEND", "value-prepend:value-prepend-orig"),
(
"VAR_PREPEND_DELIM",
"value-prepend-delim[]value-prepend-delim-orig"
Expand Down
Loading