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

RFC: Contractual Build Plan #12

Merged
merged 4 commits into from Jul 10, 2019
Merged

RFC: Contractual Build Plan #12

merged 4 commits into from Jul 10, 2019

Conversation

sclevine
Copy link
Member

@sclevine sclevine commented Jun 13, 2019

@sclevine
Copy link
Member Author

@nebhale Example for Java: [openjdk, build-system, jvm-app]

openjdk

provides: openjdk

build-system:

requires: openjdk
provides: jar

jvm-app

requires: jar, openjdk
provides: jar # output only if app dir is originally jar, otherwise build-system providing jar is necessary

@nebhale
Copy link
Contributor

nebhale commented Jun 13, 2019

Are requires merged in some way? If there are multiple buildpacks requiring something, with different metadata let's say, does the providing buildpack get a single element or all of the elements?

@sclevine
Copy link
Member Author

sclevine commented Jun 13, 2019

Each value in the build plan is now a list of requires (note [[nodejs]] instead of [nodejs]) in the order they are requested.

@tylerphelan
Copy link

I think this would help making build plan detect -> build interfaces a lot!

nebhale
nebhale previously approved these changes Jun 17, 2019
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.

I don't think that this is a hard requirement in order to enable buildpacks (we've got dozens of buildpacks that solve these same issues with idiomatic behavior rather than contract), but it doesn't seem to hurt.

Actually, I'm thinking there might be a problem. Let me work through it.

@nebhale nebhale self-requested a review June 17, 2019 17:27
jkutner
jkutner previously approved these changes Jun 18, 2019
@jkutner
Copy link
Member

jkutner commented Jun 19, 2019

Am I correct that the lifecycle would resolve the requires versus provides? That is, the lifecycle ensures that all required elements have been provided by something else?

@sclevine
Copy link
Member Author

sclevine commented Jun 19, 2019

Am I correct that the lifecycle would resolve the requires versus provides? That is, the lifecycle ensures that all required elements have been provided by something else?

That's correct -- it ensures that all required elements are provided at least once and all provided elements are required at least once. However, if an optional buildpack prevents that from holding true, non-optional buildpacks can still be considered without it.

It's worth noting that a buildpack can both provide and require the same element (and I expect this to be very common).

@jkutner
Copy link
Member

jkutner commented Jun 19, 2019

Oh I was not expected that all provided keys must be required.

How would we handle the case where a node-engine-cnb that provides “node” is used on its own?

@sclevine
Copy link
Member Author

sclevine commented Jun 19, 2019

In the CF case, node-engine-cnb would always provide node, but only require it if the app explicitly asked for it (via buildpack.yml). That means when the buildpack is non-optional, something (either app or buildpack) has to need node for the group to be considered. When the buildpack is optional, the buildpack is removed from consideration if nothing actually needs node.

This removes the current concept of "vacuous pass" (which is confusing and results in misleading metadata).

@jkutner jkutner self-requested a review June 19, 2019 13:25
@jkutner
Copy link
Member

jkutner commented Jun 19, 2019

but only require it if the app explicitly asked for it (via buildpack.yml).

Does that mean, in the CF case, that the buildpack needs to inspect the buildpack.toml to determine if app explicitly asked for node-engine-cnb?

@sclevine
Copy link
Member Author

buildpack.yml is different from buildpack.toml. The former is app configuration. If you don't have a package.json, or if you're not using the npm-cnb or yarn-cnb, you need to ask for node in the buildpack.yml to have the node-engine-cnb install it.

@sclevine sclevine dismissed stale reviews from jkutner and nebhale June 19, 2019 14:02

re-requested

@sclevine
Copy link
Member Author

To clarify: a buildpack can always provide a dependency without another buildpack requiring it by both providing and requiring the dependency. Asserting that required dependencies must be provided and vice-versa provides the most flexibility.

@nebhale
Copy link
Contributor

nebhale commented Jun 21, 2019

Reading over this, I think there's still a shortcoming where a single buildpack cannot do an "or" requirement as in, "I'll contribute if either nodejs or java is provided".

@sclevine
Copy link
Member Author

I modified the proposal so that dependencies must be provided in the current buildpack or a previous buildpack if they are required.

I also addressed Ben's question.

@sclevine sclevine changed the title RFC: Contractual Build Plan [WIP] RFC: Contractual Build Plan Jun 22, 2019
@sclevine
Copy link
Member Author

sclevine commented Jun 23, 2019

@nebhale To make it as clear as possible that, in combination with the Distribution RFC, the “requires jvm-app or node” case works with a single buildpack implementation (using a single /bin/detect and /bin/build at the repo root), here’s an example. Note that this only works with a change I just made to the Distribution RFC for this use case: the buildpack ID is now exposed as BP_ID.

buildpack.toml:

[[buildpacks]]
id = "com.example.myapm"
name = "APM Buildpack"
version = "0.0.9"
[[buildpacks.order]]
group = [
   { id = "com.example.myapm.node", version = "0.0.7" }
]
[[buildpacks.order]]
group = [
   { id = "com.example.myapm.java", version = "0.0.5" }
]
 [[buildpacks]]
id = "com.example.myapm.node"
name = "APM Buildpack for Node"
version = "0.0.7"
path = "."
[buildpacks.metadata]
# ...
[[buildpacks.stacks]]
id = "io.buildpacks.stacks.bionic"

 [[buildpacks]]
id = "com.example.myapm.java"
name = "APM Buildpack for Java"
version = "0.0.5"
path = "." # same path!
[buildpacks.metadata]
# ...
[[buildpacks.stacks]]
id = "io.buildpacks.stacks.bionic"

Repo:

buildpack.toml
/bin/detect
/bin/build

Where /bin/detect and /bin/build use the value of BP_ID to determine how they behave.

Note that the java and node buildpack definitions are in different groups to take advantage of vertical expansion.


N/A

# Drawbacks
Copy link
Member

Choose a reason for hiding this comment

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

afaict this is also a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, huge breaking change.

Copy link
Member Author

Choose a reason for hiding this comment

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

Noted below.

@hone
Copy link
Member

hone commented Jul 10, 2019

In the Heroku buildpack ecosystem, the common case is that a buildpack would require the things it provides. Is it worth having a special key for it vs having every buildpack write each entry twice?

Copy link
Member

@hone hone left a comment

Choose a reason for hiding this comment

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

From the spec WG meeting today, we're going to add another RFC for the case that a buildpack does requires and provides in the buildplan for the same key. Some suggestions was a new [[]] toml array or a provide = true key in the release Array.

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

Merging with 4/4 approval from project owners. FCP closes today due to unanimous approval.

@sclevine sclevine merged commit 10d03d9 into master Jul 10, 2019
@jromero jromero added this to Done in Planning Board Jul 12, 2019
@jromero jromero removed this from Done in Planning Board Jul 12, 2019
@jromero jromero added this to In Progress in Planning Board Jul 12, 2019
@jromero jromero moved this from In Progress to Done in Planning Board Jul 15, 2019
@sclevine sclevine removed this from Done in Planning Board Jul 16, 2019
nebhale added a commit that referenced this pull request Aug 22, 2019
Signed-off-by: Ben Hale <bhale@pivotal.io>
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.

None yet

5 participants