Skip to content
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

Distribution Specification #53

Merged
merged 12 commits into from
Aug 6, 2019
Merged

Distribution Specification #53

merged 12 commits into from
Aug 6, 2019

Conversation

sclevine
Copy link
Member

@sclevine sclevine commented Jul 11, 2019

This PR makes the spec compatible with buildpacks/rfcs#11.

Implementation PR: buildpacks/lifecycle#149

@sclevine sclevine requested review from jkutner, ekcasey, hone and nebhale and removed request for jkutner, hone, nebhale and ekcasey July 11, 2019 05:22
@sclevine sclevine changed the title Distribution Specification - Initial adjustments Distribution Specification Jul 12, 2019
@sclevine
Copy link
Member Author

@hone @jkutner @nebhale awaiting approval

Copy link
Contributor

@nebhale nebhale left a comment

Choose a reason for hiding this comment

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

No need for the buildpack identity as env-vars any longer.

buildpack.md Outdated
@@ -639,6 +647,17 @@ In either case,
| `CPATH` | `/include` | header files | [x] |
| `PKG_CONFIG_PATH` | `/pkgconfig` | pc files | [x] |

#### Buildpack Details
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm up for removing this now that each buildpack knows who it is.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we just keep BP_DIR?

Copy link
Contributor

Choose a reason for hiding this comment

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

We've been able to determine this to date without an environment variable, so I'd say continue with the fewest environment variables possible.

Copy link
Member Author

@sclevine sclevine Jul 26, 2019

Choose a reason for hiding this comment

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

Removed from this PR.

distribution.md Outdated
A symlink MUST be created for each buildpack version that the blob is assembled to support.

```
/cnb/blobs/<sha256 checksum of buildpack blob tgz>/
Copy link
Contributor

Choose a reason for hiding this comment

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

Just registering my belief that this isn't necessary.

Non-binding

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

distribution.md Outdated

A buildpack defined within `buildpack.toml` MUST either be:

1. A buildpack implementation specified by a `stacks` field or
Copy link
Contributor

Choose a reason for hiding this comment

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

"specified by" almost makes it sound like the stack value will somehow point to the buildpack implementation. Wonder if there's a better wording here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe: "denoted by the presence of a stacks field" or something like that?

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

- MUST NOT be `config` or `app`.
- MUST NOT be identical to any other buildpack ID when using a case-insensitive comparison.

Stack authors MUST choose a globally unique ID, for example: "io.buildpacks.mystack".
Copy link
Contributor

Choose a reason for hiding this comment

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

"globally unique" might be a tad confusing given that we encourage users to re-use a given stack ID to denote ABI compatibility. Wonder if there's a way to expand on that a bit here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We ran into a similar issue when documenting the basics of a custom stack. You can see what we did here: https://buildpacks.io/docs/using-pack/stacks/#creating-custom-stacks

Copy link
Member Author

Choose a reason for hiding this comment

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

A stack is a contract, not a specific set of images. The author of that contract must choose a globally unique ID that those creating stack images following that contract can use.

I think we should change the wording from "custom stack" to "stack images" in the docs. (The platform spec should probably also be clarified.)

distribution.md Outdated

A buildpack reference in `order` MUST either be:

1. A reference to a buildpack inside of the blob using a `path` field ,
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra space before comma

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

distribution.md Outdated

If an order is specified, then `stacks` MUST not be specified.

A buildpack reference inside of a `group` MUST either contain an `id` and `version` or a relative `path`.
Copy link
Contributor

@ameyer-pivotal ameyer-pivotal Jul 30, 2019

Choose a reason for hiding this comment

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

Relative to what? Buildpack.toml location? Could be helpful to clarify.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an upper bound on how far up the relative path can go? I'm guessing it can't go outside the buildpack, and while that seems obvious, it would be good to include a MUST NOT clause.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed this part of the spec.

### buildpack.toml (TOML)

```toml
[buildpack]
Copy link
Member

Choose a reason for hiding this comment

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

In order to better determine compatibly between schema changes we should have a schema version associated with every spec'd toml file. It would certainly help in the current migration for buildpack.toml but would be equally helpful in the future for other toml files.

We propose this version be 2 for builder.toml so we could assume version 1 for the previous version without this new property.

proposal:

schema = "2"

CC: @ameyer-pivotal @ekcasey

Copy link
Member

Choose a reason for hiding this comment

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

More examples with some other options:

schema = "2"

[buildpack]
id = "<buildpack ID>"
name = "<buildpack name>"
version = "<buildpack version>"

or

version = "2"

[buildpack]
id = "<buildpack ID>"
name = "<buildpack name>"
version = "<buildpack version>"

or

schema-version = "2"

[buildpack]
id = "<buildpack ID>"
name = "<buildpack name>"
version = "<buildpack version>"

Copy link
Contributor

@ameyer-pivotal ameyer-pivotal Aug 1, 2019

Choose a reason for hiding this comment

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

I really like the idea of a schema version. I strongly vote for schema-version as the most clear key name. If I had to bend, schema would be my next choice.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's move this to a different PR.

Copy link
Member

Choose a reason for hiding this comment

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

@jromero would love to see an RFC for this!

Copy link
Member Author

Choose a reason for hiding this comment

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

@jkutner worth noting that this additional RFC will delay the next lifecycle release significantly. We will likely need to fork the lifecycle to include these changes for the CF builder, and run ahead of the spec.

Copy link
Contributor

Choose a reason for hiding this comment

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

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
@sclevine
Copy link
Member Author

sclevine commented Aug 4, 2019

@hone @jkutner @nebhale removed path as discussed. Ready for approval.

(Note that this PR also contains some fixes to the Build Plan formatting in buildpack.md. Happy to pull that out if desired.)

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
@nebhale nebhale self-requested a review August 5, 2019 21:08
6. [Layer Content Metadata (TOML)](#layer-content-metadata-toml)
11. [Data Format](#data-format)
1. [launch.toml (TOML)](#launch.toml-toml)
2. [Build Plan (TOML)](#build-plan-toml)
Copy link
Member

Choose a reason for hiding this comment

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

there's a build plan and a buildpack plan?

Copy link
Member Author

Choose a reason for hiding this comment

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

The input into /bin/build is just the requires (provided to the buildpacks that said they'd provide them), while the output of /bin/detect is both requires and provides. Ben pointed out that the spec is currently incorrect and confusing because there's only one format mentioned. For now, I just renamed the stuff that goes into /bin/build "buildpack plan" as an improvement over the current version.

Copy link
Member

Choose a reason for hiding this comment

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

@sclevine but the schema shown below looks quite different from what you described. I'm not really clear on what [[entries]] is used for.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just the name of the array. The contents are the same as requires coming out of /bin/detect. I thought it would be the same format as the BOM, but the BOM also includes the buildpack list, so I split it out into a new format section.

@jkutner jkutner self-requested a review August 6, 2019 20:38
buildpack.md Show resolved Hide resolved
@hone
Copy link
Member

hone commented Aug 6, 2019

@sclevine should we specify/describe what <plan> is during bin/detect?

buildpack.md Outdated
11. [Data Format](#data-format)
1. [launch.toml (TOML)](#launch.toml-toml)
2. [Build Plan (TOML)](#build-plan-toml)
2. [Buildpack Plan (TOML)](#buildpack-plan-toml)
Copy link
Member

Choose a reason for hiding this comment

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

Just pedantic, but can we just update the numbering so this is 3+.

Signed-off-by: Stephen Levine <stephen.levine@gmail.com>
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.

7 participants