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@8 1.8.0-432 #194719

Merged
merged 2 commits into from
Nov 14, 2024
Merged

openjdk@8 1.8.0-432 #194719

merged 2 commits into from
Nov 14, 2024

Conversation

calvinit
Copy link
Contributor

@calvinit calvinit commented Oct 17, 2024

  • Have you followed the guidelines for contributing?
  • Have you ensured that your commits follow the commit style guide?
  • Have you checked that there aren't other open pull requests for the same formula update/change?
  • Have you built your formula locally with HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>, where <formula> is the name of the formula you're submitting?
  • Is your test running fine brew test <formula>, where <formula> is the name of the formula you're submitting?
  • Does your build pass brew audit --strict <formula> (after doing HOMEBREW_NO_INSTALL_FROM_API=1 brew install --build-from-source <formula>)? If this is a new formula, does it pass brew audit --new <formula>?

@github-actions github-actions bot added legacy Relates to a versioned @ formula no ARM bottle Formula has no ARM bottle labels Oct 17, 2024
@calvinit calvinit mentioned this pull request Oct 17, 2024
6 tasks
@chenrui333 chenrui333 added CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. and removed CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. labels Oct 17, 2024
@calvinit
Copy link
Contributor Author

calvinit commented Oct 18, 2024

Why is the openjdk version used to test embulk here openjdk@21 instead of openjdk@8? Did the following check fail to work correctly?

on_arm do
  depends_on "openjdk@21"
end
on_intel do
  depends_on "openjdk@8"
end

I printed the content of the embulk script at bin/embulk on my Intel Mac, and it is as follows:

#!/bin/bash
export JAVA_HOME="${JAVA_HOME:-/usr/local/opt/openjdk@8/libexec/openjdk.jdk/Contents/Home}"
exec "${JAVA_HOME}/bin/java"  -jar "/usr/local/Cellar/embulk/0.11.5/libexec/embulk-0.11.5.jar" "$@"

It seems that the JAVA_HOME environment variable already has a value (which is /usr/local/opt/openjdk@21/libexec/openjdk.jdk/Contents/Home), so the subsequent openjdk@8 environment variable value is not being applied. Alternatively, the value of java_version might actually be 21 (java_version = Hardware::CPU.intel? ? "1.8" : "21"). Could this be a bug in the test-bot or brew? Because when testing embulk individually, there is no problem, but here, when it is tested as a dependency of openjdk@8 1.8.0-432, the problem arises.

P.S. def install of embulk.rb:

def install
  java_version = Hardware::CPU.intel? ? "1.8" : "21"
  libexec.install "embulk-#{version}.jar"
  bin.write_jar_script libexec/"embulk-#{version}.jar", "embulk", java_version:
end

@calvinit
Copy link
Contributor Author

Help wanted! @cho-m
Why is the embulk test section unable to correctly retrieve the value of the JAVA_HOME environment variable?
#191470

@chenrui333 chenrui333 added the test failure CI fails while running the test-do block label Oct 21, 2024
@github-actions github-actions bot added the automerge-skip `brew pr-automerge` will skip this pull request label Oct 23, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch from fed29db to 68bd427 Compare October 23, 2024 07:30
@github-actions github-actions bot removed the automerge-skip `brew pr-automerge` will skip this pull request label Oct 23, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch from 0b8bfc0 to 68bd427 Compare October 24, 2024 02:55
@github-actions github-actions bot added automerge-skip `brew pr-automerge` will skip this pull request and removed automerge-skip `brew pr-automerge` will skip this pull request labels Oct 24, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch 6 times, most recently from 79c16e8 to bd657e1 Compare October 25, 2024 11:46
@github-actions github-actions bot added the autosquash Automatically squash pull request commits according to Homebrew style. label Oct 25, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch from 16477cb to d3955dd Compare October 25, 2024 11:59
@github-actions github-actions bot removed the autosquash Automatically squash pull request commits according to Homebrew style. label Oct 25, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch 3 times, most recently from 14edc3f to e34a3cd Compare October 25, 2024 12:41
@chenrui333 chenrui333 added the CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. label Oct 25, 2024
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch from e34a3cd to 6864d32 Compare October 29, 2024 08:00
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch 4 times, most recently from b069910 to 2448cc0 Compare November 1, 2024 19:36
@Bo98
Copy link
Member

Bo98 commented Nov 1, 2024

Looks like there's one part that doesn't accept --with-extra-cflags properly, but something like this instead does work: Bo98@940717f

@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch 2 times, most recently from aa34fd4 to 40066ab Compare November 2, 2024 18:25
@calvinit
Copy link
Contributor Author

calvinit commented Nov 3, 2024

Looks like there's one part that doesn't accept --with-extra-cflags properly, but something like this instead does work: Bo98@940717f

Yes, you’re right. I’ve referred to your commit and adjusted this PR, and it seems to be working fine now. Thank you very much!
Lastly, could we also consider including CI / macOS 15-x86_64 (pull_request) in this time?

@calvinit calvinit requested review from Bo98 and carlocab November 4, 2024 02:11
@cho-m cho-m removed the test failure CI fails while running the test-do block label Nov 4, 2024
@calvinit
Copy link
Contributor Author

calvinit commented Nov 7, 2024

Should it be approved and merged into the master branch now? @Bo98 @carlocab @cho-m @chenrui333

Formula/o/[email protected] Outdated Show resolved Hide resolved
@calvinit calvinit force-pushed the openjdk@8_1.8.0-432 branch from 40066ab to 0bf8394 Compare November 11, 2024 12:20
@calvinit calvinit requested a review from Bo98 November 12, 2024 02:20
Copy link
Member

@Bo98 Bo98 left a comment

Choose a reason for hiding this comment

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

Thanks!

@calvinit
Copy link
Contributor Author

@chenrui333 Hi, it can now be requested bottles to be published to this PR.

Copy link
Contributor

This comment was marked as outdated.

Copy link
Contributor

🤖 An automated task has requested bottles to be published to this PR.

@github-actions github-actions bot added the CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. label Nov 14, 2024
@BrewTestBot BrewTestBot added this pull request to the merge queue Nov 14, 2024
Merged via the queue into Homebrew:master with commit 5524c22 Nov 14, 2024
15 checks passed
@calvinit calvinit deleted the openjdk@8_1.8.0-432 branch November 14, 2024 06:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI-no-fail-fast Continue CI tests despite failing GitHub Actions matrix builds. CI-no-fail-fast-deps Continue dependent tests despite failing GitHub Actions matrix tests. CI-published-bottle-commits The commits for the built bottles have been pushed to the PR branch. legacy Relates to a versioned @ formula no ARM bottle Formula has no ARM bottle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants