jetty-project / jetty-alpn-agent

Enables Jetty ALPN/NPN support via -javaagent JVM option
Apache License 2.0
47 stars 16 forks source link

fix(version parsing) : more lenient on formatting #3

Closed CedricGatay closed 8 years ago

CedricGatay commented 8 years ago

"Official" docker images presents java version under the form x.y.z_k-internal, which fails java version detection. By applying this patch, we are going from :

$ docker run --rm -it -v $(pwd):/app -w /app java:openjdk-8u72-alpine java -javaagent:/app/target/jetty-alpn-agent-2.0.1-SNAPSHOT.jar -version

[jetty-alpn-agent] Could not parse java.version: 1.8.0_72-internal
[jetty-alpn-agent] Could not find a matching alpn-boot JAR for Java version: 1.8.0_72-internal
openjdk version "1.8.0_72-internal"
OpenJDK Runtime Environment (build 1.8.0_72-internal-alpine-r2-b15)
OpenJDK 64-Bit Server VM (build 25.72-b15, mixed mode)

To

$ docker run --rm -it -v $(pwd):/app -w /app java:openjdk-8u72-alpine java -javaagent:/app/target/jetty-alpn-agent-2.0.1-SNAPSHOT.jar -version

[jetty-alpn-agent] Using: alpn-boot-8.1.7.v20160121.jar
openjdk version "1.8.0_72-internal"
OpenJDK Runtime Environment (build 1.8.0_72-internal-alpine-r2-b15)
OpenJDK 64-Bit Server VM (build 25.72-b15, mixed mode)
sbordet commented 8 years ago

@gregw @joakime you know why docker images would change the JDK versioning ?

Reference: http://www.oracle.com/technetwork/java/javase/versioning-naming-139433.html

If it is official, then it must not contain the - character.

gregw commented 8 years ago

I have opened an issue on docker-java asking about this. However, I think it would be best to apply this patch so that we can run against internal versions if need be.

gregw commented 8 years ago

see https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=819522

sbordet commented 8 years ago

@CedricGatay please:

private static final Pattern VERSION_PATTERN =
            Pattern.compile("^([0-9]+)\\.([0-9]+)\\.([0-9]+)(?:_([0-9]+))?(?:-.+)?$");

I made 2 changes: the first is to make the "patch" group optional (the ? was wrongly placed), and widened the "identifier" group (the one you added), as we don't need a capturing group for that, and we want to allow any char to avoid to have to change it in the future if someone puts in some weird char in the identifier.

CedricGatay commented 8 years ago

@sbordet I made the changes you required as well as signed off my commit. Thank you for your feedback !

CedricGatay commented 8 years ago

Hi @sbordet do you have an ETA for a release including this (the 2.0.1 I guess). Do I need to do an internal release to my nexus or can I expect a "public" one anytime soon ? Thank you

sbordet commented 8 years ago

@CedricGatay Release 2.0.1 is out (may take a few hours to be in Maven Central).

CedricGatay commented 8 years ago

Thank you @sbordet !