-
Notifications
You must be signed in to change notification settings - Fork 350
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
fixes(#5439): Build should enforce a required jdk version #5440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The presence of the check is a good thing to have. However, we introduce a dependency on some additional tooling (awk
) we may not have in all the different CI machines involved in the build. Let's see if the checks are green though.
Ok, lets try to do this in Go - its easier to maintain anyway |
2c4bc8d
to
52d760a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-0
I'm not a big fan of maintaining external tools. If we want to have it, let's make sure this is not panicking if something is not found as expected.
Also, if we want to be fair and do a proper tool, what Camel K really need for building is Maven, and Maven has to have a JAVA_HOME pointing to a JDK 17 based. So, If we want to do the things properly, we need to make sure to check the java version
run on the JAVA_HOME bin and not the one in the path (which may be different).
script/check_jdk_version.go
Outdated
versionRegex := regexp.MustCompile(`version "?([1-9]+)(\.([0-9]+)){2,3}"?`) | ||
matches := versionRegex.FindStringSubmatch(versionStr) | ||
if len(matches) < 2 { | ||
panic(fmt.Sprintf("Unable to determine Java version: %s\n", versionStr)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we should not panic. We cannot test all the possible JDK out there to make sure they return the expected text.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. How about we get the java version from maven i.e.
$ mvn -version
Apache Maven 3.9.6 (bc0240f3c744dd6b6ec2920b3cd08dcc295161ae)
Maven home: /opt/homebrew/Cellar/maven/3.9.6/libexec
Java version: 17.0.6, vendor: Azul Systems, Inc., runtime: /Library/Java/JavaVirtualMachines/zulu-17.jdk/Contents/Home
Default locale: en_GB, platform encoding: UTF-8
OS name: "mac os x", version: "14.4.1", arch: "aarch64", family: "mac"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okey. Still make sure any parsing problem is not leading to a panic which would impede to complete a build.
script/check_jdk_version.go
Outdated
@@ -0,0 +1,33 @@ | |||
package main |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have a cmd/util directory where we should host any tool. Please, keep the same structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
enforces JDK 17 or above