-
Notifications
You must be signed in to change notification settings - Fork 286
feat(sim_if/nvc): add nvc coverage support #1139
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.
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
feat(sim_if/nvc): add nvc coverage support #1139
Conversation
|
Looks good! Please add a newsfragment describing this new feature, see https://vunit.github.io/contributing.html#release-notes-instructions. After that it can be merged. |
|
Looks like 3 actions are failing:
Not sure how I should resolve the first one. For the 2nd and 3rd, I can revert the UART test changes. |
|
I can try to bump NVC to the latest version. At some point it had a problem so I pinned the version we were using in the CI in order for it to pass. I can see if that problem has disappeared with later versions |
|
Our CI uses the https://hub.docker.com/r/hdlc/nvc docker image which hasn't been updated for over a year and uses nvc 1.14-devel (1.13.0.r46.g059d264f) (Using LLVM 11.0.1). Even if I workaround the issue in that version, I'm not sure it supports coverage. I will have to investigate further. Or do you already know? |
I just checked and need to correct myself, the minimum required version for the coverage is actually r1.15.0: https://github.com/nickg/nvc/releases/tag/r1.15.0 (for the |
|
Hi @LarsAsplund, the 1.14.0 does support coverage already. However, you state some commit pre 1.14 (1-14-devel). The thing is that the coverage API changed in 1.14 ( I did not have a look if the commit you referenced already contains this change, or not, however the changes made in this MR use And, as I type it, I see that @CharrierTim just pushed a fix for this. That is Open source race condition... |
|
@CharrierTim Please rebase on master and see if it works. I've updated to NVC 1.18. |
|
Great, it works now. Thanks for your contribution! |
Resolves #984
Added coverage merge support for nvc.
Extended tests in
test/vhdl/coverage/run.pyand added coverage to.test/vhdl/uart/run.pyto show a more realistic use case