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

Fix error with NVC and the --gui flag #930

Merged
merged 2 commits into from Apr 23, 2023
Merged

Fix error with NVC and the --gui flag #930

merged 2 commits into from Apr 23, 2023

Conversation

0dinD
Copy link
Contributor

@0dinD 0dinD commented Apr 23, 2023

Fixes #926

#926 (comment) by @umarcor

For reference, in GHDL the Path is converted to str in

data_file_name = str(Path(script_path) / f"wave.{self._gtkwave_fmt!s}")

Yeah, the GHDL interface does the string conversion earlier:

vunit/vunit/sim_if/ghdl.py

Lines 330 to 340 in fc91e48

script_path = str(Path(output_path) / self.name)
if not Path(script_path).exists():
makedirs(script_path)
ghdl_e = elaborate_only and config.sim_options.get("ghdl.elab_e", False)
if self._gtkwave_fmt is not None:
data_file_name = str(Path(script_path) / f"wave.{self._gtkwave_fmt!s}")
if Path(data_file_name).exists():
remove(data_file_name)

But the path is still useful (for checking if the file exists and so on), so the code ends up converting back and forth between Path and str everywhere. Is it not better to wait with the string conversion until a string is required, like what I did in cc5c607?

I could add a commit to refactor the Path handling in the GHDL (or NVC?) interface, if you want it to be more consistent.

@umarcor
Copy link
Member

umarcor commented Apr 23, 2023

@0dinD, it's ok. I provided the reference to GHDL's implementation for completeness only, to support your point that this change to NVC is correct indeed.

According to your comment/analysis, if you want to make GHDL's implementation cleaner (to reduce the number of changes to/from Path/str), that'd be welcome. Nonetheless, we can have this merged as-is, and deal with that in an upcoming PR.

Thanks for fixing this!

eine pushed a commit to 0dinD/vunit that referenced this pull request Apr 23, 2023
@eine eine added this to the v4.7.0 milestone Apr 23, 2023
@eine eine merged commit e63cf90 into VUnit:master Apr 23, 2023
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error with NVC and the --gui flag
3 participants