Change default PATH delimiter

[schneems]

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]

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

[edmorley]

Thank you for looking into this! I agree this is a footgun that can cause confusion.

Since this PR was opened, it seems upstream are sadly leaning towards wontfixing changing the spec which is unfortunate. I think there is still a chance they might be persuaded otherwise (in particular, they didn't reference the data I had provided showing the impact of the change would be minimal to non-existent), but it does mean there is a chance that if we change libcnb.rs's default here, then it will forever differ from upstream, which might cause confusion in different ways.

As such, I'm wondering if we should instead consider options other than changing to a different default, and instead perhaps make specifying the delimiter mandatory - so buildpack maintainers at least can't forget to set it?

For example, what if we changed the type of ModificationBehavior::{Append,Prepend} so they accepted the delimiter instead?

eg: ModificationBehavior::Append(":")

The API only supports being called once (really .insert() should be called .set() xref #899), so it seems fine that the delimiter would get overwritten by each use of ModificationBehavior::Append(":").

We could then leave the "reading" part as-is (ie if no delimeter file, we default to the empty string) - thereby not breaking compat with the spec there. But for the common case (libcnb.rs doing the writing and reading), that default would never end up used, since the buildpack maintainer will have been forced via the type system/API to have always specified an explicit delimiter.

As an added bonus, if upstream did ever change its default, libcnb.rs-using buildpacks would be isolated from that change, since they would all be setting an explicit delimiter already (even if that delimiter were the empty string).

Lastly, upstream CNB has dropped Windows support, so we can probably remove the Windows path delimiter of ; to simplify things here (perhaps as a separate, initial PR prior to the others?).

Thoughts? :-)

[colincasey]

what if we changed the type of ModificationBehavior::{Append,Prepend} so they accepted the delimiter instead?

Making the delimiter required and explicit seems like a reasonable compromise.

It would also be nice if the APIs for append and prepend modifications took a list of values. Which, now that I think of it, what if we extended your idea @edmorley to lift the name and value parameters of the insert function into each ModificationBehavior enum?

pub enum ModificationBehavior {
    Append { delimiter: OsString, name: OsString, values: Vec<OsString> },
    Default  { name: OsString, value: OsString },
    Delimiter { value: OsString },
    Override { name: OsString, value: OsString },
    Prepend { delimiter: OsString, name: OsString, values: Vec<OsString> },
}