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

Use unified Heroku buildpack output style #745

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Use unified Heroku buildpack output style #745

wants to merge 8 commits into from

Conversation

Malax
Copy link
Member

@Malax Malax commented Nov 22, 2024

This PR introduces a new shared library for all JVM buildpacks that implements the unified Heroku buildpack output style. All buildpacks in the repository have been modified to use this new shared library over the log module from libherokubuildpack.

In some cases, the buildpacks changes their output a little bit more to output more detailed information about the process. For example, the jvm buildpack now tells the user where the OpenJDK version is defined.

Example

# Heroku OpenJDK Buildpack

- OpenJDK version resolution
  - Using version string provided in `system.properties`
  - Selected major version `11` resolves to `11.0.25`
- OpenJDK Installation
  - Downloading and unpacking OpenJDK distribution
  - Done (50.5s)
- Applying JDK overlay
  - Skipping (directory `.jdk_overlay` not present)
- Linking base image certificates as OpenJDK keystore
  - Done.

# Heroku Maven Buildpack

- Installing Maven
  - Skipping (Maven wrapper detected)
- Running Maven build
  - Running `./mvnw -DskipTests clean install`
    Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8
    [INFO] Scanning for projects...
    [INFO]
    [INFO] -------------------< com.heroku:simple-http-service >-------------------
    [INFO] Building simple-http-service 1.0-SNAPSHOT
    [INFO] --------------------------------[ jar ]---------------------------------

Closes:
W-17215812
W-17215820
W-17215822
W-17215818

@schneems
Copy link
Contributor

schneems commented Dec 3, 2024

Not a blocker, but would help for QA-ing the output as a whole to print the getting started guide in CI. https://github.com/heroku/buildpacks-ruby/blob/187770e700a433cf9e6af678cb3eb5735b89eec5/.github/workflows/ci.yml#L62-L97

shared/src/output.rs Outdated Show resolved Hide resolved
shared/src/output.rs Outdated Show resolved Hide resolved
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.

Please merge #761, put this back in draft state and make changes until you're happy with the output (removal of newlines, using the correct style for a "value" etc.) Then we can iterate on the output style. Happy to iterate/pair too.

@Malax Malax marked this pull request as draft January 9, 2025 12:05
@Malax Malax marked this pull request as ready for review January 9, 2025 12:31
@Malax
Copy link
Member Author

Malax commented Jan 9, 2025

All (still applicable) comments have been addressed. :)

@Malax Malax force-pushed the malax/output branch 4 times, most recently from eccae7c to 29ca3fb Compare January 17, 2025 13:30
Copy link
Contributor

@runesoerensen runesoerensen left a comment

Choose a reason for hiding this comment

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

Appreciate all your work on this! I've updated my branch adopting this code after your last tweaks, and it works great. I'm approving the PR, and just wanted to add some thoughts/comments (that are likely outside the scope of what needs to be addressed here):

  • Would it make sense to always flush standard output/error at the end of the output::print_* functions? Or perhaps, more pragmatically, just calling io::stdout().flush() at the beginning of the two functions that write to stderr (i.e. output::print_error and output::print_warning)?

    There's a risk the output isn't displayed in the "expected" order when writing to both streams, e.g. this:

      output::print_section("Foo");
      output::print_error("Bar", "Baz");
    

    Should probably print output like this:

      - Foo
      ! ERROR: Bar
      ! 
      ! Baz
    

    But very frequently, the standard error output will appear before the standard output, e.g.:

      ! ERROR: Bar
      ! 
      ! Baz
      - Foo
    

    I'm curious if you have thoughts on this? It's of course fairly trivial to explicitly flush where needed, but should this be necessary when using the output module (as opposed to directly interacting with the standard error/output streams)?

  • The output from the Gradle buildpack could perhaps be improved. It's a bit tricky dealing with a daemon running in the background, and that situation hasn't changed with this PR as far as I can tell, but the unindented output from the daemon sticks out a bit (now that the buildpack output is indented/using sections). Would it make sense to indent the output to produce something like this?

- Running Gradle build
  - Starting Gradle daemon
      Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8
      Downloading https://services.gradle.org/distributions/gradle-8.12-bin.zip
      .............10%.............20%.............30%.............40%.............   50%.............            60%.............70%.............80%.............90%.............100%
      Starting a Gradle Daemon (subsequent builds will be faster)
      > Task :heroku_buildpack_start_daemon UP-TO-DATE
      
      BUILD SUCCESSFUL in 30s
        - Done (31.2s)
        - Querying tasks
        - Done (1.4s)
        - Querying dependency report
        - Done (2.9s)
- Running Gradle build
  - Running `./gradlew build -x check`
      Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8
      > Task :compileJava
      > Task :processResources
      > Task :classes
      > Task :resolveMainClassName
      > Task :bootJar
      > Task :jar
      > Task :assemble
      > Task :build
  • On a related note, it seems a bit off that there are now two sections with the same text, Running Gradle build. I noticed the old headers were changed in this PR, perhaps to accomodate the section/subsection style (or maybe I'm missing something) -- but would it make sense to name the sections something like "Gradle Daemon" and "Gradle Build", and printing the commands as subsections?

  • It's great that output::print_warning and output::print_error include the ! prefix on all lines in the body. However, empty lines have a trailing whitespace/space after the !.

    I can see some merit to using the same prefix for all lines in the error output, but guess I'd prefer it if empty error/warning body lines were written as ! rather than ! .

    This is definitely a nitpick and likely not worth the added complexity to implement. Just wanted to point out this difference compared to for instance the bullet_stream implementation (here's the relevant diff).

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.

SBT buildpack toutput

# Heroku sbt Buildpack

H2 header for buildpack names (all buildpacks)

      [info] Non-compiled module 'compiler-bridge_2.13' for Scala 2.13.10. Compiling...
      [info]   Compilation completed in 9.87s.
      [info] done compiling
      [success] Total time: 15 s, completed Jan 17, 2025, 2:45:49 PM
      [info] Wrote /workspace/target/scala-2.13/scala-getting-started_2.13-1.0-SNAPSHOT.pom
      [success] Total time: 1 s, completed Jan 17, 2025, 2:45:49 PM

  - Done (48.6s)

Missing some kind of a global "done" to separate the output of this buildpack from future buildpacks. (all buildpacks)

Gradle buildpack

Indentation on this streaming command:

- Running Gradle build
  - Starting Gradle daemon
Picked up JAVA_TOOL_OPTIONS: -Dfile.encoding=UTF-8
Downloading https://services.gradle.org/distributions/gradle-8.12-bin.zip
.............10%.............20%.............30%.............40%.............50%.............60%.............70%.............80%.............90%.............100%
Starting a Gradle Daemon (subsequent builds will be faster)
> Task :heroku_buildpack_start_daemon UP-TO-DATE

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.

We are getting double warning output when no version is specified, one in the old format and one in the new:

Correction it's a different warning, but we're still mixing and matching log styles:

$ cargo test --locked -- --ignored --test-threads 16 openjdk_default
! WARNING: No OpenJDK version specified
!
! Your application does not explicitly specify an OpenJDK version. The latest
! long-term support (LTS) version will be installed. This currently is OpenJDK 21.
!
! This default version will change when a new LTS version is released. Your
! application might fail to build with the new version. We recommend explicitly
! setting the required OpenJDK version for your application.
!
! To set the OpenJDK version, add or edit the system.properties file in the root
! directory of your application to contain:
!
! java.runtime.version = 21

[Warning: Using default version]
Your application does not explicitly specify which Maven version should be used to build your application.
The current default version 3.9.4 will be used. This default version will change from time to time.
Depending on your build configuration, a different Maven version might cause your build to fail.

We recommend that you use Maven wrapper to ensure that the same Maven version is always used to build your
application. Alternatively, you can set 'maven.version' in 'system.properties' to a supported Maven version.

thread 'main' panicked at buildpacks/maven/src/warnings.rs:28:5:
Nope
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
ERROR: failed to build: executing lifecycle: failed with status code: 51

From here https://github.com/heroku/buildpacks-jvm/blob/3b140fce22ba8f186f393676fb3f02acfee03b68/buildpacks/maven/src/warnings.rs

@runesoerensen
Copy link
Contributor

runesoerensen commented Jan 22, 2025

H2 header for buildpack names (all buildpacks)

Definitely agree we should use the same buildpack name headers across all buildpacks, but the spec actually calls for h1 headers. Maybe that needs to be updated to reflect how it's implemented in bullet_stream/should be implemented here?

I'm ok with either h1 or h2 (as long as we use them consistently), but curious why you opted for h2 in your implementation?

@schneems
Copy link
Contributor

@runesoerensen I told him to do it. I opened heroku-buildpacks/bullet_stream#23 to bring that to the larger group.

@schneems
Copy link
Contributor

Here's my take on this PR. It's implemented on top of this branch. #767

image

Take anything that's useful...drop anything that's not. Doing that helped me find some missing changes such as left-over libherokubuildpack::log calls. It prints more command info to the logs, but IMHO that's fine unless there's strong reasons to hide one. I also refactored the command error handling which would close out #762.

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.

3 participants