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

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

Merged
merged 2 commits into from
Dec 2, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@

import io.quarkus.builder.item.MultiBuildItem;

/**
* @deprecated use {@link NativeImageEnableAllCharsetsBuildItem} instead
*/
@Deprecated
public final class NativeEnableAllCharsetsBuildItem 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 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
Expand Up @@ -5,6 +5,8 @@
import java.util.Map;

import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.NativeEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageConfigBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageProxyDefinitionBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBuildItem;
Expand Down Expand Up @@ -192,4 +194,9 @@ List<NativeImageSystemPropertyBuildItem> substrateSystemProperties(List<Substrat
}
return newProps;
}

@BuildStep
NativeImageEnableAllCharsetsBuildItem enableAllCharsets(List<NativeEnableAllCharsetsBuildItem> oldProps) {
return new NativeImageEnableAllCharsetsBuildItem();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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());
Copy link
Member

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 to enableAllCharsets in the nativeConfig and maybe deprecate the old property and still somehow support it.

Copy link
Member Author

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 :-)

Copy link
Member Author

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 to enableAllCharsets in the nativeConfig and maybe deprecate the old property and still somehow support it.

I'll create an issue for this.

} else {
// todo maybe just -D is better than -J-D in this case
if (prop.getValue() == null) {
Expand Down Expand Up @@ -222,6 +225,9 @@ public NativeImageBuildItem build(NativeConfig nativeConfig, NativeImageSourceJa
} else {
command.add("-H:-AddAllCharsets");
}
if (enableAllTimeZones) {
command.add("-H:+IncludeAllTimeZones");
Copy link
Member

Choose a reason for hiding this comment

The 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 - version too as the user will be able to override it.

Copy link
Member

Choose a reason for hiding this comment

The 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));
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
import io.quarkus.deployment.builditem.ExtensionSslNativeSupportBuildItem;
import io.quarkus.deployment.builditem.JavaLibraryPathAdditionalPathBuildItem;
import io.quarkus.deployment.builditem.JniBuildItem;
import io.quarkus.deployment.builditem.NativeEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllTimeZonesBuildItem;
import io.quarkus.deployment.builditem.SslNativeConfigBuildItem;
import io.quarkus.deployment.builditem.SslTrustStoreSystemPropertyBuildItem;
import io.quarkus.deployment.builditem.SystemPropertyBuildItem;
Expand All @@ -43,7 +44,8 @@ void build(SslContextConfigurationRecorder sslContextConfigurationRecorder,
List<NativeImageConfigBuildItem> nativeImageConfigBuildItems,
SslNativeConfigBuildItem sslNativeConfig,
List<JniBuildItem> jniBuildItems,
List<NativeEnableAllCharsetsBuildItem> nativeEnableAllCharsetsBuildItems,
List<NativeImageEnableAllCharsetsBuildItem> nativeImageEnableAllCharsetsBuildItems,
List<NativeImageEnableAllTimeZonesBuildItem> nativeImageEnableAllTimeZonesBuildItems,
List<ExtensionSslNativeSupportBuildItem> extensionSslNativeSupport,
List<EnableAllSecurityServicesBuildItem> enableAllSecurityServicesBuildItems,
BuildProducer<NativeImageProxyDefinitionBuildItem> proxy,
Expand Down Expand Up @@ -137,9 +139,13 @@ void build(SslContextConfigurationRecorder sslContextConfigurationRecorder,
nativeImage.produce(new NativeImageSystemPropertyBuildItem("quarkus.jni.enable", "true"));
}

if (!nativeEnableAllCharsetsBuildItems.isEmpty()) {
if (!nativeImageEnableAllCharsetsBuildItems.isEmpty()) {
nativeImage.produce(new NativeImageSystemPropertyBuildItem("quarkus.native.enable-all-charsets", "true"));
}

if (!nativeImageEnableAllTimeZonesBuildItems.isEmpty()) {
nativeImage.produce(new NativeImageSystemPropertyBuildItem("quarkus.native.enable-all-timezones", "true"));
}
}

private Boolean isSslNativeEnabled(SslNativeConfigBuildItem sslNativeConfig,
Expand Down
31 changes: 17 additions & 14 deletions docs/src/main/asciidoc/writing-extensions.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -1660,9 +1660,12 @@ A class that will be initialized at runtime rather than build time. This will ca
`io.quarkus.deployment.builditem.nativeimage.NativeImageConfigBuildItem`::
A convenience feature that allows you to control most of the above features from a single build item.

`io.quarkus.deployment.builditem.NativeEnableAllCharsetsBuildItem`::
`io.quarkus.deployment.builditem.NativeImageEnableAllCharsetsBuildItem`::
Indicates that all charsets should be enabled in native image.

`io.quarkus.deployment.builditem.NativeImageEnableAllTimeZonesBuildItem`::
Indicates that all timezones should be enabled in native image.

`io.quarkus.deployment.builditem.ExtensionSslNativeSupportBuildItem`::
A convenient way to tell Quarkus that the extension requires SSL and it should be enabled during native image build.
When using this feature, remember to add your extension to the list of extensions that offer SSL support automatically on the https://github.com/quarkusio/quarkus/blob/master/docs/src/main/asciidoc/native-and-ssl.adoc[native and ssl guide].
Expand Down Expand Up @@ -2099,16 +2102,16 @@ public final class MyExtProcessor {
@BuildStep
void registerNativeImageReources(BuildProducer<ServiceProviderBuildItem> services) {
String service = "META-INF/services/" + io.quarkus.SomeService.class.getName();

// find out all the implementation classes listed in the service files
Set<String> implementations =
Set<String> implementations =
ServiceUtil.classNamesNamedIn(Thread.currentThread().getContextClassLoader(),
service);

// register every listed implementation class so they can be instantiated
// register every listed implementation class so they can be instantiated
// in native-image at run-time
services.produce(
new ServiceProviderBuildItem(io.quarkus.SomeService.class.getName(),
new ServiceProviderBuildItem(io.quarkus.SomeService.class.getName(),
implementations.toArray(new String[0])));
}
}
Expand All @@ -2130,13 +2133,13 @@ public final class MyExtProcessor {
void registerNativeImageReources(BuildProducer<NativeImageResourceBuildItem> resource,
BuildProducer<ReflectiveClassBuildItem> reflectionClasses) {
String service = "META-INF/services/" + io.quarkus.SomeService.class.getName();

// register the service file so it is visible in native-image
resource.produce(new NativeImageResourceBuildItem(service));
// register every listed implementation class so they can be inspected/instantiated

// register every listed implementation class so they can be inspected/instantiated
// in native-image at run-time
Set<String> implementations =
Set<String> implementations =
ServiceUtil.classNamesNamedIn(Thread.currentThread().getContextClassLoader(),
service);
reflectionClasses.produce(
Expand All @@ -2148,7 +2151,7 @@ public final class MyExtProcessor {
While this is the easiest way to get your services running natively, it's less efficient than scanning the implementation
classes at build time and generating code that registers them at static-init time instead of relying on reflection.

You can achieve that by adapting the previous build step to use a static-init recorder instead of registering
You can achieve that by adapting the previous build step to use a static-init recorder instead of registering
classes for reflection:

[source,java]
Expand All @@ -2157,19 +2160,19 @@ public final class MyExtProcessor {

@BuildStep
@Record(ExecutionTime.STATIC_INIT)
void registerNativeImageReources(RecorderContext recorderContext,
void registerNativeImageReources(RecorderContext recorderContext,
SomeServiceRecorder recorder) {
String service = "META-INF/services/" + io.quarkus.SomeService.class.getName();

// read the implementation classes
Collection<Class<? extends io.quarkus.SomeService>> implementationClasses = new LinkedHashSet<>();
Set<String> implementations = ServiceUtil.classNamesNamedIn(Thread.currentThread().getContextClassLoader(),
service);
for(String implementation : implementations) {
implementationClasses.add((Class<? extends io.quarkus.SomeService>)
implementationClasses.add((Class<? extends io.quarkus.SomeService>)
recorderContext.classProxy(implementation));
}

// produce a static-initializer with those classes
recorder.configure(implementationClasses);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
import io.quarkus.deployment.annotations.BuildProducer;
import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.FeatureBuildItem;
import io.quarkus.deployment.builditem.NativeEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBundleBuildItem;

public class MsSQLProcessor {
Expand All @@ -15,9 +15,9 @@ FeatureBuildItem feature() {

@BuildStep
void nativeResources(BuildProducer<NativeImageResourceBundleBuildItem> resources,
BuildProducer<NativeEnableAllCharsetsBuildItem> nativeEnableAllCharsets) {
BuildProducer<NativeImageEnableAllCharsetsBuildItem> nativeEnableAllCharsets) {
resources.produce(new NativeImageResourceBundleBuildItem("com.microsoft.sqlserver.jdbc.SQLServerResource"));
nativeEnableAllCharsets.produce(new NativeEnableAllCharsetsBuildItem());
nativeEnableAllCharsets.produce(new NativeImageEnableAllCharsetsBuildItem());
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,8 @@

import io.quarkus.deployment.annotations.BuildStep;
import io.quarkus.deployment.builditem.FeatureBuildItem;
import io.quarkus.deployment.builditem.NativeEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllCharsetsBuildItem;
import io.quarkus.deployment.builditem.NativeImageEnableAllTimeZonesBuildItem;
import io.quarkus.deployment.builditem.nativeimage.NativeImageResourceBuildItem;

public class JDBCMySQLProcessor {
Expand All @@ -17,7 +18,12 @@ NativeImageResourceBuildItem resource() {
}

@BuildStep
NativeEnableAllCharsetsBuildItem enableAllCharsets() {
return new NativeEnableAllCharsetsBuildItem();
NativeImageEnableAllCharsetsBuildItem enableAllCharsets() {
return new NativeImageEnableAllCharsetsBuildItem();
}

@BuildStep
NativeImageEnableAllTimeZonesBuildItem enableAllTimeZones() {
return new NativeImageEnableAllTimeZonesBuildItem();
}
}
2 changes: 1 addition & 1 deletion integration-tests/jpa-mysql/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
<description>Module that contains JPA related tests running with the MySQL database</description>

<properties>
<mysql.url>jdbc:mysql://localhost:3306/hibernate_orm_test?serverTimezone=UTC</mysql.url>
<mysql.url>jdbc:mysql://localhost:3306/hibernate_orm_test</mysql.url>
</properties>

<dependencies>
Expand Down