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

Move to buildpack API 0.10 #773

Merged
merged 18 commits into from
Feb 12, 2024
Merged

Move to buildpack API 0.10 #773

merged 18 commits into from
Feb 12, 2024

Conversation

Malax
Copy link
Member

@Malax Malax commented Feb 6, 2024

Buildpack API changes

Stack and Mixin Removal (RFC 096, Spec PR)

The stack field is sill in the spec as a deprecated field. I decided to not keep it around for libcnb. libcnb always just supported one specific version and when moving to the new buildpack API, uses should fully migrate to the target concept. This simplifies the implementation greatly (fewer permutations). If a blocker emerges, we can add support for deprecated stacks later down the line.

Open Questions

  • Are targets and order mutually exclusive as stack and order were? (slack question)

This seems to be an oversight in the spec. pack does not allow for this (code) and it seems like a to big of a change to silently make. Semantics of this are also unclear. I added code comments to the respective areas to clarify why libcnb.rs differs from the spec. I'm in contact with the maintainers to fix this spec issue upstream.

Unimplemented Buildpack API changes

Runtime Base Image Extensions (RFC 105, Spec PR)

Will be implemented at a later time. The spec is marked as experimental and the feature is basically a new kind of buildpack that shares some of the spec for regular buildpacks, but with slight differences. See the image extension spec for details. Tracking issue for this feature: #776

Build Image Environment Variables (RFC 109, Spec PR)

This does not affect the implementation in libcnb.rs. These new environment variables will always be set (regardless of the clear-env value and cannot be read manually from a directory. This is different from $CNB_PLATFORM_DIR/env/. However, this might affect buildpacks in other ways. For example, If a buildpack choses to set clear-env to true to ensure that no Maven specific environment variables are set that might mess with buildpack execution, build image environment variables can now mess with this assumption.

However, there are many other ways environment variables can end up in the buildpack's executable environment (from outside of $CNB_PLATFORM_DIR/env/). Buildpack authors should not rely on clear-env to avoid clobbering of important environment variables in any case.

Notes for reviewers

  • A lot of stack = "*" occurrences in tests and docs have been removed. Having no target specified in buildpack.toml is equivalent as it matches any target.

Closes #772.

@Malax Malax added this to the Buildpack API 0.10 milestone Feb 6, 2024
README.md Outdated Show resolved Hide resolved
@Malax Malax force-pushed the malax/cnb0.10 branch 4 times, most recently from bd8ef55 to 812a4be Compare February 7, 2024 11:54
@Malax Malax force-pushed the malax/cnb0.10 branch 8 times, most recently from 6fb3ec4 to eed004f Compare February 9, 2024 10:09
@Malax Malax force-pushed the malax/cnb0.10 branch 3 times, most recently from a50c14a to e237291 Compare February 9, 2024 10:50
@Malax Malax force-pushed the malax/cnb0.10 branch 2 times, most recently from 182c76e to 8bf0ff3 Compare February 9, 2024 10:53
@edmorley
Copy link
Member

edmorley commented Feb 9, 2024

Build Image Environment Variables (RFC 109, buildpacks/spec#349)

This does not affect the implementation in libcnb.rs. These new environment variables will always be set (regardless of the clear-env value and cannot be read manually from a directory. This is different from $CNB_PLATFORM_DIR/env/.

Is this correct? Iirc there was a bit of uncertainty around this (inconsistency between the rfc, spec and implementation) - though it's been so long I can't quite remember the outcome. See:
buildpacks/spec#330 (comment)
buildpacks/spec#367

(Either way, we would want to add support to libcnb in a separate PR, so a tracking issue in the backlog to untangle that uncertainty would be fine)

@Malax Malax marked this pull request as ready for review February 9, 2024 11:07
@Malax Malax requested a review from a team as a code owner February 9, 2024 11:07
@Malax
Copy link
Member Author

Malax commented Feb 9, 2024

@edmorley:

Is this correct?

The buildpack API spec 0.10 says:

During the detection and build phases, the lifecycle MUST provide as environment variables any operator-provided files in <build-config>/env with environment variable names and contents matching the file names and contents. This applies for all values of clear-env or if clear-env is undefined in buildpack.toml.

Which is quite clear IMO. The <build-config> placeholder is only mentioned once (not defined, nor re-used) in the Buildpack API spec and there is no environment variable for it like for the other directories such as CNB_PLATFORM_DIR or CNB_LAYERS_DIR. I interpret this as the strong intent for the "operator defined environment variables" to be magic from the buildpack POV.

The PR that implements this feature into lifecycle does not seem to make any attempt to expose build config environments separately. The current implementation in main isn't different in that regard, platform env and build config env are merged together: https://github.com/buildpacks/lifecycle/blob/751c7a2737be2e6a8f66bfcc2e0cf0058661d6ff/env/env.go#L81-L112

With the Buildpack API spec and lifecycle implementation agreeing, there doesn't seem to be any follow-up required for libcnb.rs here. If anyone has contradicting information I'm happy to stand corrected! :)

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for tackling this!

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
examples/basics/src/main.rs Outdated Show resolved Hide resolved
libcnb-data/src/buildpack/mod.rs Outdated Show resolved Hide resolved
libcnb-data/src/buildpack/mod.rs Outdated Show resolved Hide resolved
libcnb-data/src/buildpack/mod.rs Show resolved Hide resolved
libcnb-data/src/buildpack/target.rs Show resolved Hide resolved
libcnb-data/src/buildpack/target.rs Outdated Show resolved Hide resolved
libcnb/src/runtime.rs Outdated Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
@Malax Malax force-pushed the malax/cnb0.10 branch 2 times, most recently from 55c77c6 to 0104524 Compare February 12, 2024 10:12
@Malax Malax requested a review from edmorley February 12, 2024 10:44
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

🎉

libcnb/src/target.rs Outdated Show resolved Hide resolved
libcnb/src/target.rs Outdated Show resolved Hide resolved
libcnb/src/build.rs Outdated Show resolved Hide resolved
@Malax Malax merged commit 033f612 into main Feb 12, 2024
4 checks passed
@Malax Malax deleted the malax/cnb0.10 branch February 12, 2024 11:50
schneems added a commit to heroku/buildpacks-ruby that referenced this pull request May 15, 2024
* Bump the libcnb group across 1 directory with 3 updates

Bumps the libcnb group with 3 updates in the / directory: [libcnb](https://github.com/heroku/libcnb.rs), [libherokubuildpack](https://github.com/heroku/libcnb.rs) and [libcnb-test](https://github.com/heroku/libcnb.rs).


Updates `libcnb` from 0.17.0 to 0.19.0
- [Release notes](https://github.com/heroku/libcnb.rs/releases)
- [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md)
- [Commits](heroku/libcnb.rs@v0.17.0...v0.19.0)

Updates `libherokubuildpack` from 0.17.0 to 0.21.0
- [Release notes](https://github.com/heroku/libcnb.rs/releases)
- [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md)
- [Commits](heroku/libcnb.rs@v0.17.0...v0.21.0)

Updates `libcnb-test` from 0.17.0 to 0.19.0
- [Release notes](https://github.com/heroku/libcnb.rs/releases)
- [Changelog](https://github.com/heroku/libcnb.rs/blob/main/CHANGELOG.md)
- [Commits](heroku/libcnb.rs@v0.17.0...v0.19.0)

---
updated-dependencies:
- dependency-name: libcnb
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: libcnb
- dependency-name: libherokubuildpack
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: libcnb
- dependency-name: libcnb-test
  dependency-type: direct:production
  update-type: version-update:semver-minor
  dependency-group: libcnb
...

Signed-off-by: dependabot[bot] <[email protected]>

* Change to mutable layers

The layer trait interface was changed to mutable in heroku/libcnb.rs#669

* Switch from stacks to targets AKA "Stay on target"

Buildpack API 0.10 removed the concept of stacks in favor of targets heroku/libcnb.rs#773.

This commit works to upgrade applications in place by migrating metadata to support the new serialization format. This is supported by implementing TryMigrate from the `magic_migrate` crate https://docs.rs/magic_migrate/latest/magic_migrate/macro.try_migrate_link.html.

https://www.youtube.com/watch?v=NnP5iDKwuwk

* Add explicit Distro/Architecture cache messages

* Use updated magic_migrate macros

* Apply suggestions from code review

Co-authored-by: Ed Morley <[email protected]>

* Document breaking changes

* Align warning messages and error names

* Update buildpack.toml

- Add explanation for when stacks can be removed
- Move "targets" closer to "stacks" as they're logically tied together.

* Integration test for metadata migration

* Support heroku-20 and heroku-22 (only)

This buildpack does not support heroku-24 (yet) remove this stack from the TargetId struct. It also supports heroku-20 which was previously not present.

* Fix changelog

* Standardize println in integration tests

* Consistent cache clear messages

---------

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Ed Morley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for buildpack.toml targets for Buildpack API 0.10
3 participants