[svsim] Better error message when verilator not on PATH #3829
Conversation
jackkoenig
force-pushed
the
svsim-better-error-no-verilator
branch
from
213afb9
to
827bc93
Compare
| 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
(cherry picked from commit 41dd267)
(cherry picked from commit 41dd267)
Contributor Checklist
docs/src?Type of Improvement
Desired Merge Strategy
Release Notes
Reviewer Checklist (only modified by reviewer)
3.6.x,5.x, or6.xdepending on impact, API modification or big change:7.0)?Enable auto-merge (squash), clean up the commit message, and label withPlease Merge.Create a merge commit.