8000 Change default PATH delimiter by schneems · Pull Request #945 · heroku/libcnb.rs · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
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.

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

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

Conversation

schneems
Copy link
Contributor

The default PATH delimiter is `` (empty string) rather than the OS delimiter. This has caused many issues and heartache buildpacks/spec#285.

The upstream spec does not state the delimiter that should be used when none is provided. This change suggests that we should default to : for unix and ; for windows. This can be seen as a dry-run for the spec proposal change buildpacks/rfcs#326. If this change can be made with little to no reported problems, we can send that information upstream.

Other suggestions have been floated #534. I suggest that we prioritize the most common case, which for Heroku buildpacks is using the OS path separator.

The default PATH delimiter is `` (empty string) rather than the OS delimiter. This has caused many issues and heartache buildpacks/spec#285.

The upstream spec does not state the delimiter that should be used when none is provided. This change suggests that we should default to `:` for unix and `;` for windows. This can be seen as a dry-run for the spec proposal change buildpacks/rfcs#326. If this change can be made with little to no reported problems, we can send that information upstream.

Other suggestions have been floated #534. I suggest that we prioritize the most common case, which for Heroku buildpacks is using the OS path separator.
@schneems schneems force-pushed the schneems/different-default-delimiter branch from 719422d to 4144bb9 Compare May 13, 2025 23:29
@schneems schneems marked this pull request as ready for review May 13, 2025 23:35
@schneems schneems requested a review from a team as a code owner May 13, 2025 23:35
@@ -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
Contributor Author

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 ;).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant
0