fix: timezone mappings missing for Mysql JDBC driver in native mode #5621

machi1990 · 2019-11-20T11:37:13Z

Fixes #5269

/cc @johnaohara

core/deployment/src/main/java/io/quarkus/deployment/pkg/NativeConfig.java

...loyment/src/main/java/io/quarkus/deployment/builditem/NativeEnableAllTimeZonesBuildItem.java

gsmet

I added a few comments and questions.

core/deployment/src/main/java/io/quarkus/deployment/pkg/NativeConfig.java

gsmet · 2019-11-20T12:57:16Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

+            if (nativeConfig.addAllTimeZones) {
+                command.add("-H:+IncludeAllTimeZones");
+            } else {
+                command.add("-H:-IncludeAllTimeZones");


I wonder if it should be Optional and if we should do nothing if not defined.

Because it could not be defined in the config and then the user could override it with the additional build args.

GraalVM has the bad habit of adjusting their flags over time; setting them explicitly has been useful before.

But it's an interesting point that then the can't easily override it. Let's amit the explicit disablemnet until we introduce (later) a configuration option?

@machi1990 could you remove the command.add("-H:-IncludeAllTimeZones"); line to follow @gsmet 's suggestion?

Sorry I realize all the typos in my previous comment made it unclear :)

Thanks for bringing this one, I had previously forgotten about it. I have pushed a new version without it @gsmet @Sanne

geoand · 2019-11-20T15:13:59Z

This will need a rebase to fix CI

Sanne · 2019-11-22T16:49:55Z

Thanks looks great, and I verified it works. I've added a very minor comment which you might want to address - but no strong opinion, I could merge it as-is if you prefer keeping that.

machi1990 · 2019-11-27T11:50:13Z

Thanks looks great, and I verified it works. I've added a very minor comment which you might want to address - but no strong opinion, I could merge it as-is if you prefer keeping that.

Hey @Sanne ping, I have force pushed a change to incorporate @gsmet suggestion. I think this PR is good to go :-)

Sanne · 2019-11-27T13:20:41Z

thanks @machi1990 ! I'll test it now..

gsmet

I added a couple more comments.

gsmet · 2019-11-28T14:46:28Z

...nt/src/main/java/io/quarkus/deployment/builditem/substrate/DeprecatedBuildItemProcessor.java

@@ -192,4 +194,9 @@
        }
        return newProps;
    }
+
+    @BuildStep
+    NativeImageEnableAllCharsetsBuildItem enableAllCharSets(List<NativeEnableAllCharsetsBuildItem> oldProps) {


Suggested change

NativeImageEnableAllCharsetsBuildItem enableAllCharSets(List<NativeEnableAllCharsetsBuildItem> oldProps) {

NativeImageEnableAllCharsetsBuildItem enableAllCharsets(List<NativeEnableAllCharsetsBuildItem> oldProps) {

gsmet · 2019-11-28T14:51:28Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

@@ -156,6 +157,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());


I don't think removing the configuration property here is the solution.

Let's tackle the issue now. I would rename addAllCharsets to enableAllCharsets in the nativeConfig and maybe deprecate the old property and still somehow support it.

I see your point and I agree. I'd have preferred to do it in another PR, since it is mostly unrelated :-)

I don't think removing the configuration property here is the solution.

Let's tackle the issue now. I would rename addAllCharsets to enableAllCharsets in the nativeConfig and maybe deprecate the old property and still somehow support it.

I'll create an issue for this.

gsmet · 2019-11-28T14:54:16Z

core/deployment/src/main/java/io/quarkus/deployment/pkg/steps/NativeImageBuildStep.java

@@ -225,6 +228,9 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa
            } else {
                command.add("-H:-AddAllCharsets");
            }
+            if (enableAllTimeZones) {
+                command.add("-H:+IncludeAllTimeZones");


If we have a proper property then I suppose we can ad the - version too as the user will be able to override it.

Sure that's the plan, as soon as we have a better way to expose this configuration.

Sanne · 2019-11-29T13:26:39Z

many thanks @machi1990 , let's merge this. Sorry for the delay, I had an unfortunate combination of travel and some problems with my hardware.

Sanne · 2019-11-29T13:27:32Z

@gsmet I see you blocked the merge. Are you ok to lift that? it's good to go IMO, we can polish further at another round.

… mode This fixes the issue by enabling all timezones by default when we have mysql extension Fixes quarkusio#5269

machi1990 · 2019-11-29T16:21:09Z

Just rebased with master to fix CI failure.

machi1990 · 2019-11-29T16:21:59Z

many thanks @machi1990 , let's merge this. Sorry for the delay, I had an unfortunate combination of travel and some problems with my hardware.

Thanks @Sanne , no problem.

gsmet · 2019-12-02T16:00:04Z

I suppose we can tweak things in a subsequent PR. But I really would like things to be symmetrical between charsets and timezones because it's exactly the same thing.

Sanne · 2019-12-02T20:00:14Z

thanks @gsmet and @machi1990 . Sure we can aim for a "symmetrical" solution, we can get there one step at a time :)

machi1990 requested a review from Sanne November 20, 2019 11:37

machi1990 added the area/persistence OBSOLETE, DO NOT USE label Nov 20, 2019

jaikiran reviewed Nov 20, 2019

View reviewed changes

core/deployment/src/main/java/io/quarkus/deployment/pkg/NativeConfig.java Outdated Show resolved Hide resolved

machi1990 force-pushed the fix/5269 branch from 91a9e63 to 190cee3 Compare November 20, 2019 12:35

gsmet reviewed Nov 20, 2019

View reviewed changes

...loyment/src/main/java/io/quarkus/deployment/builditem/NativeEnableAllTimeZonesBuildItem.java Outdated Show resolved Hide resolved

gsmet requested changes Nov 20, 2019

View reviewed changes

machi1990 force-pushed the fix/5269 branch 2 times, most recently from 48ebf01 to cc04654 Compare November 20, 2019 21:37

machi1990 requested review from Sanne and removed request for Sanne November 22, 2019 08:47

machi1990 added this to the 1.1.0 milestone Nov 22, 2019

machi1990 requested a review from gsmet November 22, 2019 11:43

machi1990 force-pushed the fix/5269 branch from cc04654 to 4961eaf Compare November 22, 2019 17:56

gsmet requested changes Nov 28, 2019

View reviewed changes

machi1990 force-pushed the fix/5269 branch from 4961eaf to 83a75d8 Compare November 28, 2019 19:31

Sanne approved these changes Nov 29, 2019

View reviewed changes

machi1990 added 2 commits November 29, 2019 17:20

feat(core): add NativeEnableAllTimeZonesBuildItem build item

79b8153

fix(mysql): Timezone mappings missing for Mysql JDBC driver in native…

c1f3e5b

… mode This fixes the issue by enabling all timezones by default when we have mysql extension Fixes quarkusio#5269

machi1990 force-pushed the fix/5269 branch from 83a75d8 to c1f3e5b Compare November 29, 2019 16:20

gsmet approved these changes Dec 2, 2019

View reviewed changes

gsmet merged commit f33a35b into quarkusio:master Dec 2, 2019

machi1990 deleted the fix/5269 branch December 2, 2019 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: timezone mappings missing for Mysql JDBC driver in native mode #5621

fix: timezone mappings missing for Mysql JDBC driver in native mode #5621

machi1990 commented Nov 20, 2019

gsmet left a comment

gsmet Nov 20, 2019

Sanne Nov 20, 2019

Sanne Nov 22, 2019

Sanne Nov 22, 2019

machi1990 Nov 22, 2019

geoand commented Nov 20, 2019

Sanne commented Nov 22, 2019

machi1990 commented Nov 27, 2019

Sanne commented Nov 27, 2019

gsmet left a comment

gsmet Nov 28, 2019

gsmet Nov 28, 2019

machi1990 Nov 28, 2019

machi1990 Dec 2, 2019

gsmet Nov 28, 2019

Sanne Nov 29, 2019

Sanne commented Nov 29, 2019

Sanne commented Nov 29, 2019

machi1990 commented Nov 29, 2019

machi1990 commented Nov 29, 2019

gsmet commented Dec 2, 2019

Sanne commented Dec 2, 2019

	NativeImageEnableAllCharsetsBuildItem enableAllCharSets(List<NativeEnableAllCharsetsBuildItem> oldProps) {
	NativeImageEnableAllCharsetsBuildItem enableAllCharsets(List<NativeEnableAllCharsetsBuildItem> oldProps) {

fix: timezone mappings missing for Mysql JDBC driver in native mode #5621

fix: timezone mappings missing for Mysql JDBC driver in native mode #5621

Conversation

machi1990 commented Nov 20, 2019

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Nov 20, 2019

Sanne commented Nov 22, 2019

machi1990 commented Nov 27, 2019

Sanne commented Nov 27, 2019

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sanne commented Nov 29, 2019

Sanne commented Nov 29, 2019

machi1990 commented Nov 29, 2019

machi1990 commented Nov 29, 2019

gsmet commented Dec 2, 2019

Sanne commented Dec 2, 2019