Buildpack API deprecation warning not shown for transitive dependency buildpacks

edmorley commented 7 months ago

Summary

If a buildpack is using a deprecated Buildpack API version, lifecycle outputs a warning like so during detect:

===> DETECTING
Warning: Buildpack 'heroku/nodejs-npm@2.3.0' requests deprecated API '0.6'

However, if the buildpack in question isn't in the top level buildpack group, and instead is a transitive dependency of a composite buildpack, then the warning isn't shown.

It seems that the check here isn't traversing the full buildpack order group graph: https://github.com/buildpacks/lifecycle/blob/dbd27bdef0e60e4145b34d7726cab612b0e922f2/cmd/lifecycle/main.go#L136-L143 https://github.com/buildpacks/lifecycle/blob/dbd27bdef0e60e4145b34d7726cab612b0e922f2/cmd/apis.go#L35-L66

Reproduction

Steps

mkdir testcase && cd $_
pack build test --builder heroku/builder-classic:22 --buildpack heroku/nodejs-npm@2.3.0
pack build test --builder heroku/builder-classic:22 --buildpack heroku/nodejs-function@2.3.0

Current behavior

At step 2 (where the buildpack is referenced directly), a deprecation warning is correctly shown:

===> DETECTING
Warning: Buildpack 'heroku/nodejs-npm@2.3.0' requests deprecated API '0.6'
Timer: Detector started at 2023-11-15T08:45:46Z
======== Results ========
fail: heroku/nodejs-npm@2.3.0

At step 3 (where the buildpack with the deprecated Buildpack API version is instead a transitive dependency of the composite buildpack heroku/nodejs-function), the deprecation warning is missing:

===> DETECTING
Timer: Detector started at 2023-11-15T08:45:18Z
======== Results ========
pass: heroku/nodejs-engine@2.3.0
fail: heroku/nodejs-npm@2.3.0
fail: heroku/nodejs-function-invoker@2.3.0

Expected behavior

For the deprecation warning about Buildpack API version to be shown regardless of whether the buildpack in question is referenced directly, or is a transitive dependency of a composite buildpack.

Context

lifecycle version

0.17.2 (only since 0.18.x has since dropped support for Buildpack API 0.6, meaning there are no deprecated versions left at the moment with which the issue can be demonstrated).

platform version(s)

Pack 0.32.1+git-b14250b.build-5241

anything else?

The buildpacks in question: https://github.com/heroku/buildpacks-nodejs/blob/v2.3.0/buildpacks/npm/buildpack.toml https://github.com/heroku/buildpacks-nodejs/blob/v2.3.0/meta-buildpacks/nodejs-function/buildpack.toml

natalieparellano commented 7 months ago

Thanks for reporting this @edmorley. On a side note, does it make sense for a composite buildpack to declare a Buildpack API version? I guess buildpack.toml has to follow some schema... but still, it feels wrong. Maybe a question for another day...

edmorley commented 7 months ago

Good question. We need some way to version the schema, but using something like schema-version (like project.toml uses) for composite buildpacks would seem a bit strange when component buildpacks don't use schema-version and instead use the Buildpack API version to represent a combination of schema version plus implementation details version.

I suppose even though composite buildpacks don't implement detect/build, the order grouping is still quite Buildpack API spec specific -- if we ever changed how concepts like groups or optional worked, then it would seem like more than just a schema change, so at that point linking that change to an actual Buildpack API version would presumable make sense. As such, perhaps it's fine for composite buildpacks to have an API version?

edmorley commented 4 months ago

This issue is currently in the lifecycle 0.17.3 milestone, but (a) this issue it should probably be in one of the newer milestones, (b) the lifecycle 0.17.3 milestone should probably be closed out, given that version was already released?

Also, ref this issue: I don't think adding a Buildpack API version to a composite buildpack is the correct fix. I think the deprecation check should always iterate through the dependent buildpacks - since there's would be no guarantee that the composite buildpack even uses the same Buildpack API version as it's component buildpack dependents.

buildpacks / lifecycle