-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
fix: timezone mappings missing for Mysql JDBC driver in native mode #5621
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package io.quarkus.deployment.builditem; | ||
|
||
import io.quarkus.builder.item.MultiBuildItem; | ||
|
||
public final class NativeImageEnableAllCharsetsBuildItem extends MultiBuildItem { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
package io.quarkus.deployment.builditem; | ||
|
||
import io.quarkus.builder.item.MultiBuildItem; | ||
|
||
public final class NativeImageEnableAllTimeZonesBuildItem extends MultiBuildItem { | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -145,6 +145,7 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa | |
process.waitFor(); | ||
} | ||
Boolean enableSslNative = false; | ||
boolean enableAllTimeZones = false; | ||
for (NativeImageSystemPropertyBuildItem prop : nativeImageProperties) { | ||
//todo: this should be specific build items | ||
if (prop.getKey().equals("quarkus.ssl.native") && prop.getValue() != null) { | ||
|
@@ -155,6 +156,8 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa | |
nativeConfig.enableAllSecurityServices |= Boolean.parseBoolean(prop.getValue()); | ||
} else if (prop.getKey().equals("quarkus.native.enable-all-charsets") && prop.getValue() != null) { | ||
nativeConfig.addAllCharsets |= Boolean.parseBoolean(prop.getValue()); | ||
} else if (prop.getKey().equals("quarkus.native.enable-all-timezones") && prop.getValue() != null) { | ||
enableAllTimeZones = Boolean.parseBoolean(prop.getValue()); | ||
} else { | ||
// todo maybe just -D is better than -J-D in this case | ||
if (prop.getValue() == null) { | ||
|
@@ -222,6 +225,9 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa | |
} else { | ||
command.add("-H:-AddAllCharsets"); | ||
} | ||
if (enableAllTimeZones) { | ||
command.add("-H:+IncludeAllTimeZones"); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we have a proper property then I suppose we can ad the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure that's the plan, as soon as we have a better way to expose this configuration. |
||
} | ||
if (!protocols.isEmpty()) { | ||
command.add("-H:EnableURLProtocols=" + String.join(",", protocols)); | ||
} | ||
|
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.
I don't think removing the configuration property here is the solution.
Let's tackle the issue now. I would rename
addAllCharsets
toenableAllCharsets
in thenativeConfig
and maybe deprecate the old property and still somehow support it.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.
I see your point and I agree. I'd have preferred to do it in another PR, since it is mostly unrelated :-)
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.
I'll create an issue for this.