-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
8274544: Langtools command's usage were garbled on Japanese Windows #5771
Conversation
👋 Welcome back itakiguchi! A progress list of the required criteria for merging this PR into |
Webrevs
|
javac does not use PrintStream for standard out/err, it uses PrintWriter.
So I added -Dfile.encoding=COMPAT expect java and javaw commands on launcher. jshell does not work as expected on b12
on b13
It's UTF-8, not native encoding. I don't think it's good fix, so please give me some advices. |
/label i18n |
@takiguc |
What was the cause of the failure?
I think we should fix the root cause of this, instead of specifying
Do you mean
No it should not. Naoto |
@naotoj
I got following exception via tools/javac/diags/CheckResourceKeys.java
About jshell, my sample was not good,
By b13
By b13 with file.encoding=COMPAT
If I need to create another issue, please let me know. |
Regarding javac, the patch to Regarding JShell, I guess I need to try to find a way to reproduce for me, so that I can debug. AFAIK the main process does not read/write from/to the console using |
For searchability, you could fix the typo in the PR title and JBS summary: grABled -> gARbled. A nit, really. |
I've forgot to write a note on the test, sorry: simply add |
@pavelrappo Many thanks. |
The encoding used in |
Helllo @naotoj . The following executables had same issue, I fixed them.
I could not find out the following executables had same issue or not
The following executables worked fine.
The following executables were GUI apps
These fixes don't have testcases. |
@takiguc - if JShell is still an issue, is there a chance you could try this: Not sure if it will help, but it might (this won't change the default charset, but could send the output in a sensible encoding for the console. Thanks! |
src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
Outdated
Show resolved
Hide resolved
src/jdk.compiler/share/classes/com/sun/tools/javac/util/Log.java
Outdated
Show resolved
Hide resolved
Thanks, @lahodaj . I tested your code, it worked fine on standard case ! But, to execute previous saved command list, user needs to specify -J-Dfile.encoding=COMPAT and -R-Dfile.encoding=COMPAT options. I'd like to discuss jshell things by JDK-8274784. |
I just grepped |
The changes in 4427d87 look okay. I assume most of these tools will never run with a SM enabled and don't need to be granted permission to reading native.encoding. |
Hello @naotoj .
I applied following changes and lahodaj's code (I'm not sure, it's expected one...)
But it did not work for previously saved encoded command list. I think you may be confused because of my bad explanation. |
I got your issue now. Since the current code issues BTW, the suggestion I made in |
Hello @naotoj . |
I'd appreciate it, as I don't have a Japanese Windows environment at hand. |
BTW, does the PoC in the jshell bug report really causing the issue?
This is ASCII, so save/restore does not seem to cause any issues across JDKs with and without JEP400. Did you mean |
@jonathan-gibbons The |
@jonathan-gibbons I appreciate your comment.
I was confused since the fixed code did not call System.out/System.err directly.
Output is:
[2] refers default charset (UTF-8) Could you explain more detail ? |
Terminal setting
Java testcase by using Scanner.
When
|
I needed to rebase my fixed code since JavapTask.java and JdepsTask.java were updated. Hello @jonathan-gibbons .
Hello @naotoj . |
Firstly, please do NOT rebase your fix, as it will clobber the review history. Use merge instead. |
@naotoj @jonathan-gibbons |
Hello @jonathan-gibbons . |
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 strongly dislike this proposed changeset. In my opinion, the original change that has provoked the changes here is a bad/incompatible change, and should maybe be reconsidered. The fact that a change in the Java runtime has triggered the need for so many changes in application-style code is some sort of giant "canary in the coalmine".
Generally, the issue is related to the fact that we wrap a PrintStream in a PrintWriter ... and, if I understand correctly, the writer ends up with the wrong character encoding. Is it possible to change PrintWriter and/or PrintStream so that the correct underlying encoding used by the PrintStream is also used by the PrintWriter. That way, all existing uses where a PrintWriter wraps a PrintStream would continue to work without any modification.
cc: @jddarcy with his CSR hat on, for the compatibility issues relating to the issue that caused the problems being addressed here.
Informally, I suggest one of the following:
I note that |
JEP 400 has moved the JDK to using UTF-8 as the default charset, a long overdue change. So if you create a PrintStream or a PrintWriter without specifying the charset then it will default to UTF-8. The issue that I think we have with javac and a few other tools is that they are creating PrintWriters on PrintStreams where the underlying streams are stdout/stderr and so using the native encoding. There was exploration into special casing this scenario during JEP 400 that was rejected because it complicates the spec and may not be feasible in cases where there are many layers in the onion. If there is resistance to fixing the tools then we might have to re-visit this. Naoto may have more to say on this. |
I think you mean the PrintWriter constructors here. Yes, there is merit in that. PrintStream is a bit unusual in that it's an OutputStream but it has a charset because it prints text output. |
Good suggestions. Filed a JBS issue: https://bugs.openjdk.java.net/browse/JDK-8276970 |
Yes, sorry for the confusion. |
It's not just the JDK tools that are an issue. I think that wrapping some PrintStream into a PrintWriter is a common enough idiom that it is not reasonable to fix all occurrences -- i.e. including those outside of JDK. |
Hello @naotoj .
For javac code, we may not use PrintStream.getCharset() directly because javac code is compiled by boot compiler.
If we add following constructors against PrintWriter, we just change javap and jshell code.
I really appreciate if you handle this kind of code change via JEP-400. |
Generally, I think it would be a good goal for JEP-400 to not require any source-code changes to any use-sites, at least for this common idiom of wrapping a |
The gist of the issue is that |
JEP-400 (UTF-8 by Default) was eabled on JDK18-b13.
After JDK18-b13, javac and some other langtool command's usage were garbled on Japanese Windows.
These commands use PrintWriter instead of standard out/err with PrintStream.
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/5771/head:pull/5771
$ git checkout pull/5771
Update a local copy of the PR:
$ git checkout pull/5771
$ git pull https://git.openjdk.java.net/jdk pull/5771/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 5771
View PR using the GUI difftool:
$ git pr show -t 5771
Using diff file
Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/5771.diff