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

SD-256 enable colored output by default on unix #5663

Merged

Conversation

gourlaysama
Copy link
Contributor

This enables -Dscala.color by default when running via the shell
script and when stdout is a terminal.

Can be overriden on the CLI or via JAVA_OPTS:

$ qbin/scala -nc -e 'println(util.Properties.propOrFalse("scala.color"))'
true
$ qbin/scala -nc -e 'println(util.Properties.propOrFalse("scala.color"))' | cat
false
$ qbin/scala -nc -Dscala.color=false -e 'println(util.Properties.propOrFalse("scala.color"))'
false
$ JAVA_OPTS="-Dscala.color=false" qbin/scala -nc -e 'println(util.Properties.propOrFalse("scala.color"))'
false

@adriaanm
Copy link
Contributor

Sweet! Thanks! Anyone out there familiar with unix gotchas like this? I'm not qualified to review, but would love to get this in!

# enable colored output by default
# we prepend it to JAVA_OPTS so users can override it on the
# command line or via JAVA_OPTS
[[ -t 1 ]] && JAVA_OPTS="-Dscala.color=true $JAVA_OPTS"
Copy link
Member

Choose a reason for hiding this comment

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

Rather than mess directly with JAVA_OPTS I would follow the precendence set by -Dscala.usejavacp/OVERRIDE_USEJAVACP

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally didn't want to clutter the command line to run with one more variable, but you're right, it's cleaner that way. Done.

@gourlaysama gourlaysama force-pushed the ticket/sd-256-enable-repl-colors-unix-2 branch from b103b06 to a842077 Compare January 30, 2017 18:25
@smarter
Copy link
Member

smarter commented Jan 30, 2017

That only solves part of the problem, it would also be nice to get colors when running sbt console for example, for this the default value of scala.color should be auto which would behave like grep --color=auto and many other tools, but this means that the terminal detection must be done on the JVM.

@smarter
Copy link
Member

smarter commented Jan 30, 2017

@gourlaysama
Copy link
Contributor Author

The original reason I did it in the bash script was that we only want to activate it by default on unix-like things.

Hmm, having an auto mode that tests System.console would indeed make it work all on its own in sbt and with the bash runner. Then I can add a -Dscala.color=false default in the bat runner for windows.
Maybe that's a better solution?

@smarter
Copy link
Member

smarter commented Jan 30, 2017

I think the Windows console supports color now, though I have no idea how to test for that: https://blogs.msdn.microsoft.com/commandline/2016/09/22/24-bit-color-in-the-windows-console/ Simply disabling color in the batch script might be enough.

@smarter
Copy link
Member

smarter commented Jan 30, 2017

@gourlaysama
Copy link
Contributor Author

Or always disable if the platform is windows

Oh, right, I'll just do that.

I think the Windows console supports color now, though I have no idea how to test for that

After a bit of digging: it is enabled by default when used from the Windows Subsystem for Linux, but not for normal windows console programs run directly or from cmd or powershell. cmd.exe does enable it for itself, so it works within a .bat script, but java.exe doesn't, and that property isn't inherited by child processes for compatibility reasons. So no color out of the box for the normal usecase. We could enable it, but it is a native API, so JNI and all that... let's just keep it disabled on Windows.

@gourlaysama
Copy link
Contributor Author

@smarter done. Now it works in SBT too :)

I moved the property somewhere more central since it is already used in two places. The next step would be to use it for a few more things, like the default ConsoleReporter, and maybe more things in the repl.

@gourlaysama
Copy link
Contributor Author

library/mima failed: java.lang.RuntimeException: MiMa failed with exit code 2

😦

@adriaanm
Copy link
Contributor

adriaanm commented Feb 8, 2017

Awesome! Yeah, scala's package-private can't be represented in byte code. Will need whitelisting :-/

@gourlaysama gourlaysama force-pushed the ticket/sd-256-enable-repl-colors-unix-2 branch 2 times, most recently from abf6948 to 1118b52 Compare February 13, 2017 15:56
@lrytz
Copy link
Member

lrytz commented Feb 21, 2017

LGTM - needs a rebase unfortunately

`scala.color` now has 3 states: `true`, `false` and `auto`
(the default). `auto` allows colors if not on windows and if the shell
is interactive (as in, both stdin and stdout are a tty).

The autodetect works as expected when run via SBT too, and it can always
be overriden on the CLI or via JAVA_OPTS.
@gourlaysama gourlaysama force-pushed the ticket/sd-256-enable-repl-colors-unix-2 branch from 1118b52 to 6411170 Compare February 21, 2017 14:41
@gourlaysama
Copy link
Contributor Author

Rebased.

@lrytz
Copy link
Member

lrytz commented Feb 21, 2017

/rebuild - interesting failure

Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)

@adriaanm adriaanm merged commit a8c4a54 into scala:2.12.x Feb 21, 2017
@adriaanm
Copy link
Contributor

🎉

@gourlaysama gourlaysama deleted the ticket/sd-256-enable-repl-colors-unix-2 branch February 21, 2017 20:37
@SethTisue SethTisue added the release-notes worth highlighting in next release notes label Feb 22, 2017
@SethTisue
Copy link
Member

Now it works in SBT too

confirmed. 🙌

gourlaysama added a commit to gourlaysama/scala-partest that referenced this pull request Feb 23, 2017
Partest shows a single `Task` to sbt and does all the concurrency and
running subtasks itself, which isn't really how this should work, so sbt
doesn't expect the task to send back events concurrently:
`EventHandler.handle` adds events to an `ArrayList` when forking (see
[ForkMain][2]), and is called after each test, and if it happens on two
threads at exactly the wrong moment:

```
Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)
```

See scala/scala#5663 ([build log][1] or the failed run).

[1]: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/4300/consoleFull
[2]: https://github.com/sbt/sbt/blob/v0.13.13/testing/agent/src/main/java/sbt/ForkMain.java#L294
SethTisue added a commit to SethTisue/scala that referenced this pull request Mar 6, 2017
only trivial merge conflict involving a method renaming

*   d1d700e - (origin/HEAD, origin/2.12.x) Merge pull request scala#5754 from Philippus/issue/html-tag-in-hover (2 days ago) <Lukas Rytz>
|\
| * cf64718 - pattern for entitylink was too narrow, cleaned up the tests (3 days ago) <Philippus Baalman>
* |   f771395 - Merge pull request scala#5671 from retronym/topic/stubby-2 (3 days ago) <Lukas Rytz>
|\ \
| * | ad13063 - Remove non-essential fix for stub symbol failure (4 days ago) <Jason Zaugg>
| * | 7f2d1fa - Avoid forcing info transforms of primitive methods (2 weeks ago) <Jason Zaugg>
| * | 37a0eb7 - Avoid stub symbol related crash in backend (2 weeks ago) <Jason Zaugg>
* | |   96a7617 - Merge pull request scala#5622 from edmundnoble/extra-errs (4 days ago) <Adriaan Moors>
|\ \ \
| * | | 466e52b - Match error lengths (4 weeks ago) <Edmund Noble>
| * | | d1fc983 - Improved error messages for identically named, differently prefixed types (9 weeks ago) <Edmund Noble>
|  / /
* | |   f2e05c2 - Merge pull request scala#5728 from Philippus/issue/html-tag-in-hover (4 days ago) <Lukas Rytz>
|\ \ \
| | |/
| |/|
| * | e3c5df8 - added missing Inline matches to inlineToStr so it is now exhaustive scala.xml.XML.loadString(tag).text will remove all html tags inside the HtmlTag (9 days ago) <Philippus Baalman>
* | |   920bc4e - Merge pull request scala#5743 from som-snytt/issue/10207-bad-update (7 days ago) <Lukas Rytz>
|\ \ \
| * | | 094f7f9 - SI-10207 Error before update conversion (8 days ago) <Som Snytt>
* | | |   1b4d36f - Merge pull request scala#5746 from paulp/pr/partest (7 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | eac00e1 - Add partest paths to the list of watched sources. (8 days ago) <Paul Phillips>
|/ / /
* | |   5f1a638 - Merge pull request scala#5732 from retronym/topic/build-info-malarkey (10 days ago) <Adriaan Moors>
|\ \ \
| * | | 5e9acac - More predictable performance of SBT build startup, reload (11 days ago) <Jason Zaugg>
|  / /
* | |   759a7b7 - Merge pull request scala#5735 from SethTisue/sd-313 (10 days ago) <Adriaan Moors>
|\ \ \
| * | | ed4ddf5 - increase timeouts on some sys.process tests (11 days ago) <Seth Tisue>
* | | |   e2aaf2c - Merge pull request scala#5723 from dragos/issue/regression-assert-ide (11 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | e5c957e - Fix regression in 5751763 (12 days ago) <Iulian Dragos>
* | | |   f174bfb - Merge pull request scala#5731 from janekdb/issue/scalaGH-644/fix-spec-latex-rendering (11 days ago) <Seth Tisue>
|\ \ \ \
| * | | | ba4c6d4 - scalaGH-644: Remove static html styling of spec code blocks (11 days ago) <Janek Bogucki>
|/ / / /
* | | |   8e40bef - Merge pull request scala#5729 from scala/revert-5658-topic/hashhash (12 days ago) <Adriaan Moors>
|\ \ \ \
| |_|/ /
|/| | |
| * | | 86cd70f - (origin/revert-5658-topic/hashhash) Revert "Fix erasure of the qualifier of ##" (12 days ago) <Adriaan Moors>
|/ / /
* | |   cbf7daa - Merge pull request scala#5681 from Philippus/issue/9704 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | b8a8ac1 - moved Pattern and TagsNotToClose to a HtmlTag companion object (13 days ago) <Philippus Baalman>
| * | | a019082 - SI-9704 don't add a closed HtmlTag if it is already closed (4 weeks ago) <Philippus Baalman>
|  / /
* | |   effde0c - Merge pull request scala#5726 from scala/revert-5629-issue/10120-quote-err (13 days ago) <Adriaan Moors>
|\ \ \
| * | | d60f6e3 - (origin/revert-5629-issue/10120-quote-err) Revert "SI-10133 Require escaped single quote char lit" (13 days ago) <Adriaan Moors>
|/ / /
* | |   a8c4a54 - Merge pull request scala#5663 from gourlaysama/ticket/sd-256-enable-repl-colors-unix-2 (13 days ago) <Adriaan Moors>
|\ \ \
| * | | 6411170 - SD-256 enable colored output by default on unix (13 days ago) <Antoine Gourlay>
* | | |   c96a977 - Merge pull request scala#5658 from retronym/topic/hashhash (13 days ago) <Lukas Rytz>
|\ \ \ \
| |/ / /
|/| | |
| * | | f85c62e - Fix erasure of the qualifier of ## (6 weeks ago) <Jason Zaugg>
|  / /
* | |   76bfb9e - Merge pull request scala#5708 from szeiger/issue/si10194 (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 1d22ee4 - SI-10194: Fix abstract type resolution for overloaded HOFs (13 days ago) <Stefan Zeiger>
|  / /
* | |   dabec1a - Merge pull request scala#5700 from retronym/ticket/10154-refactor (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 06eee79 - Refactor implementation of lookupCompanion (2 weeks ago) <Jason Zaugg>
|  / /
* | |   2f1e0c2 - Merge pull request scala#5704 from som-snytt/issue/10190-elide-string (13 days ago) <Lukas Rytz>
|\ \ \
| * | | 6fb3825 - SI-10190 Elide string to empty instead of null (3 weeks ago) <Som Snytt>
|  / /
* | |   13f7b2a - Merge pull request scala#5640 from optimizely/repl-import-handler (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | aa7e335 - Fix ImportHandler's reporting of importedNames and importedSymbols (8 weeks ago) <Hao Xia>
| * | | c89d821 - Fix SIOOBE in Name#pos for substrings of length 1 (8 weeks ago) <Jason Zaugg>
|  / /
* | |   023a96a - Merge pull request scala#5629 from som-snytt/issue/10120-quote-err (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | 05cc3e2 - SI-10120 ReplReporter handles message indent (7 weeks ago) <Som Snytt>
| * | | 939abf1 - SI-10120 Extra advice on unclosed char literal (8 weeks ago) <Som Snytt>
| * | | 855492c - SI-10133 Require escaped single quote char lit (8 weeks ago) <Som Snytt>
|  / /
* | |   e21ab42 - Merge pull request scala#5660 from som-snytt/issue/9464-spec (2 weeks ago) <Adriaan Moors>
|\ \ \
| * | | a6dcceb - SI-9464 Clarify spec on no final trait (6 weeks ago) <Som Snytt>
|  / /
* | |   e87a436 - Merge pull request scala#5659 from retronym/ticket/10026 (2 weeks ago) <Adriaan Moors>
|\ \ \
| |/ /
|/| |
| * | 777a0e5 - SI-10026 Fix endless cycle in runtime reflection (2 weeks ago) <Jason Zaugg>
| |/
* |   23e5ed9 - Merge pull request scala#5707 from retronym/topic/java9-prepare (2 weeks ago) <Lukas Rytz>
|\ \
| * | 96e8e97 - Workaround bug in Scala runtime reflection on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | fe2d9a4 - Avoid ambiguous overload on JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 6bba8f7 - Adapt to change in ClassLoader in JDK 9 (3 weeks ago) <Jason Zaugg>
| * | 8136057 - Bump scala-asm version (3 weeks ago) <Jason Zaugg>
|  /
* |   cad3c3d - Merge pull request scala#5709 from adriaanm/sam_wild_bound (2 weeks ago) <Lukas Rytz>
|\ \
| * | c396e96 - Ignore BoundedWildcardType in erasure type map (2 weeks ago) <Adriaan Moors>
* | |   3e9df41 - Merge pull request scala#5711 from retronym/ticket/jrt (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 09c7edc - Faster and simpler Java 9 classpath implementation (2 weeks ago) <Jason Zaugg>
|  / /
* | |   7b9d3cc - Merge pull request scala#5713 from janekdb/issue/scalaGH-644/sync-jekyll-README-to-Gemfile (2 weeks ago) <Lukas Rytz>
|\ \ \
| * | | 5e5ec9a - scalaGH-644: Expand note regarding Jekyll versions (2 weeks ago) <Janek Bogucki>
|  / /
* | |   144f7e0 - Merge pull request scala#5714 from dragos/issue/usage-sterr-SI-10178 (2 weeks ago) <Lukas Rytz>
|\ \ \
| |/ /
|/| |
| * | 640c85e - SI-10178 Route reporter.echo to stdout (2 weeks ago) <Iulian Dragos>
|  /
* |   2fec08b - Merge pull request scala#5717 from som-snytt/issue/10148-followup (2 weeks ago) <Adriaan Moors>
|\ \
| |/
|/|
| * f3d271b - SI-10148 Accept verbose zero (2 weeks ago) <Som Snytt>
* 147e5dd - Merge pull request scala#5716 from adriaanm/i296 (2 weeks ago) <Jason Zaugg>
* 12437a0 - Ensure ordering for args to `choose` in DurationTest (2 weeks ago) <Adriaan Moors>
@som-snytt
Copy link
Contributor

Just noticed my scripts with -Dscala.color stopped working. That is, it's no longer taken as -Dscala.color=true or even auto.

lrytz pushed a commit to lrytz/scala-partest that referenced this pull request May 9, 2018
Partest shows a single `Task` to sbt and does all the concurrency and
running subtasks itself, which isn't really how this should work, so sbt
doesn't expect the task to send back events concurrently:
`EventHandler.handle` adds events to an `ArrayList` when forking (see
[ForkMain][2]), and is called after each test, and if it happens on two
threads at exactly the wrong moment:

```
Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)
```

See scala/scala#5663 ([build log][1] or the failed run).

[1]: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/4300/consoleFull
[2]: https://github.com/sbt/sbt/blob/v0.13.13/testing/agent/src/main/java/sbt/ForkMain.java#L294
lrytz pushed a commit to lrytz/scala-partest that referenced this pull request May 9, 2018
Partest shows a single `Task` to sbt and does all the concurrency and
running subtasks itself, which isn't really how this should work, so sbt
doesn't expect the task to send back events concurrently:
`EventHandler.handle` adds events to an `ArrayList` when forking (see
[ForkMain][2]), and is called after each test, and if it happens on two
threads at exactly the wrong moment:

```
Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)
```

See scala/scala#5663 ([build log][1] or the failed run).

[1]: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/4300/consoleFull
[2]: https://github.com/sbt/sbt/blob/v0.13.13/testing/agent/src/main/java/sbt/ForkMain.java#L294
lrytz pushed a commit to lrytz/scala that referenced this pull request May 9, 2018
Partest shows a single `Task` to sbt and does all the concurrency and
running subtasks itself, which isn't really how this should work, so sbt
doesn't expect the task to send back events concurrently:
`EventHandler.handle` adds events to an `ArrayList` when forking (see
[ForkMain][2]), and is called after each test, and if it happens on two
threads at exactly the wrong moment:

```
Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)
```

See scala#5663 ([build log][1] or the failed run).

[1]: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/4300/consoleFull
[2]: https://github.com/sbt/sbt/blob/v0.13.13/testing/agent/src/main/java/sbt/ForkMain.java#L294
lrytz pushed a commit to lrytz/scala that referenced this pull request May 9, 2018
Partest shows a single `Task` to sbt and does all the concurrency and
running subtasks itself, which isn't really how this should work, so sbt
doesn't expect the task to send back events concurrently:
`EventHandler.handle` adds events to an `ArrayList` when forking (see
[ForkMain][2]), and is called after each test, and if it happens on two
threads at exactly the wrong moment:

```
Caused by: sbt.ForkMain$ForkError: java.lang.ArrayIndexOutOfBoundsException: 15
	at java.util.ArrayList.add(ArrayList.java:459)
	at sbt.ForkMain$Run$2$1.handle(ForkMain.java:294)
	at scala.tools.partest.sbt.SBTRunner$$anon$1.onFinishTest(SBTRunner.scala:70)
	at scala.tools.partest.nest.SuiteRunner.runTest(Runner.scala:781)
	at scala.tools.partest.nest.SuiteRunner.$anonfun$runTestsForFiles$2(Runner.scala:788)
	at scala.tools.partest.package$$anon$2.call(package.scala:135)
```

See scala#5663 ([build log][1] or the failed run).

[1]: https://scala-ci.typesafe.com/job/scala-2.12.x-validate-test/4300/consoleFull
[2]: https://github.com/sbt/sbt/blob/v0.13.13/testing/agent/src/main/java/sbt/ForkMain.java#L294
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes worth highlighting in next release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants