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 some Vivado issues (SystemVerilog support, space in file name, environment variable) #871

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion docs/conf.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

extensions = [
"sphinx.ext.autodoc",
"sphinx.ext.autosummary",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"sphinx.ext.extlinks",
"sphinx.ext.intersphinx",
"sphinx.ext.todo",
Expand Down Expand Up @@ -113,7 +114,7 @@
# -- InterSphinx --------------------------------------------------------------

intersphinx_mapping = {
"python": ("https://docs.python.org/3.8/", None),
"python": ("https://docs.python.org/3/", None),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"pytest": ("https://docs.pytest.org/en/latest/", None),
"osvb": ("https://umarcor.github.io/osvb", None),
}
Expand Down
1 change: 1 addition & 0 deletions docs/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ often"* approach through automation. :ref:`Read more <about>`
py/ui
hdl_libraries
examples
tool_integration/index

.. toctree::
:caption: Continuous Integration
Expand Down
11 changes: 11 additions & 0 deletions docs/tool_integration/index.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
.. _tool_integration:

Tool Integration
================
There are additional integration support available for selected tools.

Vivado Integration
==================

.. automodule:: vunit.vivado
:members:
2 changes: 2 additions & 0 deletions vunit/vivado/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

44 changes: 34 additions & 10 deletions vunit/vivado/vivado.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,21 @@
"""

from subprocess import check_call
from os import makedirs
from os import makedirs, environ
from pathlib import Path


def add_from_compile_order_file(
vunit_obj, compile_order_file, dependency_scan_defaultlib=True, fail_on_non_hdl_files=True
): # pylint: disable=too-many-locals
"""
Add Vivado IP:s from a compile order file
Add Vivado IP:s from a compile order file.

:param project_file: :class:`~vunit.ui.VUnit`
: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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation of parameters/arguments.

"""
compile_order, libraries, include_dirs = _read_compile_order(compile_order_file, fail_on_non_hdl_files)

Expand All @@ -30,8 +36,9 @@ def add_from_compile_order_file(

no_dependency_scan = []
with_dependency_scan = []
verilog_file_endings = (".v", ".vp", ".sv")
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)
oscargus marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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'].

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Part of fixing #796


# Optionally use VUnit dependency scanning for everything in xil_defaultlib, which
# typically contains unencrypted top levels that instantiate encrypted implementations.
Expand Down Expand Up @@ -63,7 +70,13 @@ def add_from_compile_order_file(

def create_compile_order_file(project_file, compile_order_file, vivado_path=None):
"""
Create compile file from Vivado project
Create compile file from Vivado project.

:param project_file: Project filename.
: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.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better documentation related to environment variable.

"""
print(f"Generating Vivado project compile order into {str(Path(compile_order_file).resolve())} ...")

Expand All @@ -81,18 +94,19 @@ def create_compile_order_file(project_file, compile_order_file, vivado_path=None

def _read_compile_order(file_name, fail_on_non_hdl_files):
"""
Read the compile order file and filter out duplicate files
Read the compile order file and filter out duplicate files.
"""
compile_order = []
unique = set()
include_dirs = set()
libraries = set()

valid_file_types = ("Verilog", "VHDL", "Verilog Header", "SystemVerilog")
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:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use a list instead of a tuple.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There lines fix #796

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

if fail_on_non_hdl_files:
raise RuntimeError(f"Unsupported compile order file: {file_name}")
print(f"Compile order file ignored: {file_name}")
Expand All @@ -119,12 +133,22 @@ def run_vivado(tcl_file_name, tcl_args=None, cwd=None, vivado_path=None):
"""
Run tcl script in Vivado in batch mode.

Note: the shell=True is important in windows where Vivado is just a bat file.
:param tcl_file_name: Path to tcl file
:param tcl_args: tcl arguments passed to Vivado via the ``-tclargs`` switch
:param cwd: Passed as ``cwd`` to :func:`subprocess.check_call`
: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.
"""
vivado = "vivado" if vivado_path is None else str(Path(vivado_path).resolve() / "bin" / "vivado")
cmd = f"{vivado} -nojournal -nolog -notrace -mode batch -source {str(Path(tcl_file_name).resolve())}"
vivado = (
str(Path(vivado_path).resolve() / "bin" / "vivado")
if vivado_path is not None
else environ.get("VUNIT_VIVADO_PATH", "vivado")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is easier to comment line by line.

This line introduces the environment variable (documented above).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixes #811

cmd = f'"{vivado}" -nojournal -nolog -notrace -mode batch -source "{Path(tcl_file_name).resolve()}"'
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and below fixes #686

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])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More quoting of stuff in case of spaces.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.