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.

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

added plusargs support for ghdl #3214

Merged
merged 1 commit into from Feb 4, 2023
Merged

Conversation

gigo333
Copy link
Contributor

@gigo333 gigo333 commented Jan 30, 2023

When using ghdl -r some arguments (like vhdl version) must be passed after the top defnition while others must be passed after the vpi arguments (like --wave).
Now when using the plusargs optional arguments, this argument will be passed after the vpi args , while the test_args arguments will be passed after the top module declaration.

@imphil
Copy link
Member

imphil commented Jan 30, 2023

@themperek Can you have a look to see if we need additional tests for plusarg support?

@themperek
Copy link
Contributor

@themperek Can you have a look to see if we need additional tests for plusarg support?

Yes. On the way actually.

@codecov
Copy link

codecov bot commented Jan 30, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@8fef13a). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head 468609b differs from pull request most recent head 58a370c. Consider uploading reports for the commit 58a370c to get more accurate results

@@            Coverage Diff            @@
##             master    #3214   +/-   ##
=========================================
  Coverage          ?   68.04%           
=========================================
  Files             ?       48           
  Lines             ?     8824           
  Branches          ?     2428           
=========================================
  Hits              ?     6004           
  Misses            ?     2458           
  Partials          ?      362           
Impacted Files Coverage Δ
cocotb/runner.py 28.96% <ø> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@gigo333
Copy link
Contributor Author

gigo333 commented Feb 1, 2023

I'm sorry, @themperek, i don't have much time now to write the tests.

@ktbarrett
Copy link
Member

I think this is fine without a test, plusargs support was added to all the other simulators without a test and it existed in the Makefiles previously. @gigo333 There have been changes to the runner file since you've pushed this PR and there are conflicts. Do you have time to resolve the conflicts at least?

@gigo333
Copy link
Contributor Author

gigo333 commented Feb 2, 2023

@ktbarrett I have resolved the conflicts.

@ktbarrett
Copy link
Member

It appears to still be in conflict. I'm not sure why... My guess is that you need to rebase your branch onto master. cocotb uses rebasing for PRs and I'm guessing while you merged to a clean working point, it won't rebase cleanly, for whatever reason. Are you familiar and comfortable with rebasing? If not, here is some documentation.

@ktbarrett
Copy link
Member

@themperek You okay with this?

@themperek themperek merged commit 918fc41 into cocotb:master Feb 4, 2023
@themperek
Copy link
Contributor

@gigo333 Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants