Decouple BOM and build plan #139
Conversation
* Resolves #133 * Implements RFC 0053 - https://github.com/buildpacks/rfcs/blob/main/text/0053-decouple-buildpack-plan-and-bom.md Signed-off-by: Emily Casey <ecasey@vmware.com>
* other minor style fixes Signed-off-by: Emily Casey <ecasey@vmware.com>
| - MAY log output from the build process to `stdout`. | ||
| - MAY emit error, warning, or debug messages to `stderr`. | ||
| - MAY write a list of possible commands for launch to `<layers>/launch.toml`. | ||
| - MAY write a list of sub-paths within `<app>` to `<layers>/launch.toml`. | ||
| - SHOULD write BOM (Bill-of-Materials) entries to `<layers>/launch.toml` describing any contributions to the app image. | ||
| - SHOULD write build BOM entries to `<layers>/build.toml` describing any contributions to the build environment. |
There was a problem hiding this comment.
Is there any convention on the statements' order?
Maybe it's worth writing all of "SHOULD" statements and then the "MAY" statements of each file.
There was a problem hiding this comment.
I like that idea but line 436, - MAY read the Buildpack Plan., gives me pause. It feels strange to put that line after - SHOULD write a list containing any unmet Buildpack Plan entries...
There was a problem hiding this comment.
Maybe we can put line 437 after line 443. Then all of the statements that relate to build.toml will be together.
|
|
||
| A buildpack MAY add extra entries to `<plan>` that do not correspond to entries added during the detection phase. | ||
| For each entry in `<plan>`: | ||
| - **If** there is an unmet entry in `build.toml` with a matching `name`, the lifecycle |
There was a problem hiding this comment.
I originally thought IF was the convention before @sclevine corrected it in the platform spec rewrite #87 (comment)
I think the correct thing to do here is fix every other IF in this document to match this one.
There was a problem hiding this comment.
No problem. I've just saw other IF in this document so I thought this is the convention.
I don't mind whether the convention is If or IF :)
| For each entry in `<plan>`: | ||
| - **If** there is an unmet entry in `build.toml` with a matching `name`, the lifecycle | ||
| - MUST include the entry in the `<plan>` of the next buildpack that provided an entry with that name during the detection phase. | ||
| - **Else**, the lifecycle |
buildpack.md
Outdated
|
|
||
| For each dependency contributed to the app image, the buildpack: | ||
|
|
||
| - SHOULD add a bill-of-materials entry to `bom` describing the dependency, where: |
There was a problem hiding this comment.
Maybe it'll be clearer to write:
- SHOULD add a bill-of-materials entry to the `bom` section describing...
although it's also clear the way it is right now.
There was a problem hiding this comment.
I think your suggested wording improves readability but I have doubts about whether "section" is the technically correct word. How do you feel about "array" instead of "section"?
buildpack.md
Outdated
| ``` | ||
|
|
||
| For each dependency contributed by the buildpack to the build environment, the buildpack: | ||
| - SHOULD add a bill-of-materials entry to `bom` describing the dependency, where: |
There was a problem hiding this comment.
The same comment as above.
Again, it's also clear the way it is right now, so maybe it's only me that think that it will be clearer with the small change.
| @@ -778,14 +807,29 @@ The lifecycle MUST accept slices that do not contain any files or directory. How | |||
|
|
|||
| The lifecycle MUST include all unmatched files in the app directory in any number of additional layers in the OCI image. | |||
|
|
|||
| For each label, the buildpack: | |||
| ### build.toml (TOML) | |||
There was a problem hiding this comment.
I don't see where we mention that the build.toml from each buildpack will be aggregated by the lifecycle into report.toml. Is that also spec'd?
There was a problem hiding this comment.
The report.toml changes should be added to the platform API in a different PR. This PR is strictly for the buildpack API.
Signed-off-by: Emily Casey <ecasey@vmware.com> Co-authored-by: Natalie Arellano <narellano@vmware.com>
* https://tools.ietf.org/html/rfc2119 Signed-off-by: Emily Casey <ecasey@vmware.com>
| @@ -894,7 +938,7 @@ If an `order` is specified, then `stacks` MUST NOT be specified. | |||
| *Key: `api = "<buildpack API version>"`* | |||
| - MUST be in form `<major>.<minor>` or `<major>`, where `<major>` is equivalent to `<major>.0` | |||
| - MUST describe the implemented buildpack API. | |||
| - SHOULD indicate the lowest compatible `<minor>` IF buildpack behavior is consistent with multiple `<minor>` versions of a given `<major>` | |||
| - SHOULD indicate the lowest compatible `<minor>` if buildpack behavior is consistent with multiple `<minor>` versions of a given `<major>` | |||
There was a problem hiding this comment.
Is there a rhyme or reason to when we choose to bold these or not?
There was a problem hiding this comment.
That's a good question. I didn't bold this one b/c it wasn't bold before. I could reverse engineer the rational (not at the beginning of the sentence something something...) but I would just be making things up. Maybe @sclevine can articulate the rule here.
Signed-off-by: Emily Casey <ecasey@vmware.com>
Signed-off-by: Emily Casey <ecasey@vmware.com>
buildpack.md
Outdated
| - MAY augment the Buildpack Plan with more refined metadata. | ||
| - MAY remove entries with duplicate names in the Buildpack Plan to refine the metadata. | ||
| - MAY remove all entries of the same name from the Buildpack Plan to defer those entries to subsequent `/bin/build` executables. | ||
| - SHOULD write a list containing any unmet Buildpack Plan entries to `<layers>/build.toml` to defer those entries to subsequent `/bin/build` executables. |
There was a problem hiding this comment.
I'm not sure if we define unmet anywhere in the spec? We explain what happens to unmet entries below, but might be good to define this term before it's used.
There was a problem hiding this comment.
Let me know if the changes in this commit address your concern.
Related: I think the Buildpack API would generally benefit from a terminology section but that change is probably out of scope for this PR. I created an issue for this change - #145
* Adds launch/build bom entries to typical buildpack behavior Signed-off-by: Emily Casey <ecasey@vmware.com>
Resolves #133