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: add xlint and file.encoding to Java #2471

Merged
merged 1 commit into from
May 13, 2021

Conversation

mhatano
Copy link
Contributor

@mhatano mhatano commented May 13, 2021

Adding back UTF-8 encoding related options and -Xlint. Tested on Windows 10 and Ubuntu 21.04.

@mhatano
Copy link
Contributor Author

mhatano commented May 13, 2021

  • Adding back '-J-Dfile.encoding=UTF-8' for javac so that its warning messages are printed in UTF-8 as well as its source files are treated as UTF-8 encoding.
  • Removing '-encoding UTF-8' for javac since that is covered by -J-Dfile.encoding=UTF-8 option
  • Adding back -Xlint for reaporting detailed warning messages for compilers
  • removing quotation marks around file.encoding and UTF-8 for java command, since java command does not understand quotation marks when it is not removed by shell (cmd.exe won't interpret that)
  • confirmed working correctly on Windows 10 and Ubuntu 21.04

update for issue #1166

@@ -8,7 +8,7 @@ function JavaArgs(filepath, context) {
const className = GrammarUtils.Java.getClassName(context)
const classPackages = GrammarUtils.Java.getClassPackage(context)
const tempFolder = GrammarUtils.createTempFolder("jar-")
const cmd = `javac -encoding UTF-8 -sourcepath '${sourcePath}' -d '${tempFolder}' '${filepath}' && java -D'file.encoding'='UTF-8' -cp '${tempFolder}' ${classPackages}${className}`
const cmd = `javac -J-Dfile.encoding=UTF-8 -Xlint -sourcepath '${sourcePath}' -d '${tempFolder}' '${filepath}' && java -Dfile.encoding=UTF-8 -cp '${tempFolder}' ${classPackages}${className}`
Copy link
Member

@aminya aminya May 13, 2021

Choose a reason for hiding this comment

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

Based on the docs -encoding UTF-8 is a valid way to set the encoding.
https://docs.oracle.com/en/java/javase/13/docs/specs/man/javac.html
https://docs.oracle.com/javase/8/docs/technotes/tools/windows/javac.html

What version of Java are you trying this on? I added a test and tested this on Windows, and I had no issues.

This is running examples/helloworld.java on master branch:
image

Suggested change
const cmd = `javac -J-Dfile.encoding=UTF-8 -Xlint -sourcepath '${sourcePath}' -d '${tempFolder}' '${filepath}' && java -Dfile.encoding=UTF-8 -cp '${tempFolder}' ${classPackages}${className}`
const cmd = `javac -encoding UTF-8 -Xlint -sourcepath '${sourcePath}' -d '${tempFolder}' '${filepath}' && java -Dfile.encoding=UTF-8 -cp '${tempFolder}' ${classPackages}${className}`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"-encoding UTF-8" will only set source file encoding to UTF-8. Since javac.exe will produce its warning/error messages in its default encoding (for exapmle, in Japanese Windows, it's CP932/ShiftJIS) and in local language (such as Japanese) so it will become garbled. -J-Dfile.encoding=UTF-8 will switch whole output messages of javac.exe to UTF-8.

For example, if you implement java.io.Serializable in your main class and won't set serialversionUID, javac will output warning message, and with '-J-Dfile.encoding=UTF-8', it looks like this:
image
while only with "-encoding UTF-8" option, this would show:
image

for -J option, please check "Standard Options" section in https://docs.oracle.com/en/java/javase/11/tools/javac.html

Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we set both?

Copy link
Contributor Author

@mhatano mhatano May 13, 2021

Choose a reason for hiding this comment

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

not necessarily I suppose. '-J-Dfile.encoding=UTF-8' will do the job, and I am sure in my environment '-encoding UTF-8' is not there and the package works fine.

Copy link
Member

Choose a reason for hiding this comment

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

I added it just in case. Could you update your branch and try it now so I can merge this?

* Adding back '-J-Dfile.encoding=UTF-8' for javac so that its warning messages are printed in UTF-8 as well as its source files are treated as UTF-8 encoding.
* Removing '-encoding UTF-8' for javac since that is covered by -J-Dfile.encoding=UTF-8 option
* Adding back -Xlint for reaporting detailed warning messages for compilers
* removing quotation marks around file.encoding and UTF-8 for java command, since java command does not understand quotation marks when it is not removed by shell (cmd.exe won't interpret that)

Co-Authored-By: weather-tracker <[email protected]>
@aminya aminya force-pushed the mhatano-patch-2 branch from cbaf396 to 54a0a7f Compare May 13, 2021 06:59
@aminya aminya changed the title Update java.js fix: add xlint and file.encoding to Java May 13, 2021
@mhatano
Copy link
Contributor Author

mhatano commented May 13, 2021

Moving detailed explanations to next comment to shorten pull-request message.

@aminya
Copy link
Member

aminya commented May 13, 2021

That's because of the commit message, not the pull request. But thank you! I will fix it once I want to merge this.

@mhatano
Copy link
Contributor Author

mhatano commented May 13, 2021

okay.

@aminya aminya merged commit e730f08 into atom-community:master May 13, 2021
@github-actions
Copy link

🎉 This PR is included in version 3.32.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mhatano mhatano deleted the mhatano-patch-2 branch May 13, 2021 08:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants