search

leonardt / fault

A Python package for testing hardware (part of the magma ecosystem)
BSD 3-Clause "New" or "Revised" License
41 stars 13 forks source link

Print Errors #159

Closed David-Durst closed 5 years ago

David-Durst commented 5 years ago

It appears that https://github.com/leonardt/fault/pull/150 disabled printing errors. When verilator fails due to an assertion, that pull request ends the program before emitting stderr. This adds back printing stderr.

Did I add back the error printing in the correct place?

sgherbst commented 5 years ago

Thanks for the feedback and sorry for the inconvenience. I think we should move the assertion on line 50 to somewhere after line 139. That way you'll get to see all of STDOUT and STDERR. I'll update this branch with that change shortly...

sgherbst commented 5 years ago

Hey David, I'm really sorry to nitpick... but this will potentially print out STDOUT and STDERR twice. By default subprocess_run prints out via logging.info, which requires something like this to see:

logging.basicConfig(level=logging.INFO)

Since that would not be obvious to users though, perhaps the default should be to print directly to STDOUT instead? In that case we just need to change the default disp_type to print on line 62 of fault/subprocess_run.py.

David-Durst commented 5 years ago

I don't understand how my change will cause double printing. The below change will only print if the program is immediately exiting. If the assert doesn't fail, then no printing will occur. https://github.com/leonardt/fault/pull/159/commits/329e26650f0e06fdbe91d0d6afd51a7fab013d4f

However, I now see that the print change is an alternative that accomplishes my goal (getting stderr and stdout) while printing it in a more readable format.

I've replaced the change https://github.com/leonardt/fault/pull/159/commits/329e26650f0e06fdbe91d0d6afd51a7fab013d4f with the print change.

I'm still not happy with the output however. I believe that stdout and stderr should only be printed if something goes wrong. Otherwise, the log will be too cluttered. https://github.com/leonardt/fault/pull/159/commits/dab6b3a151e53458e9f929834c9fa4ac3739a510 accomplishes this. Thoughts?

sgherbst commented 5 years ago

Great point, I think there are at least two different use cases we should support:

  1. Running a one-off test in which case it is useful to see output in realtime, independent of whether there is an error (this is why process_output prints lines as they come in)
  2. Running a large batch of tests, in which case it is only desirable to see output when there is an error (note that an error could be indicated either by a return code or by an error string depending on the tool in question)

As a first pass, I made an update (ec21205) so that "disp_type" takes on a slightly different meaning -- options are "realtime" (corresponding to case 1 above) and "on_error" (corresponding to case 2). The default is on_error, but I threaded this option through to SystemVerilogTarget, VerilatorTarget, and SpiceTarget so that the user can change this on a per-run basis. This should keep the output log clean by default while allowing the user to see more if desired. Does this resolve your concerns about subprocess printing?

David-Durst commented 5 years ago

This looks good. A remaining issue is how to make this accessible through the standard interface: https://github.com/leonardt/fault/blob/b231d8bb3c8d98b4368df22658eb74f7b4323c53/fault/tester.py#L274

Your printer is only for verilator, right?

sgherbst commented 5 years ago

The printer is for system-verilog, verilog-ams, verilator, and spice targets. I added an option to the constructors for each kind of Target called disp_type, which in turn is passed to subprocess_run, so compile_and_run should work with a disp_type argument.

sgherbst commented 5 years ago

@David-Durst if you're OK with this approach then I'll merge and close out this PR. Thanks for the great feedback!

David-Durst commented 5 years ago

lgtm

https://github.com/leonardt/fault/blob/499664ab626e87678c9a1474fa9f535a4141ffee/fault/tester.py#L265-L272

Your comment makes sense. I thought that the disp_type parameter would need to be passed to the self.run call in self._compile_and_run. That wouldn't work as _compile_and_run doesn't pass **kwargs to self._compile. I now see that disp_type can be passed to self.run, which does receive **kwargs from self._compile.