Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix color emission to check for interactive terminal #3334

Merged
merged 1 commit into from
Jun 7, 2023
Merged

Conversation

jackkoenig
Copy link
Contributor

@jackkoenig jackkoenig commented Jun 7, 2023

This is really hard to test. See Release Notes below for user-facing API and limitations.

I would like to test this but it can't be done in ScalaTest nor really even with SBT, you need to publishLocal and then use scala-cli and then some way of processing the output for correctness.

Contributor Checklist

  • Did you add Scaladoc to every public function/method?
  • Did you add at least one test demonstrating the PR?
  • Did you delete any extraneous printlns/debugging code?
  • Did you specify the type of improvement?
  • Did you add appropriate documentation in docs/src?
  • Did you request a desired merge strategy?
  • Did you add text to be included in the Release Notes for this change?

Type of Improvement

  • Feature (or new API)

Desired Merge Strategy

  • Squash

Release Notes

  • Chisel will now detect when it should print warnings, errors, and deprecations in color.
    • Color can be controlled with environment variable CHISEL_USE_COLOR. Set to true to force Chisel to use color and false to disable it.
    • Due to how the JVM works, detection requires interactive stdout, stderr, and stdin. Build tools like SBT virtualize stdin and thus color will be disabled by default when running a Chisel main with SBT. Detection also requires environment variable TERM to be set to something other than dumb.

Reviewer Checklist (only modified by reviewer)

  • Did you add the appropriate labels? (Select the most appropriate one based on the "Type of Improvement")
  • Did you mark the proper milestone (Bug fix: 3.5.x or 3.6.x depending on impact, API modification or big change: 5.0.0)?
  • Did you review?
  • Did you check whether all relevant Contributor checkboxes have been checked?
  • Did you do one of the following when ready to merge:
    • Squash: You/ the contributor Enable auto-merge (squash), clean up the commit message, and label with Please Merge.
    • Merge: Ensure that contributor has cleaned up their commit history, then merge with Create a merge commit.

@jackkoenig jackkoenig added the Feature New feature, will be included in release notes label Jun 7, 2023
@jackkoenig jackkoenig added this to the 3.6.x milestone Jun 7, 2023
@jackkoenig
Copy link
Contributor Author

jackkoenig commented Jun 7, 2023

Here's how I tested this (as a record for posterity).

First show that it doesn't work on current main, with file main.scala:

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.10"
//> using lib "org.chipsalliance::chisel::6.0.0-M1+27-6e8eee88-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin::6.0.0-M1+27-6e8eee88-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import circt.stage.ChiselStage

class Foo extends Module {
  val in = IO(Input(Vec(4, UInt(8.W))))
  val idx = IO(Input(UInt(3.W)))
  val out = IO(Output(UInt(8.W)))

  out := in(idx)
}

object Main extends App {
  println(ChiselStage.emitCHIRRTL(new Foo))
}
# Should see color
scala-cli main.scala

# Will see the color codes in the text file, this is bad
scala-cli main.scala > main.txt

Now publish local (sbt "unipublish / publishLocal) this branch and try it out (fixed.scala):

//> using repository "sonatype-s01:snapshots"
//> using scala "2.13.10"
//> using lib "org.chipsalliance::chisel::6.0.0-M1+27-a832e2b0-SNAPSHOT"
//> using plugin "org.chipsalliance:::chisel-plugin::6.0.0-M1+27-a832e2b0-SNAPSHOT"
//> using options "-unchecked", "-deprecation", "-language:reflectiveCalls", "-feature", "-Xcheckinit", "-Xfatal-warnings", "-Ywarn-dead-code", "-Ywarn-unused", "-Ymacro-annotations"

import chisel3._
import circt.stage.ChiselStage

class Foo extends Module {
  val in = IO(Input(Vec(4, UInt(8.W))))
  val idx = IO(Input(UInt(3.W)))
  val out = IO(Output(UInt(8.W)))

  out := in(idx)
}

object Main extends App {
  println(ChiselStage.emitCHIRRTL(new Foo))
}
# Should see color
scala-cli fixed.scala

# Will NOT see color in the text file
scala-cli fixed.scala > fixed.txt

# Will NOT see color with no stdin (see release notes)
scala-cli fixed.scala </dev/null

# Can also test the environment variables
CHISEL_USE_COLOR=false scala-cli fixed.scala

Copy link
Contributor

@chick chick left a comment

Choose a reason for hiding this comment

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

That's cool. Looks good to me.

@jackkoenig jackkoenig merged commit 308eb6a into main Jun 7, 2023
@jackkoenig jackkoenig deleted the fix-color branch June 7, 2023 21:08
@mergify mergify bot added the Backported This PR has been backported label Jun 7, 2023
mergify bot pushed a commit that referenced this pull request Jun 7, 2023
mergify bot pushed a commit that referenced this pull request Jun 7, 2023
mergify bot added a commit that referenced this pull request Jun 7, 2023
(cherry picked from commit 308eb6a)

Co-authored-by: Jack Koenig <koenig@sifive.com>
mergify bot added a commit that referenced this pull request Jun 7, 2023
(cherry picked from commit 308eb6a)

Co-authored-by: Jack Koenig <koenig@sifive.com>
@sequencer sequencer mentioned this pull request Jun 20, 2023
14 tasks
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Backported This PR has been backported Feature New feature, will be included in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants