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

Proper UTF-8 encoding and line endings on Windows #5457

Merged
merged 19 commits into from Nov 21, 2018

Conversation

Projects
None yet
2 participants
@michelou
Copy link
Contributor

michelou commented Nov 16, 2018

This PR is correlated with PR #5159; it includes a few changes (line endings) also present in that request.

I have tested the submitted changes on a Windows machine running the batch script project\scripts\build.bat (part of PR #5444) which particularly reproduces the two steps clone and compile of Dotty CI.

The changes address two issues on Windows:

  • Unspecified text encoding in some file operations
    Examples: Source.fromFile(f)Source.fromFile(f, "UTF-8")
    new PrintStream(os)new PrintStream(os, autoFlush, "UTF-8")
  • Platform-specific new lines
    Example: "\n"sys.props("line.separator")
@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Nov 19, 2018

Can this PR only include the last commit? I am happy to review it separately

@allanrenucci
Copy link
Member

allanrenucci left a comment

Otherwise LGTM.

Do we still have failing tests on Windows after this patch?

@@ -24,8 +26,9 @@ class DecompilationPrinter extends Phase {
var os: OutputStream = null
var ps: PrintStream = null
try {
implicit val codec = Codec.UTF8
os = File(outputDir.fileNamed("decompiled.scala").path).outputStream(append = true)

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

I would not introduce an implicit in scope but pass the implicit explicitly:

Suggested change Beta
os = File(outputDir.fileNamed("decompiled.scala").path).outputStream(append = true)
os = File(outputDir.fileNamed("decompiled.scala").path)(Codec.UTF8).outputStream(append = true)
os = File(outputDir.fileNamed("decompiled.scala").path).outputStream(append = true)
ps = new PrintStream(os)
ps = new PrintStream(os, /*autoFlush*/false, "UTF-8")

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

I would mimic the named argument syntax:

Suggested change Beta
ps = new PrintStream(os, /*autoFlush*/false, "UTF-8")
ps = new PrintStream(os, /* autoFlush = */ false, "UTF-8")
@@ -15,6 +15,9 @@ import scala.annotation.switch
import scala.collection.mutable

trait MessageRendering {

private final val EOL: String = sys.props("line.separator")

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

Remove and import:

import java.lang.System.{lineSeparator => EOL}
@@ -5,6 +5,8 @@ import org.junit.{Ignore, Test}

class ReplCompilerTests extends ReplTest {

private final val EOL: String = sys.props("line.separator")

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

Remove and import:

import java.lang.System.{lineSeparator => EOL}
@@ -14,6 +14,8 @@ import dotc.reporting.MessageRendering
/** Runs all tests contained in `compiler/test-resources/repl/` */
class ScriptedTests extends ReplTest with MessageRendering {

private final val EOL: String = sys.props("line.separator")

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

Remove and import:

import java.lang.System.{lineSeparator => EOL}
@@ -187,6 +187,8 @@ trait ParallelTesting extends RunnerOrchestration { self =>
protected final val realStdout = System.out
protected final val realStderr = System.err

protected final val EOL: String = sys.props("line.separator")

This comment has been minimized.

@allanrenucci

allanrenucci Nov 21, 2018

Member

Remove and import:

import java.lang.System.{lineSeparator => EOL}

@allanrenucci allanrenucci merged commit eb175cb into lampepfl:master Nov 21, 2018

1 check passed

continuous-integration/drone/pr the build was successful
Details
@allanrenucci

This comment has been minimized.

Copy link
Member

allanrenucci commented Nov 21, 2018

Thanks @michelou

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment