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.

Sign up for GitHub

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

Jump to bottom

Add a --color flag to the CLI and disable colors for Pkl test #571

Closed
wants to merge 1 commit into from
Closed

Add a --color flag to the CLI and disable colors for Pkl test #571

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pkl-cli/src/main/kotlin/org/pkl/cli/commands/TestCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class TestCommand(helpLink: String) :

override fun run() {
CliTestRunner(
options = baseOptions.baseOptions(modules, projectOptions),
options = baseOptions.baseOptions(modules, projectOptions, disableColors = true),
testOptions = testOptions.cliTestOptions
)
.run()
Expand Down
1 change: 1 addition & 0 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,7 @@ data class CliBaseOptions(

/** Hostnames, IP addresses, or CIDR blocks to not proxy. */
val httpNoProxy: List<String>? = null,
val colors: String = "auto"
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make this an enum:

enum class Color {
  /**
   * Output ANSI-formatted text only when the process STDOUT/STDERR are attached to a TTY.
   */
  AUTO,

  /**
   * Never output ANSI-formatted text.
   */
  NEVER,

  /**
   * Always output ANSI-formatted text.
   */
  ALWAYS
}

And change this type to said enum, and also add a doc comment. Also, nit: s/colors/color

Suggested change
val colors: String = "auto"
/** Whether to output ANSI-formatted text. */
val color: Color = Color.AUTO

) {

companion object {
Expand Down
17 changes: 17 additions & 0 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ import java.nio.file.Files
import java.nio.file.Path
import java.util.regex.Pattern
import kotlin.io.path.isRegularFile
import org.fusesource.jansi.Ansi
import org.fusesource.jansi.AnsiConsole
import org.fusesource.jansi.AnsiMode
import org.pkl.core.*
import org.pkl.core.evaluatorSettings.PklEvaluatorSettings
import org.pkl.core.http.HttpClient
Expand All @@ -38,6 +41,20 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
if (cliOptions.testMode) {
IoUtils.setTestMode()
}
when (cliOptions.colors) {
"never" -> {
// Configure Jansi to not even inject Ansi codes
System.setProperty(Ansi.DISABLE, "true")

// But also strip anything that might end up in the output
AnsiConsole.err().mode = AnsiMode.Strip
AnsiConsole.out().mode = AnsiMode.Strip
}
"always" -> {
AnsiConsole.err().mode = AnsiMode.Force
AnsiConsole.out().mode = AnsiMode.Force
}
}

try {
proxyAddress?.let(IoUtils::setSystemProxy)
Expand Down
12 changes: 10 additions & 2 deletions pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/commands/BaseOptions.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ package org.pkl.commons.cli.commands

import com.github.ajalt.clikt.parameters.groups.OptionGroup
import com.github.ajalt.clikt.parameters.options.*
import com.github.ajalt.clikt.parameters.types.choice
import com.github.ajalt.clikt.parameters.types.int
import com.github.ajalt.clikt.parameters.types.long
import com.github.ajalt.clikt.parameters.types.path
Expand Down Expand Up @@ -198,6 +199,11 @@ class BaseOptions : OptionGroup() {
.single()
.split(",")

val colors: String by
option(names = arrayOf("--colors"), help = "Enable or disable colour output in the terminal")
.choice("auto", "never", "always")
.default("auto")
Comment on lines +202 to +205
Copy link
Contributor

@bioball bioball Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Let's follow the convention set by unix-y tools like ls, diff, grep, etc, and call this --color.

Also, with the above suggestion to enum-ify this (requires import com.github.ajalt.clikt.parameters.types.enum)

Suggested change
val colors: String by
option(names = arrayOf("--colors"), help = "Enable or disable colour output in the terminal")
.choice("auto", "never", "always")
.default("auto")
val color: Color by
option(names = arrayOf("--color"), help = "Enable or disable colour output in the terminal")
.enum<Color>(key = { it.name.lowercase() })
.default(Color.AUTO)


// hidden option used by native tests
private val testPort: Int by
option(names = arrayOf("--test-port"), help = "Internal test option", hidden = true)
Expand All @@ -208,7 +214,8 @@ class BaseOptions : OptionGroup() {
fun baseOptions(
modules: List<URI>,
projectOptions: ProjectOptions? = null,
testMode: Boolean = false
testMode: Boolean = false,
disableColors: Boolean = false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
disableColors: Boolean = false,

): CliBaseOptions {
return CliBaseOptions(
sourceModules = modules,
Expand All @@ -230,7 +237,8 @@ class BaseOptions : OptionGroup() {
noProject = projectOptions?.noProject ?: false,
caCertificates = caCertificates,
httpProxy = proxy,
httpNoProxy = noProxy ?: emptyList()
httpNoProxy = noProxy ?: emptyList(),
colors = if (disableColors) "never" else colors,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
colors = if (disableColors) "never" else colors,
color = color

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This exists to provide a mechanism for the test subcommand to forcefully disable not just colours, but also the injection if ANSI codes completely. I've been having issues because the pkl test expected output because they don't travel along the standard output rails. As a consequence the ANSI codes don't get stripped if they're injected.

By providing this escape hatch, I can ensure that colour are disabled before there's any opportunity for the Jansi formatting object to init, because once it's created you can't disable the injection of ANSI codes, only strip them out later. That mixed with the fact that caught errors in pkl test are truncated creates a bit of nightmare as the outputs get truncated to slightly different lengths depending on if colours were enabled, regardless of if those ANSI code actually made it to the output.

I tried to figure out where the error truncation etc was happening, so I could modify it to handle ANSI/non-ANSI outputs in an equivalent way, but I haven't been able to track down where it happens.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if (when?) we switch to a text formatter, we can control this ourselves by passing a no-op text formatter when --color=never. Then, we're in full control of how formatting happens rather than mutating global state.

Anyways, I don't know if that's relevant to my suggestion here. CliBaseOptions is a data class, and you can always modify it however you want because you get a derived copy method.

val options = baseOptions(myModules).copy(color = Color.NEVER)

Copy link
Contributor Author

@thomaspurchas thomaspurchas Jul 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's only relevant due to how Jansi handles global state. It's caught me out a couple of times because Jansi doesn't handle changes to global state the way you might expect. The final result is what you expect, but the mechanism by which Jansi achieves that result depends on the exact order of operations (mutating global state before or after doing the initial Jansi setup). In most cases it doesn't make any practical difference, but for the specific case of Pkl test it does (which I learned the hard way).

For this specific case I need to make sure the baseOptions are set correctly before the common CLI setup function runs, because that's where Jansi setup happens. It's not enough to only modify the state within the execution context of the test command (which is what I originally tried).

Agree we can just get rid of this hack by implementing a proper formatting API, and having direct control over how control codes are injected. It's also a good argument to do a proper formatting API sooner rather than later. The number of hacks and shortcuts I've had to introduce to get Jansi working as expected is getting pretty uncomfortable at this point. So I'll probably look towards doing that, rather than continuing further with this approach.

)
}
}
1 change: 0 additions & 1 deletion pkl-commons-test/src/main/kotlin/org/pkl/commons/test/InputOutputTestEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,6 @@ abstract class InputOutputTestEngine :
context: ExecutionContext,
dynamicTestExecutor: DynamicTestExecutor
): ExecutionContext {

val (success, actualOutput) = generateOutputFor(inputFile)
val expectedOutputFile = expectedOutputFileFor(inputFile)

Expand Down
1 change: 1 addition & 0 deletions pkl-core/src/test/kotlin/org/pkl/core/LanguageSnippetTestsEngine.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,7 @@ abstract class AbstractNativeLanguageSnippetTestsEngine : AbstractLanguageSnippe
add(pklExecutablePath.toString())
add("eval")
add("--no-cache")
add("--colors=never")
if (inputFile.startsWith(projectsDir)) {
val projectDir = inputFile.getProjectDir()
if (projectDir != null) {
Expand Down
3 changes: 2 additions & 1 deletion pkl-gradle/src/main/java/org/pkl/gradle/task/BasePklTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,8 @@ protected CliBaseOptions getCliBaseOptions() {
getTestPort().getOrElse(-1),
Collections.emptyList(),
getHttpProxy().getOrNull(),
getHttpNoProxy().getOrElse(List.of()));
getHttpNoProxy().getOrElse(List.of()),
"auto");
Copy link
Contributor

@bioball bioball Jul 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm pretty sure this means that Gradle will not output any colors, because this is executed as a child process.

Anyway, we should provide the same options in Gradle (auto, never, always). Take a look at how the other options are configured to see what to do here. Or, leave a TODO comment here so we can follow up in a future PR.

}
return cachedOptions;
}
Expand Down
3 changes: 2 additions & 1 deletion pkl-gradle/src/main/java/org/pkl/gradle/task/ModulesTask.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -180,7 +180,8 @@ protected CliBaseOptions getCliBaseOptions() {
getTestPort().getOrElse(-1),
Collections.emptyList(),
null,
List.of());
List.of(),
"auto");
}
return cachedOptions;
}
Expand Down