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

[svsim] Better error message when verilator not on PATH #3829

Merged
merged 1 commit into from
Feb 16, 2024
Merged

[svsim] Better error message when verilator not on PATH #3829

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
13 changes: 8 additions & 5 deletions svsim/src/main/scala/verilator/Backend.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,8 @@
package svsim.verilator

import svsim._
import java.io.{BufferedReader, InputStreamReader}
import scala.sys.process._
import scala.collection.mutable

object Backend {
object CompilationSettings {
Expand All @@ -22,10 +23,12 @@ object Backend {
enableAllAssertions: Boolean = false)

def initializeFromProcessEnvironment() = {
val process = Runtime.getRuntime().exec(Array("which", "verilator"))
val outputReader = new BufferedReader(new InputStreamReader(process.getInputStream()))
val executablePath = outputReader.lines().findFirst().get()
process.waitFor()
val output = mutable.ArrayBuffer.empty[String]
val exitCode = List("which", "verilator").!(ProcessLogger(output += _))
if (exitCode != 0) {
throw new Exception(s"verilator not found on the PATH!\n${output.mkString("\n")}")
}
val executablePath = output.head.trim
Comment on lines +26 to +31
Copy link
Member

Choose a reason for hiding this comment

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

This definitely works on macOS and linux. Does this work on Windows?

It may be better to instead do verilator --version which may be more platform independent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I doubt which works on Windows except maybe WSL/WSL2. verilator --version won't give the path to it, which is what this code is trying to do (not sure why, it's how it's always been). I think I'll merge this fix as is (although will try to add a test), but point taken about trying to make it platform independent. That being said, I'm not sure how well Verilator works on Windows anyway since it also uses a Makefile that is very much written in a *nix style... Checking all of this on Windows is probably just a big task across the board.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rereading your suggestion (from review) about a test, I don't think there's any reasonable way to test the behavior of "which" here, at least not via ScalaTest. I'm not sure how valuable it is that this code is getting the full path to Verilator, but it does have some nice properties like ensuring the generated Makefile for replaying points to the exact Verilator rather than requiring the user to have the same environment.

new Backend(executablePath = executablePath)
}
}
Expand Down
Loading