-
Notifications
You must be signed in to change notification settings - Fork 258
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
Add NVC support #904
Add NVC support #904
Conversation
@nickg I tried to run all acceptance tests locally and ran into a few failing tests. I built nvc from a Github clone and is running under Windows:
I looks like the problem is related to file accesses. For example,
results in
The error status is Any idea? |
I haven't tested VUnit on Windows before, I'll have a look. |
Shall I add nvc tests to CI in a feature-branch so that @nickg can cherry-pick it? |
The problem seems to be that |
@nickg Thanks, I will get back and give it a try this evening. |
@nickg I tested your updates and now all tests are green! I noticed one thing when testing with VHDL/Compliance-Tests though. When I enable VHDL-2019, VUnit will compile a logging package making use of call paths. That didn't seem to work so I changed master to check for simulator support with a new simulator interface method: That being said, after I made the change I observed some commit messages in nvc suggesting that call paths are supported. Is that so? Something that works in Linux but not on Windows? If it is a simple fix we can take care of it now but otherwise I'm okey with merging this PR as it stands. Call path support can be added later if that is more convenient. |
@LarsAsplund it should work, including on Windows, but it was only added recently so it's not been thoroughly tested. Here's the simple regression test I'm using: https://github.com/nickg/nvc/blob/master/test/regress/stdenv3.vhd What do I need to change in order test in VHDL-2019 mode? |
Do you mean this?
I believe this error is correct: there is an ambiguity between those two overloads (at least GHDL also errors on the same thing). Do other simulators accept it? I think it can be fixed easily by using a qualified expression like: diff --git a/vunit/vhdl/logging/src/location_pkg-body-2019p.vhd b/vunit/vhdl/logging/src/location_pkg-body-2019p.vhd
index 0202ce5ba333..381e9c033893 100644
--- a/vunit/vhdl/logging/src/location_pkg-body-2019p.vhd
+++ b/vunit/vhdl/logging/src/location_pkg-body-2019p.vhd
@@ -26,7 +26,7 @@ package body location_pkg is
end if;
end if;
- write(result.file_name, "");
+ write(result.file_name, string'(""));
return result;
end;
end package body; |
@nickg I didn't check any deeper but you're right, that's on me. I will fix it. It works with Aldec's simulators which are the only ones we have supporting VHDL-2019. GHDL would probably fail on it as well but without 2019 support it is never exposed. I will make an update in an hour or so. I think the new |
Sure, I just added it in 611661c |
@nickg Good, works as expected. I've pushed the ambiguity fix to master so I need yet another rebase. After that I think we're ready to merge this PR. |
Well, no point in you making the rebase... Done. Thanks for your contribution. |
I rebased the CI branch after the merge of this PR. Acceptance tests are all passing, but vcomponents are failing on
|
@umarcor I can reproduce it locally, I'll have a look... |
|
Updated now. Hopefully that removes the problem. |
Rebased CI branch. Running: https://github.com/VUnit/vunit/actions/runs/4287438129 |
OK, I actually fixed the bug just now anyway in nickg/nvc@1c81bcc but I ran into a second error:
I think I know what the problem is but it will take a while longer to fix. Perhaps you could mark that test as expected fail with NVC temporarily? |
Yeah, I see the same thing. Let's see what you can find. |
All tests other than the AXI Stream VC's are green. So, that's good news! This might be last feature/bug to catch. |
This is fixed on the latest master branch and all the verification_components tests pass for me now. Could you test again? |
@nickg all green! https://github.com/VUnit/vunit/actions/runs/4337431930 Please, let us know which is the first tag including these enhancements, so that we can use it as the min version to recommend to users. |
I'll make a 1.9 release in a month or so but there's a few other things I want to get in first. Thanks for testing! |
This adds support for NVC, a VHDL simulator I have been developing, based on some initial work from @kraigher a few years ago. It can pass all the acceptance tests but needs the very latest master branch.