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

Introduce heroku/jvm-standalone buildpack #545

Closed
wants to merge 2 commits into from
Closed

Conversation

Malax
Copy link
Member

@Malax Malax commented Jul 26, 2023

Currently, the heroku/jvm buildpack can be used without any other buildpack. It will then install OpenJDK (and some Heroku specific related extras). This enabled customers to run any application that requires a JVM without needing to build their application with a buildpack.

The downside to this approach is that other buildpacks cannot use heroku/jvm in a conditional way. This wasn't a problem for the JVM buildpacks in this repository as they all required a JVM in any case. Heroku's Ruby buildpack is a different story. It doesn't require a JVM unless JRuby is used. With the current design, adding heroku/jvm to a "group" would always install OpenJDK, even when JRuby wasn't used.

This PR removes the ability to use heroku/jvm in a standalone fashion to enable other buildpacks to nicely compose.

For users that require the old behavior for their applications can now use the heroku/jvm-standalone buildpack instead. It uses the same code and is a drop-in replacement.

Open Tasks

Tasks that need to be completed before merging:

  • Create docker.io/heroku/buildpack-add-cnb-require-jdk repository
  • Create docker.io/heroku/buildpack-jvm-standalone repository

Closes GUS-W-13829430

Comment on lines +4 to +8
id = "heroku/add-cnb-require-jdk"
version = "1.1.2"
name = "Heroku Internal: Add CNB require for JDK"
homepage = "https://github.com/heroku/buildpacks-jvm"
description = "Internal buildpack that adds 'jdk' to the build 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.

I'm not happy with the name and description here. If anyone has a better suggestion: let's hear it! :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I hear the name/description question, and I want to propose a different buildpack strategy. Instead of:

  • Only provide jdk: heroku/jvm
  • Only require jdk: heroku/add-cnb-require-jdk
  • Provide and require jdk: heroku/jvm-standalone

We could use:

  • Only provide jdk: heroku/jdk-provide
  • Only require jdk: heroku/jdk-require
  • Provide and require jdk: heroku/jvm

Then the behavior of existing heroku/jvm users is preserved, and the naming of the modules is perfectly clear. The downside is the CNB abstraction is leaking through in the naming, but I think that's fine.

If you feel like you want to deprecate and change the heroku/jvm behavior, I think now's a good time. We could rename it to heroku/jvm-standalone or heroku/jdk-standalone. For all the other buildpacks heroku/<language>, it's usually a meta-buildpack (in that it does more/not-less). I like the nodejs paradigm. If I came from a node heroku project to the JVM ecosystem, I might expect heroku/jvm to do the sbt/java detection for me and fall back to "just" installing the JDK.

To that end, if we're going to break and make people use different buildpacks, we could break hard and go with this scheme:

  • Only provide jdk: heroku/jdk-provide
  • Only require jdk: heroku/jdk-require
  • Provide and require jdk: heroku/jdk-standalone
  • Install sbt, java, or jdk: heroku/jvm

It's a lot of buildpacks, but I feel like the naming is clear and consistent.

Iterating on description/name

To answer your original action/question. Description idea:

Internal: Requires jdk in the build plan but does not provide it. Meant to be used along with the heroku/jvm buildpack.

The premise is to explain the behavior and the intent. Behavior "requires jdk" intent: use along side another buildpack.

By tagging it as internal, are we trying to signal that others shouldn't use it and that it's an unstable interface? If so, it might be worth spelling that out. I think that this is such a simple buildpack that it would be okay to use externally, but I understand the

I think the name is fine. It does what it says on the tin. I don't know where name will show up for customers down the road. If it becomes an issue we could re-name it.

Copy link
Contributor

@schneems schneems left a comment

Choose a reason for hiding this comment

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

To sum up, what I'm seeing here, is the same strategy as #536 but with 3 buildpacks instead of 2 (and better naming). It will meet the needs of heroku/ruby and would fix my issue of the JDK getting installed no matter what. I'm in favor of the idea.

I also proposed a different naming strategy. Instead of:

  • Only provide jdk: heroku/jvm
  • Only require jdk: heroku/add-cnb-require-jdk
  • Provide and require jdk: heroku/jvm-standalone

We could use:

  • Only provide jdk: heroku/jdk-provide
  • Only require jdk: heroku/jdk-require
  • Provide and require jdk: heroku/jvm

Requests

  • Iteration on naming. At least give me your gut feedback on my suggestion. It might seem like a bike-shed, but it's much harder to change later
  • Update the wording in the changelog

@@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0

## [Unreleased]

### Changed

* This buildpack can no longer be used without another buildpack that requires `jdk` in the build plan. This enables other (meta-)buildpacks to include `heroku/jvm` and only require to install OpenJDK when they need it. For users that used `heroku/jvm` to install OpenJDK in a standalone fashion, a new `heroku/jvm-standalone` buildpack was created that can be used as a drop-in replacement. ([#545](https://github.com/heroku/buildpacks-jvm/pull/545))
Copy link
Contributor

Choose a reason for hiding this comment

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

There's this part is a bit off "and only require to install". When I started messing with the wording, I ended up iterating a bit and came up with this. Please either take this suggestion wholesale or update with your own wording.

Suggested change
* This buildpack can no longer be used without another buildpack that requires `jdk` in the build plan. This enables other (meta-)buildpacks to include `heroku/jvm` and only require to install OpenJDK when they need it. For users that used `heroku/jvm` to install OpenJDK in a standalone fashion, a new `heroku/jvm-standalone` buildpack was created that can be used as a drop-in replacement. ([#545](https://github.com/heroku/buildpacks-jvm/pull/545))
* This `heroku/jvm` buildpack no longer installs OpenJDK unconditionally. For prior `heroku/jvm` users who want to install OpenJDK in a standalone fashion, a new `heroku/jvm-standalone` buildpack was created as a drop-in replacement. Buildpack authors can install OpenJDK by requiring the `jdk` in their build plan and adding `heroku/jvm` in their build group. ([#545](https://github.com/heroku/buildpacks-jvm/pull/545))

Comment on lines +4 to +8
id = "heroku/add-cnb-require-jdk"
version = "1.1.2"
name = "Heroku Internal: Add CNB require for JDK"
homepage = "https://github.com/heroku/buildpacks-jvm"
description = "Internal buildpack that adds 'jdk' to the build plan."
Copy link
Contributor

Choose a reason for hiding this comment

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

I hear the name/description question, and I want to propose a different buildpack strategy. Instead of:

  • Only provide jdk: heroku/jvm
  • Only require jdk: heroku/add-cnb-require-jdk
  • Provide and require jdk: heroku/jvm-standalone

We could use:

  • Only provide jdk: heroku/jdk-provide
  • Only require jdk: heroku/jdk-require
  • Provide and require jdk: heroku/jvm

Then the behavior of existing heroku/jvm users is preserved, and the naming of the modules is perfectly clear. The downside is the CNB abstraction is leaking through in the naming, but I think that's fine.

If you feel like you want to deprecate and change the heroku/jvm behavior, I think now's a good time. We could rename it to heroku/jvm-standalone or heroku/jdk-standalone. For all the other buildpacks heroku/<language>, it's usually a meta-buildpack (in that it does more/not-less). I like the nodejs paradigm. If I came from a node heroku project to the JVM ecosystem, I might expect heroku/jvm to do the sbt/java detection for me and fall back to "just" installing the JDK.

To that end, if we're going to break and make people use different buildpacks, we could break hard and go with this scheme:

  • Only provide jdk: heroku/jdk-provide
  • Only require jdk: heroku/jdk-require
  • Provide and require jdk: heroku/jdk-standalone
  • Install sbt, java, or jdk: heroku/jvm

It's a lot of buildpacks, but I feel like the naming is clear and consistent.

Iterating on description/name

To answer your original action/question. Description idea:

Internal: Requires jdk in the build plan but does not provide it. Meant to be used along with the heroku/jvm buildpack.

The premise is to explain the behavior and the intent. Behavior "requires jdk" intent: use along side another buildpack.

By tagging it as internal, are we trying to signal that others shouldn't use it and that it's an unstable interface? If so, it might be worth spelling that out. I think that this is such a simple buildpack that it would be okay to use externally, but I understand the

I think the name is fine. It does what it says on the tin. I don't know where name will show up for customers down the road. If it becomes an issue we could re-name it.

@Malax
Copy link
Member Author

Malax commented Jul 31, 2023

Closing in favour of #546.

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.

2 participants