RFC: Replace positional args to Buildpack executables with env vars #190
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,149 @@ | ||
| # Meta | ||
| [meta]: #meta | ||
| - Name: Replace positional args to Buildpack executables with env vars | ||
| - Start Date: 2021-11-18 | ||
| - Author(s): [jkutner](https://github.com/jkutner) | ||
| - Status: Draft Draft | ||
| - RFC Pull Request: (leave blank) | ||
| - CNB Pull Request: (leave blank) | ||
| - CNB Issue: (leave blank) | ||
| - Supersedes: N/A | ||
|
|
||
| # Summary | ||
| [summary]: #summary | ||
|
|
||
| This is a proposal to replace the positional arguments of Buildpack executables with named environment variables. | ||
|
|
||
| # Definitions | ||
| [definitions]: #definitions | ||
|
|
||
| - *positional arguments* - values passed into an executable command such that they are accessible as `$1`, `$2`, etc | ||
|
|
||
| # Motivation | ||
| [motivation]: #motivation | ||
|
|
||
| Postional arguments to Buildpack executables have been the way for the buildpack execution engine to provide inputs for buildpacks since buildpacks v1 was created. However, positional arguments are limiting, and what they represent is not obvious without reading the spec. | ||
|
|
||
| In the spec today they are defined as: | ||
|
|
||
| * `/bin/detect <platform[AR]> <plan[E]>` | ||
| * `/bin/build <layers[EIC]> <platform[AR]> <plan[ER]>` | ||
|
|
||
| Using env vars helps make these inputs more easily accesible from language bindings like [libcnb.bash](https://github.com/jkutner/libcnb.bash). | ||
|
|
||
| # What it is | ||
| [what-it-is]: #what-it-is | ||
|
|
||
| We will deprecate the positional arguments to `bin/detect` and `bin/build` with the following environment variables: | ||
|
|
||
| ### bin/detect | ||
|
|
||
| * `CNB_PLATFORM_DIR` - replaces the first positional argument to `bin/build`. Uses the same env var name as the Platform spec. | ||
|
Contributor
There was a problem hiding this comment. /queue-issue buildpacks/lifecycle "Add CNB_PLATFORM_DIR and CNB_BUILD_PLAN_PATH to bin/detect" |
||
| * `CNB_BUILD_PLAN_PATH` - replaces the second positional argument to `bin/build`. Uses the same env var name as the Platform spec. | ||
|
|
||
| ### bin/build | ||
|
|
||
| * `CNB_LAYERS_DIR` - replaces the first positional argument to `bin/build`. **Note:** Uses the same env var name as the Platform spec, but refers to a different location. | ||
|
Contributor
There was a problem hiding this comment. /queue-issue buildpacks/lifecycle "Add CNB_LAYERS_DIR, CNB_PLATFORM_DIR and CNB_BP_PLAN_PATH to bin/build" |
||
| * `CNB_PLATFORM_DIR` - replaces the second positional argument to `bin/build`. Uses the same env var name as the Platform spec. | ||
| * `CNB_BP_PLAN_PATH` - replaces the third positional argument to `bin/build`. | ||
|
|
||
| # How it Works | ||
| [how-it-works]: #how-it-works | ||
|
|
||
| Provide the environment variables with the same mechanism used to provide `CNB_BUILDPACK_DIR`. For example in [lifecycle's `build.go`](https://github.com/buildpacks/lifecycle/blob/880a801db2d4bfbb39671a66f7aadd96c0231e37/buildpack/build.go): | ||
|
|
||
| ```go | ||
| cmd.Env = append(cmd.Env, EnvPlatformDir+"="+b.Dir) | ||
| ``` | ||
|
|
||
| The positional arguments will be deprecated, but no warnings will be emitted if they are consumed. The lifecycle will continue to provide them to buildpack executable indefinitely, with no plan to remove them. | ||
|
Member
There was a problem hiding this comment. I feel strongly that we should just remove them for buildpacks opting into the new API. |
||
|
|
||
| ## Renaming Platform Env Vars | ||
|
|
||
| Because `CNB_LAYERS_DIR` in the new input vars conflicts with a platform env var name, we intend to rename the platform env var as part of a larger effort to refactor the platform spec. That will be convered in a future RFC. | ||
|
|
||
| The conflict of env var names is strictly an experience problem. Because the env vars are used in different contexts, there is little risk that they will be reused by the internals of lifecycle or pack. | ||
|
|
||
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| - People have been using positional arguments to buildpacks for literally a decade | ||
| - New env var names conflict with platform env vars | ||
|
|
||
| # Alternatives | ||
| [alternatives]: #alternatives | ||
|
|
||
| - Do this but don't deprecate the positional arguments (support both) | ||
|
|
||
| # Prior Art | ||
| [prior-art]: #prior-art | ||
|
|
||
| - Buildpack v1, v2a, & v2b | ||
|
|
||
| # Unresolved Questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| N/A | ||
|
|
||
| # Spec. Changes (OPTIONAL) | ||
| [spec-changes]: #spec-changes | ||
|
|
||
| ## Buildpack spec | ||
|
|
||
| ### Detection | ||
|
|
||
| Executable: `/bin/detect`, Working Dir: `<app[AR]>` | ||
|
|
||
| | Input | Attributes | Description | ||
| |---------------------------|------------|---------------------------------------------- | ||
| | `$0` | | Absolute path of `/bin/detect` executable | ||
| | `$CNB_BUILD_PLAN_PATH` | E | Absolute path to the build plan | ||
| | `$CNB_PLATFORM_DIR` | AR | Absolute path to the platform directory | ||
| | `$CNB_PLATFORM_DIR/env/` | | User-provided environment variables for build | ||
| | `$CNB_PLATFORM_DIR/#` | | Platform-specific extensions | ||
|
|
||
| | Output | Description | ||
| |--------------------|---------------------------------------------- | ||
| | [exit status] | Pass (0), fail (100), or error (1-99, 101+) | ||
| | Standard output | Logs (info) | ||
| | Standard error | Logs (warnings, errors) | ||
| | `$CNB_BUILD_PLAN_PATH` | Contributions to the the Build Plan (TOML) | ||
|
|
||
| ### Build | ||
|
|
||
| Executable: `/bin/build`, Working Dir: `<app[AI]>` | ||
|
|
||
| | Input | Attributes | Description | ||
| |---------------------------|------------|---------------------------------- | ||
| | `$0` | | Absolute path of `/bin/build` executable | ||
| | `$CNB_LAYERS_DIR` | EIC | Absolute path to the buildpack layers directory | ||
| | `$CNB_BP_PLAN_PATH` | ER | Relevant [Buildpack Plan entries](#buildpack-plan-toml) from detection (TOML) | ||
| | `$CNB_PLATFORM_DIR` | AR | Absolute path to the platform directory | ||
| | `$CNB_PLATFORM_DIR/env/` | | User-provided environment variables for build | ||
| | `$CNB_PLATFORM_DIR/#` | | Platform-specific extensions | ||
|
|
||
| | Output | Description | ||
| |------------------------------------------|-------------------------------------- | ||
| | [exit status] | Success (0) or failure (1+) | ||
| | Standard output | Logs (info) | ||
| | Standard error | Logs (warnings, errors) | ||
| | `$CNB_LAYERS_DIR/launch.toml` | App metadata (see [launch.toml](#launchtoml-toml)) | ||
| | `$CNB_LAYERS_DIR/launch.sbom.<ext>` | Launch Software Bill of Materials (see [Software-Bill-of-Materials](#bill-of-materials)) | ||
| | `$CNB_LAYERS_DIR/build.toml` | Build metadata (see [build.toml](#buildtoml-toml)) | ||
| | `$CNB_LAYERS_DIR/build.sbom.<ext>` | Build Software Bill of Materials (see [Software-Bill-of-Materials](#bill-of-materials)) | ||
| | `$CNB_LAYERS_DIR/store.toml` | Persistent metadata (see [store.toml](#storetoml-toml)) | ||
| | `$CNB_LAYERS_DIR/<layer>.toml` | Layer metadata (see [Layer Content Metadata](#layer-content-metadata-toml)) | ||
| | `$CNB_LAYERS_DIR/<layer>.sbom.<ext>` | Layer Software Bill of Materials (see [Software-Bill-of-Materials](#bill-of-materials)) | ||
| | `$CNB_LAYERS_DIR/<layer>/bin/` | Binaries for launch and/or subsequent buildpacks | ||
| | `$CNB_LAYERS_DIR/<layer>/lib/` | Shared libraries for launch and/or subsequent buildpacks | ||
| | `$CNB_LAYERS_DIR/<layer>/profile.d/` | Scripts sourced by Bash before launch | ||
| | `$CNB_LAYERS_DIR/<layer>/profile.d/<process>/` | Scripts sourced by Bash before launch for a particular process type | ||
| | `$CNB_LAYERS_DIR/<layer>/exec.d/` | Executables that provide env vars via the [Exec.d Interface](#execd) before launch | ||
| | `$CNB_LAYERS_DIR/<layer>/exec.d/<process>/` | Executables that provide env vars for a particular process type via the [Exec.d Interface](#execd) before launch | ||
| | `$CNB_LAYERS_DIR/<layer>/include/` | C/C++ headers for subsequent buildpacks | ||
| | `$CNB_LAYERS_DIR/<layer>/pkgconfig/` | Search path for pkg-config for subsequent buildpacks | ||
| | `$CNB_LAYERS_DIR/<layer>/env/` | Env vars for launch and/or subsequent buildpacks | ||
| | `$CNB_LAYERS_DIR/<layer>/env.launch/` | Env vars for launch (after `env`, before `profile.d`) | ||
| | `$CNB_LAYERS_DIR/<layer>/env.launch/<process>/` | Env vars for launch (after `env`, before `profile.d`) for the launched process | ||
| | `$CNB_LAYERS_DIR/<layer>/env.build/` | Env vars for subsequent buildpacks (after `env`) | ||
| | `$CNB_LAYERS_DIR/<layer>/*` | Other content for launch and/or subsequent buildpacks | ||
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel strongly that we should just make a hard cut over between API versions. While this will require buildpack authors to preform migration steps when moving to the new API, it's the only way to keep API version complexity in check and it is consistent with our recent policies/behavior.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I think that's true, this is a pretty big shift change. How many of these kind of changes do we want to make? I strongly feel that proposals like #172 are disruptive and fairly drastic and we can't keep going down this kind of path if we want to keep a community around.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to make this change I think there are two options are
The way I see it the amount of total pain is the same in both cases and delaying the migration until we remove a bunch of deprecated features all at once, isn't necessarily better for buildpack authors. I would feel differently if users had to migrate to upgrade the lifecycle, but they can run the latest lifecycle and stay on the current API as long as they like (until we deprecate and remove the API).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With any breaking change, I think we need to compare the burden of supporting backwards compat with the burden we put on our users/community to migrate. In this case, the burden to migrate can be very high (I would compare it to the addition of the
[types]table in layer toml). At the same time, I don't see how continuing to support the env vars adds "complexity".We only get to make so many breaking changes before people stop upgrading, or leave the community. I think #172 consumes our budget for that tolerance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ekcasey i'm not proposing we remove the positional args at all