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

openjdk@17 17.0.10 #160094

Closed
wants to merge 2 commits into from
Closed

Conversation

Porkepix
Copy link
Contributor

@Porkepix Porkepix commented Jan 16, 2024

Created by brew bump


Created with brew bump-formula-pr.

  • resource blocks have been checked for updates.

@github-actions github-actions bot added legacy Relates to a versioned @ formula bump-formula-pr PR was created using `brew bump-formula-pr` labels Jan 16, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label Jan 19, 2024
@chenrui333 chenrui333 added in progress Stale bot should stay away and removed stale No recent activity labels Jan 19, 2024
openjdk@17: update boostrap jdk

Signed-off-by: Rui Chen <[email protected]>
@chenrui333 chenrui333 added CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. long build Set a long timeout for formula testing labels Jan 19, 2024
@chenrui333
Copy link
Member

  checking for version string... 17.0.10+0
  configure: error: The path of BOOT_JDK_ARG, which resolves as "/private/tmp/openjdkA17-20240119-5379-qyga7x/jdk17u-jdk-17.0.10-ga/boot-jdk/Contents/Home", is not found.
  /private/tmp/openjdkA17-20240119-5379-qyga7x/jdk17u-jdk-17.0.10-ga/build/.configure-support/generated-configure.sh: line 84: 5: Bad file descriptor

@github-actions github-actions bot added autosquash Automatically squash pull request commits according to Homebrew style. and removed CI-long-timeout [DEPRECATED] Use longer GitHub Actions CI timeout. labels Jan 19, 2024
@bevanjkay bevanjkay requested a review from cho-m January 23, 2024 23:02
Comment on lines -57 to +59
url "https://download.java.net/java/GA/jdk17.0.2/dfd4a8d0985749f896bed50d7138ee7f/8/GPL/openjdk-17.0.2_macos-aarch64_bin.tar.gz"
sha256 "602d7de72526368bb3f80d95c4427696ea639d2e0cc40455f53ff0bbb18c27c8"
url "https://cdn.azul.com/zulu/bin/zulu17.46.19-ca-jdk17.0.9-macosx_aarch64.tar.gz"
sha256 "d6837676e55b97772b6512e253fdaf8ab282bb216c0f8366b6c5905cd02b5056"
Copy link
Member

Choose a reason for hiding this comment

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

Was there a specific issue that required updating JDK on ARM?

Copy link
Contributor Author

@Porkepix Porkepix Jan 26, 2024

Choose a reason for hiding this comment

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

I sometimes update these to use bump version minus one to build as it seems generally more appropriate.
Here it's @chenrui333 's doing to move to these to Zulu and I don't know why.
Same for when on another bump resource was updated to build from binary release of the exact same version: generally you want to build with version n-1, so I didn't knew why use the same.

Copy link
Member

Choose a reason for hiding this comment

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

Same major version may be worth it here as there is no OpenJDK 16 ARM release. It would allow avoiding a 3rd party JDK. For older OpenJDK, it may depend on if there are any required fixes, e.g. OpenJDK 10 is no longer supported so may not get all backports (Mainly issue for new platforms like Apple ARM).

Unless there is a required fix, I think we can stick to OpenJDK's binaries for bootstrap on newer OpenJDK formulae (openjdk@17 and openjdk).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I meant for patch versions, like using 17.0.1 to build 17.0.2 (or latest 17.x.y for 18.0.0).

Copy link
Member

@cho-m cho-m Jan 26, 2024

Choose a reason for hiding this comment

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

For OpenJDK, n-1 refers to previous major version and is the standard choice of boot JDK.

Changing patch release usually has minor/no impact and OpenJDK doesn't provide them after a certain point. They point to Temurin releases instead https://wiki.openjdk.org/display/JDKUpdates/JDK+17u#JDK17u-Downloads

Copy link
Member

Choose a reason for hiding this comment

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

just a bootstrap jdk, it is fine to use whatever latest

Copy link
Member

Choose a reason for hiding this comment

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

just a bootstrap jdk, it is fine to use whatever latest

The main thing is we only resort to Zulu as a last resort (e.g. OpenJDK7 doesn't exist and OpenJDK11 doesn't provide ARM release).

Copy link
Member

Choose a reason for hiding this comment

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

Anyway, I think this should be reverted unless there is a specific issue. And if there is a specific issue, it should be included in the comment.

For example, the reason we use Zulu on openjdk@8:

# Oracle doesn't serve JDK 7 downloads anymore, so we use Zulu JDK 7 for bootstrapping.
# https://www.azul.com/downloads/?version=java-7-lts&package=jdk
resource "boot-jdk" do

Copy link
Member

Choose a reason for hiding this comment

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

We definitely should not use a random non-open boot JDK here.

Comment on lines -57 to +59
url "https://download.java.net/java/GA/jdk17.0.2/dfd4a8d0985749f896bed50d7138ee7f/8/GPL/openjdk-17.0.2_macos-aarch64_bin.tar.gz"
sha256 "602d7de72526368bb3f80d95c4427696ea639d2e0cc40455f53ff0bbb18c27c8"
url "https://cdn.azul.com/zulu/bin/zulu17.46.19-ca-jdk17.0.9-macosx_aarch64.tar.gz"
sha256 "d6837676e55b97772b6512e253fdaf8ab282bb216c0f8366b6c5905cd02b5056"
Copy link
Member

Choose a reason for hiding this comment

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

We definitely should not use a random non-open boot JDK here.

@Porkepix Porkepix mentioned this pull request Feb 17, 2024
1 task
@SMillerDev SMillerDev removed the in progress Stale bot should stay away label Feb 20, 2024
Copy link
Contributor

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. To keep this pull request open, add a help wanted or in progress label.

@github-actions github-actions bot added the stale No recent activity label Feb 23, 2024
@github-actions github-actions bot closed this Feb 24, 2024
@@ -77,7 +78,7 @@ class OpenjdkAT17 < Formula
def install
boot_jdk = buildpath/"boot-jdk"
resource("boot-jdk").stage boot_jdk
boot_jdk /= "Contents/Home" if OS.mac?
boot_jdk /= "Contents/Home" if OS.mac? && !Hardware::CPU.arm?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@chenrui333 I don't know what was the purpose of this change, but another PR was merged without, in case this was important to get.

@github-actions github-actions bot added the outdated PR was locked due to age label Mar 28, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 28, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
autosquash Automatically squash pull request commits according to Homebrew style. bump-formula-pr PR was created using `brew bump-formula-pr` CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. legacy Relates to a versioned @ formula long build Set a long timeout for formula testing outdated PR was locked due to age stale No recent activity
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants