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

Adding more JDK support for Jenkins Mac agent nodes #376

Merged
merged 4 commits into from
Dec 8, 2023

Conversation

jordarlu
Copy link
Contributor

@jordarlu jordarlu commented Dec 7, 2023

Description

  • Adding JDK8, JDK14, JDK17, JDK19, JDK20, and JDK21 on Jenkins MAC agent node setup shell script
  • Set default JAVA to JDK11

ref : https://ports.macports.org/port/openjdk11-temurin/details/

Issues Resolved

A Mac agent build failure on JDK21 (https://build.ci.opensearch.org/blue/organizations/jenkins/publish-opensearch-min-snapshots/detail/publish-opensearch-min-snapshots/432/pipeline) due to a bug (opensearch-project/opensearch-build#4272)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Jeff Lu <[email protected]>
Comment on lines 34 to 37
for d in /Library/Java/JavaVirtualMachines/*/Contents/Home; do
jenv add "$d"
done
jenv global temurin64-11.0.21
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion:

  1. Change d to some other better variable name to be more visiable
  2. See if you can make every entry an alias for easier setup, recommending openjdk-<majorVersion> as that is the output of the javaVersion from detectAgent jenkins lib such as:

https://github.com/opensearch-project/opensearch-build-libraries/blob/main/vars/detectDockerAgent.groovy#L16C27-L16C34
https://github.com/opensearch-project/opensearch-build/blob/main/jenkins/opensearch/distribution-build.jenkinsfile#L102

Thanks.

Copy link
Contributor Author

@jordarlu jordarlu Dec 8, 2023

Choose a reason for hiding this comment

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

thanks, Peter, looks like jenv did support using the alias per jenv/jenv#306 , so, I am adapting it in revised PR here now.. and we can remove the for-loop
Also, seems like some issues on jdk14, 19, and 20 on my local Mac test, let me remove them for now.

Signed-off-by: Jeff Lu <[email protected]>
@jordarlu jordarlu merged commit 4d068fd into opensearch-project:main Dec 8, 2023
3 checks passed
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