Fix some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

oscargus · 2022-11-07T11:37:55Z

Closes #686
Closes #796
Closes #811

Not sure where it can be tested. Didn't really find a test file for vivado.py?

The vivado module is not in the documentation? So not really clear where I should document the environment variable. (But I can improve the documentation for run_vivado.

LarsAsplund · 2022-11-07T12:35:04Z

Yes, please do something about the documentation. vivado.py started out more as an example as we generally do not maintain tool integrations other than what's related to simulators and we generally do not document the examples that much. Since this is becoming more than just an example I suggest that it also gets some documentation.

oscargus · 2022-11-07T14:59:53Z

Now there is some sort of documentation for the functions in the module. Located at the bottom of the "Documentation" section (below "Examples").

oscargus · 2022-12-02T14:55:47Z

Sorry to bother you @LarsAsplund , but can you please see if anything else is required (or if you prefer the Vivado menu entry elsewhere).

docs/index.rst

vunit/vivado/vivado.py

oscargus · 2023-08-31T08:34:29Z

Sorry for letting this slip. Will try to find some time "soon" to complete it.

oscargus · 2023-09-09T12:19:47Z

Finally got around to this. Not sure this is exactly what you wanted, but at least a "Tool Integration" menu item and then Vivado shows up there. Not clear to me how the "dynamic menus" work and if this will be an entry there (or if that it a better solution).

SzymonHitachi · 2023-10-11T17:12:06Z

Hello, bump on this one @oscargus @LarsAsplund . Any chance this could get pulled into mainline? This is very useful for using with Verification IPs

…vironment variable)

umarcor

This PR seems to address multiple issues in a single commit. @oscargus can you please split it into three commits so it's easier to review? It's ok to keep them in a single PR tho.

umarcor · 2024-03-12T14:16:20Z

docs/conf.py

@@ -113,7 +114,7 @@
 # -- InterSphinx --------------------------------------------------------------

 intersphinx_mapping = {
-    "python": ("https://docs.python.org/3.8/", None),
+    "python": ("https://docs.python.org/3/", None),


This belongs to a separated PR or, at least, to a separated commit within this PR.

umarcor · 2024-03-12T14:17:13Z

docs/conf.py

@@ -30,6 +30,7 @@

 extensions = [
    "sphinx.ext.autodoc",
+    "sphinx.ext.autosummary",


I don't see where is the .. autosummary:: used in the docs. Is this extension used anywhere?

You are correct. I probably thought it was required for automodule at the time.

umarcor · 2024-03-12T14:19:58Z

vunit/vivado/__init__.py

@@ -13,3 +13,5 @@
    add_from_compile_order_file,
    create_compile_order_file,
 )
+
+__all__ = ["run_vivado", "add_from_compile_order_file", "create_compile_order_file"]


As far as I am aware, the purpose of __all__ seems to be to reduce the number of imported elements when from <module> import * is used. However, doing so is generally considered a not good programming practice. In fact, we recently merged a PR to reduce the "catch them all" imports in VHDL (#992). Therefore, I'm not sure we want to support/enforce those kind of imports in Python.

If I remember correctly: this will also make the documentation clearer, but I'll play around with it and see if it is actually the case.

umarcor · 2024-03-12T14:40:54Z

vunit/vivado/vivado.py

    for library_name, file_name in compile_order:
-        is_verilog = file_name.endswith(".v") or file_name.endswith(".vp")
+        is_verilog = file_name.endswith(verilog_file_endings)


Rather than using .endswith, I would suggest using Pathlib's .suffix (https://docs.python.org/3/library/pathlib.html#pathlib.PurePath.suffix) because semantically we are cheking the extension of a filename not the end of an arbitrary string. At the same time, since the tuple/list is used once only, the declaraction of the variable feels redundant. I would suggest Path(file_name).suffix in ['.v', '.vp', '.sv'].

Part of fixing #796

umarcor · 2024-03-12T14:42:07Z

vunit/vivado/vivado.py

    with Path(file_name).open("r", encoding="utf-8") as ifile:
        for line in ifile.readlines():
            library_name, file_type, file_name = line.strip().split(",", 2)

-            if file_type not in ("Verilog", "VHDL", "Verilog Header"):
+            if file_type not in valid_file_types:


I would use a list instead of a tuple.

Is there a concrete reason for that? I tend to go for tuples as there is a (minor) performance benefit of those.

There lines fix #796

Conceptually, I understand the tuples as an array of a fixed size, while a list is kind of expected to grow/reduce. Hence, I tend to use lists in other places of the codebase.

However, if there is a performance benefit, it is ok to use tuples here, and I will take it into account when touching other parts of the codebase. It's always nice to learn something!

umarcor · 2024-03-12T14:47:48Z

vunit/vivado/vivado.py


    print(cmd)
+    # shell=True is important in Windows where Vivado is just a bat file.
    check_call(cmd, cwd=cwd, shell=True)


Probably not to be done in this PR, but check_call's args parameter can be either a list of a string: https://docs.python.org/3/library/subprocess.html#subprocess.CompletedProcess.args.
I feel it would be cleaner if we managed cmd as a list, so we could append tcl_args without explicitly using " ".join. See https://docs.python.org/3/library/subprocess.html#converting-an-argument-sequence-to-a-string-on-windows.

oscargus · 2024-03-12T16:03:30Z

I'll see what I can do in terms of splitting it. It's been a while.

oscargus · 2024-03-12T16:05:20Z

vunit/vivado/vivado.py

+        str(Path(vivado_path).resolve() / "bin" / "vivado")
+        if vivado_path is not None
+        else environ.get("VUNIT_VIVADO_PATH", "vivado")
+    )


I think it is easier to comment line by line.

This line introduces the environment variable (documented above).

oscargus · 2024-03-12T16:06:25Z

vunit/vivado/vivado.py

+        if vivado_path is not None
+        else environ.get("VUNIT_VIVADO_PATH", "vivado")
+    )
+    cmd = f'"{vivado}" -nojournal -nolog -notrace -mode batch -source "{Path(tcl_file_name).resolve()}"'


This gets rid of redundant str in f-string and quotes the location of vivado (in case of spaces).

This and below fixes #686

oscargus · 2024-03-12T16:06:49Z

vunit/vivado/vivado.py

    if tcl_args is not None:
-        cmd += " -tclargs " + " ".join([str(val) for val in tcl_args])
+        cmd += " -tclargs " + " ".join([f'"{val}"' for val in tcl_args])


More quoting of stuff in case of spaces.

oscargus · 2024-03-12T16:09:01Z

vunit/vivado/vivado.py

+    :param compile_order_file: Filename for to write compile-order file.
+    :param vivado_path: Path to Vivado install directory. If ``None``, the
+        environment variable ``VUNIT_VIVADO_PATH`` is used if set. Otherwise,
+        rely on that ``vivado`` is in the path.


Better documentation related to environment variable.

oscargus · 2024-03-12T16:09:31Z

vunit/vivado/vivado.py

+    :param compile_order_file: Compile-order file (from :func:`create_compile_order_file`)
+    :param dependency_scan_defaultlib: Whether to do VUnit scanning of ``xil_defaultlib``.
+    :param fail_on_non_hdl_files: Whether to fail on non-HDL files.
+


Documentation of parameters/arguments.

oscargus force-pushed the vivadofixes branch 2 times, most recently from f87ce3e to 56ee9ad Compare November 7, 2022 11:57

oscargus force-pushed the vivadofixes branch 3 times, most recently from fbd9cbf to 746b483 Compare November 7, 2022 14:53

oscargus force-pushed the vivadofixes branch 2 times, most recently from 4f457d8 to 30d9da6 Compare November 7, 2022 18:41

LarsAsplund reviewed Dec 8, 2022

View reviewed changes

docs/index.rst Outdated Show resolved Hide resolved

vunit/vivado/vivado.py Show resolved Hide resolved

eine added Enhancement Simulator support Docs labels Dec 8, 2022

eine added this to the v4.7.0 milestone Dec 8, 2022

eine modified the milestones: v4.7.0, v4.8.0, v5.0.0 Apr 19, 2023

oscargus force-pushed the vivadofixes branch from 30d9da6 to afdbad7 Compare September 9, 2023 12:13

Fix some Vivado issues (SystemVerilog support, space in file name, en…

319370d

…vironment variable)

oscargus force-pushed the vivadofixes branch from afdbad7 to 319370d Compare March 12, 2024 10:44

umarcor reviewed Mar 12, 2024

View reviewed changes

oscargus commented Mar 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

Fix some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

oscargus commented Nov 7, 2022 •

edited

LarsAsplund commented Nov 7, 2022

oscargus commented Nov 7, 2022

oscargus commented Dec 2, 2022

oscargus commented Aug 31, 2023

oscargus commented Sep 9, 2023

SzymonHitachi commented Oct 11, 2023

umarcor left a comment

umarcor Mar 12, 2024

umarcor Mar 12, 2024

oscargus Mar 12, 2024

umarcor Mar 12, 2024

oscargus Mar 12, 2024

umarcor Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

umarcor Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

umarcor Mar 13, 2024

umarcor Mar 12, 2024

oscargus commented Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

oscargus Mar 12, 2024

Fix some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

Are you sure you want to change the base?

Fix some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

Conversation

oscargus commented Nov 7, 2022 • edited

LarsAsplund commented Nov 7, 2022

oscargus commented Nov 7, 2022

oscargus commented Dec 2, 2022

oscargus commented Aug 31, 2023

oscargus commented Sep 9, 2023

SzymonHitachi commented Oct 11, 2023

umarcor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Mar 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oscargus commented Nov 7, 2022 •

edited