Change default PATH delimiter #945
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
719422d to
4144bb9
Compare
schneems
commented
May 16, 2025
| @@ -482,7 +483,7 @@ impl LayerEnvDelta { | |||
| self.entries | |||
| .get(&(ModificationBehavior::Delimiter, key.into())) | |||
| .cloned() | |||
| .unwrap_or_default() | |||
| .unwrap_or(OsString::from(PATH_LIST_SEPARATOR)) | |||
There was a problem hiding this comment.
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
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.