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 console encoding on JDK 18 (Maven + general execution) #3836

Merged

Conversation

matthiasblaesing
Copy link
Contributor

With JDK 18 the default charset for java was changed. Before this
change, the default charset (exposed as the system property
file.encoding) was platform specific. After the change the default
charset was unified to UTF-8 on all platforms.

Code that interacts with native APIs can use the system property
native.encoding to query the platform encoding and use that. And related
to this the same is true for applications interacting with the system
console.

The primarly affected platform is windows, as both mac OS and Linux
already default to UTF-8 as file encoding.

In this PR two areas are covered:

  • General execution (using "External Execution API"): The module already
    allows users to specify the encoding to use. It used the default
    enconding as a fallback, if none was specified. This fallback was
    enhanced to query the native.encoding system property first. If it is
    present, it will be used as default encoding (JDK 18+), if it is
    missing, the original behavior using the default file encoding is
    activated.

  • Maven target execution: maven target execution is basicly an external
    execution, so the same considerations apply. In contrast to the
    general execution API, the encoding for maven execution can't be
    overriden, so both code path (input + output) are configured to use
    the native.enconfig property if present or fallback to the default
    file encoding.

With JDK 18 the default charset for java was changed. Before this
change, the default charset (exposed as the system property
file.encoding) was platform specific. After the change the default
charset was unified to UTF-8 on all platforms.

Code that interacts with native APIs can use the system property
native.encoding to query the platform encoding and use that. And related
to this the same is true for applications interacting with the system
console.

The primarly affected platform is windows, as both mac OS and Linux
already default to UTF-8 as file encoding.

In this PR two areas are covered:

- General execution (using "External Execution API"): The module already
  allows users to specify the encoding to use. It used the default
  enconding as a fallback, if none was specified. This fallback was
  enhanced to query the native.encoding system property first. If it is
  present, it will be used as default encoding (JDK 18+), if it is
  missing, the original behavior using the default file encoding is
  activated.

- Maven target execution: maven target execution is basicly an external
  execution, so the same considerations apply. In contrast to the
  general execution API, the encoding for maven execution can't be
  overriden, so both code path (input + output) are configured to use
  the native.enconfig property if present or fallback to the default
  file encoding.
@matthiasblaesing
Copy link
Contributor Author

matthiasblaesing commented Mar 22, 2022

This fixes encoding problems for maven execution introduced by JEP 400. The aim is not to reply on COMPAT encoding proposed by #3265.

This does not yet fix ant execution. That will need to be a followup.

@mbien could you please have a look at this?

@mbien
Copy link
Member

mbien commented Mar 22, 2022

@matthiasblaesing I like the change, good job.

My main concern was that there might be something we overlook, since NB is a big codebase. We should ask windows users to test on JDK 18 once there is a RC to get more coverage.

Do you think we could simulate the JDK 18 behavior on JDK 11 by setting native.encoding to system native and file.encoding to UTF-8 and run a CI job on windows?

One cosmetic nitpick would be that this is duplicated code, but unless more places show up where we need it its probably ok to leave it like this. Potentially add a comment to inform that the code needs to be updated in two places if there are any changes.

I am going to close #3265 and disconnect tit from the issue.

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

Successfully merging this pull request may close these issues.

None yet

2 participants