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

Update native encoding detection for JEP400 #1393

Conversation

@matthiasblaesing
Copy link
Member

@matthiasblaesing matthiasblaesing commented Oct 24, 2021

JEP 400 will make UTF-8 the default value for file.encoding for java:

https://openjdk.java.net/jeps/400

For JNA this means on windows (on mac OS and Linux the encoding for native can be expected to be UTF-8 already) the native codepage would not be used anymore.

See the comments of the first commit for an explanation how it will be reworked.

@matthiasblaesing
Copy link
Member Author

@matthiasblaesing matthiasblaesing commented Oct 24, 2021

@dbwiddis could you please have a look at this?

// native.encoding, prior versions don't have that property and will
// report NULL for it.
// The algorithm is simple: If native.encoding is set, it will be used
// else the original implementation of Charset#defaultCharset is used
String nativeEncoding = System.getProperty("native.encoding");
Charset nativeCharset = null;
if (nativeEncoding != null) {
try {
nativeCharset = Charset.forName(nativeEncoding);
} catch (Exception ex) {
LOG.log(Level.WARNING, "Failed to get charset for native.encoding value : '" + nativeEncoding + "'", ex);
}
}
Copy link
Contributor

@tresf tresf Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this introduce a warning on initialization of Native class for all JDKs older than JDK18? Is that intended? Would this be a custom property like the boot.library.path where JNA should introduce a custom property? Otherwise, there would be no way to remove runtime warnings, right?

Copy link
Member Author

@matthiasblaesing matthiasblaesing Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On JDKs prior to 18 native.encoding will not be set. In that case nativeEncoding will be null and the if block will not be entered (see the check in line 131). From my POV the only way to get a warning, is when native.encoding is set to an invalid value. This can only happen

  • if there is a bug in the JDK
  • you tried to override the property in an invalid way

both cases are valid warning reasons.

Copy link
Contributor

@tresf tresf Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry missed the !=. 👌

@@ -768,6 +768,8 @@ osname=macosx;processor=aarch64
<property name="file.reference.jna.build" location="${build}"/>
<property name="file.reference.jna.jar" location="${build}/${jar}"/>
<property name="libs.junit.classpath" refid="test.libs"/>
<property name="javac.source" value="${compatibility}" />
Copy link
Contributor

@tresf tresf Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you help explain where the variable ${compatibility} introduced in the codebase?

Copy link
Member Author

@matthiasblaesing matthiasblaesing Oct 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is determined in the build script itself:

jna/build.xml

Lines 98 to 110 in 65019cc

<!--
Default compatibility, 1.6, or whatever version is running
Release builds of JNA target 1.6 and should be build on JDK 8, as JDK 9
introduced changes in the ByteBuffer class, which result in classes, that
can't be loaded on Java 6.
JDK 11 is the last JDK, that supports creation of Java 6 compatible class
files.
-->
<condition property="compatibility" value="1.6" else="9">
<matches pattern="^1\.\d+$" string="${ant.java.version}"/>
</condition>

Copy link
Contributor

@tresf tresf Nov 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is determined in the build script itself:

Thanks!

@tresf
Copy link
Contributor

@tresf tresf commented Oct 26, 2021

@matthiasblaesing what is the idea behind introducing the JDK source and target properties in each of the contrib build files (e.g. rather than simply providing this as a single property?)

@matthiasblaesing
Copy link
Member Author

@matthiasblaesing matthiasblaesing commented Oct 26, 2021

@matthiasblaesing what is the idea behind introducing the JDK source and target properties in each of the contrib build files (e.g. rather than simply providing this as a single property?)

The properties javac.source and javac.target are inspired by the setup of ntservice. The ant script generated by NetBeans already supports these properties and I did not want to reinvent the wheel.

…mine default charset

JNA used the defaultCharset to determine which encoding to use when
converting strings to native char*. The defaultCharset is set from
the system property file.encoding. Up to JDK 17 its value defaulted
to the system default encoding. From JDK 18 onwards its default value
changed to UTF-8.
JDK 18+ exposes the native encoding as the new system property
native.encoding, prior versions don't have that property and will
report NULL for it.
The algorithm is simple: If native.encoding is set, it will be used
else the original implementation of Charset#defaultCharset is used.
@matthiasblaesing matthiasblaesing force-pushed the native_encoding_jdk18 branch from 65019cc to 06ec1e4 Oct 29, 2021
@matthiasblaesing matthiasblaesing merged commit 18d0988 into java-native-access:master Nov 3, 2021
9 checks passed
@tresf
Copy link
Contributor

@tresf tresf commented Nov 5, 2021

@matthiasblaesing what is the idea behind introducing the JDK source and target properties in each of the contrib build files (e.g. rather than simply providing this as a single property?)

The properties javac.source and javac.target are inspired by the setup of ntservice. The ant script generated by NetBeans already supports these properties and I did not want to reinvent the wheel.

Ok, it just seems odd to declare a constant used by all ant scripts redundantly versus in a dedicated property file. It's not a matter of reinvention, but centralization, ant doesn't care if the property comes from a shared file, command line, etc, it will always be immutable and the first instance (e.g. NetBeans via CLI) will override what's in the xml/properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants