-
Notifications
You must be signed in to change notification settings - Fork 263
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
|
||
extensions = [ | ||
"sphinx.ext.autodoc", | ||
"sphinx.ext.autosummary", | ||
"sphinx.ext.extlinks", | ||
"sphinx.ext.intersphinx", | ||
"sphinx.ext.todo", | ||
|
@@ -113,7 +114,7 @@ | |
# -- InterSphinx -------------------------------------------------------------- | ||
|
||
intersphinx_mapping = { | ||
"python": ("https://docs.python.org/3.8/", None), | ||
"python": ("https://docs.python.org/3/", None), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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), | ||
} | ||
|
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: |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I am aware, the purpose of There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
|
||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rather than using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. | ||
|
@@ -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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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())} ...") | ||
|
||
|
@@ -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: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would use a list instead of a tuple. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There lines fix #796 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}") | ||
|
@@ -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") | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe 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()}"' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This gets rid of redundant There was a problem hiding this comment. Choose a reason for hiding this commentThe 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]) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably not to be done in this PR, but |
There was a problem hiding this comment.
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?There was a problem hiding this comment.
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.