From 1a6bf1e23cb6b988afd4ac2fb3edbb892c7c1062 Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Fri, 7 Jul 2023 17:29:48 +0200 Subject: [PATCH 1/6] Use short_path when collecting paths to source files Full path consist of an optional first part called `root`, and the second part which is the `short_path`. Usually for non-generated files `path` is equal to `short_path`, but this is not the case for the generated files. Documentation states that the `short_path` should be used if the file is used in the runfiles to handle both generated and standard files correctly. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/cocotb.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cocotb/cocotb.bzl b/cocotb/cocotb.bzl index d8da2939..3479664d 100644 --- a/cocotb/cocotb.bzl +++ b/cocotb/cocotb.bzl @@ -32,7 +32,7 @@ def _dict_to_argstring(data, argname): return result def _files_to_argstring(data, argname): - return _list_to_argstring(data, argname, "path") + return _list_to_argstring(data, argname, "short_path") def _pymodules_to_argstring(data, argname): remove_py = lambda s: s.removesuffix(".py") From df2fbd16ca98fa5766c7e285c9b2fd8eda0f93b6 Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Sat, 8 Jul 2023 00:42:23 +0200 Subject: [PATCH 2/6] Remove cocotb_wrapper.py dependency on cocotb This change allows the user to specify his installation of cocotb and prevents circular includes when the rule is imported in other repositories. However, with this change user has to explicitely add cocotb to the list of dependencies in the cocotb_test rule. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/BUILD | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/cocotb/BUILD b/cocotb/BUILD index d87c4182..18378bae 100644 --- a/cocotb/BUILD +++ b/cocotb/BUILD @@ -12,13 +12,11 @@ # See the License for the specific language governing permissions and # limitations under the License. -load("@rules_hdl_pip_deps//:requirements.bzl", "requirement") - py_binary( name = "cocotb_wrapper", srcs = ["cocotb_wrapper.py"], python_version = "PY3", srcs_version = "PY3", visibility = ["//visibility:public"], - deps = [requirement("cocotb")], + deps = [], ) From d600f1a45a722666476dea17bade15cfc0b8af86 Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Mon, 10 Jul 2023 15:57:25 +0200 Subject: [PATCH 3/6] Make the cocotb_wrapper attribute of the cocotb_test rule public Make cocotb_wrapper attribute public to allow for modyfying the script for other cocotb versions or different build setups. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/cocotb.bzl | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/cocotb/cocotb.bzl b/cocotb/cocotb.bzl index 3479664d..49d86cbe 100644 --- a/cocotb/cocotb.bzl +++ b/cocotb/cocotb.bzl @@ -61,7 +61,7 @@ def _collect_verilog_files(ctx): ] verilog_files = depset( [src for sub_tuple in verilog_srcs for src in sub_tuple] + - ctx.files.verilog_sources + ctx.files.verilog_sources, ) return verilog_files.to_list() @@ -100,7 +100,7 @@ def _cocotb_test_impl(ctx): command = ( "PATH={}:$PATH ".format(path) + - "python {}".format(ctx.executable._cocotb_wrapper.short_path) + + "python {}".format(ctx.executable.cocotb_wrapper.short_path) + " --sim {}".format(ctx.attr.sim_name) + " --hdl_library {}".format(ctx.attr.hdl_library) + " --hdl_toplevel {}".format(ctx.attr.hdl_toplevel) + @@ -128,12 +128,12 @@ def _cocotb_test_impl(ctx): transitive_files = depset( direct = [py_toolchain.interpreter], transitive = [dep[PyInfo].transitive_sources for dep in ctx.attr.deps] + - [ctx.attr._cocotb_wrapper[PyInfo].transitive_sources] + + [ctx.attr.cocotb_wrapper[PyInfo].transitive_sources] + [py_toolchain.files], ) runfiles = ctx.runfiles( - files = ctx.files._cocotb_wrapper + + files = ctx.files.cocotb_wrapper + verilog_files + vhdl_files + ctx.files.test_module, @@ -249,7 +249,7 @@ _cocotb_test_attrs = { doc = "The list of python libraries to be linked in to the simulation target", providers = [PyInfo], ), - "_cocotb_wrapper": attr.label( + "cocotb_wrapper": attr.label( cfg = "exec", executable = True, doc = "Cocotb wrapper script", From 00bfc83105bf1bed264664c28f130c37bdbb4eb0 Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Tue, 11 Jul 2023 11:48:06 +0200 Subject: [PATCH 4/6] Correctly handle waves attribute in cocotb_test rule Previously, the waves attribute was the only one that was not passed to the cocotb script that ran the simulation. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/cocotb.bzl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/cocotb/cocotb.bzl b/cocotb/cocotb.bzl index 49d86cbe..cdd8fad5 100644 --- a/cocotb/cocotb.bzl +++ b/cocotb/cocotb.bzl @@ -88,6 +88,7 @@ def _cocotb_test_impl(ctx): parameters_args = _dict_to_argstring(ctx.attr.parameters, "parameters") verbose_args = " --verbose" if ctx.attr.verbose else "" + waves_args = " --waves" if ctx.attr.waves else "" seed_args = " --seed {}".format(ctx.attr.seed) if ctx.attr.seed != "" else "" test_module_args = _pymodules_to_argstring(ctx.files.test_module, "test_module") @@ -117,6 +118,7 @@ def _cocotb_test_impl(ctx): defines_args + parameters_args + verbose_args + + waves_args + seed_args + test_module_args ) From 297848f68daf968237cc22a65fedd6f1e5ef3cbe Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Fri, 14 Jul 2023 13:49:36 +0200 Subject: [PATCH 5/6] Correctly propagate failures in `cocotb_test` rule The previous rule implementation did not check whether the test passed or failed in the cocotb runner. The additional check introduced in this commit allows the script to inform Bazel about the actual test results. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/cocotb_wrapper.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/cocotb/cocotb_wrapper.py b/cocotb/cocotb_wrapper.py index 91643abb..1b8db1d0 100644 --- a/cocotb/cocotb_wrapper.py +++ b/cocotb/cocotb_wrapper.py @@ -13,7 +13,7 @@ # limitations under the License. import argparse -from cocotb.runner import get_runner +from cocotb.runner import get_runner, check_results_file cocotb_build_flags = [ @@ -167,6 +167,6 @@ def __call__(self, parser, namespace, values, option_string=None): test_flags = filter_args(vars(args), cocotb_test_flags) runner = get_runner(args.sim) - runner.build(**build_flags) - runner.test(**test_flags), + results_xml = runner.test(**test_flags) + check_results_file(results_xml) From 83b51f4b10e0e8970f88b087b5fab3f90eb89834 Mon Sep 17 00:00:00 2001 From: Robert Winkler Date: Fri, 14 Jul 2023 13:54:22 +0200 Subject: [PATCH 6/6] Improve python dependency handling in cocotb_test rule The changes made in this commit include improvements that allow the `py_library` targets to be properly passed to the `cocotb_test` rule via the `deps` attribute. Now all transitive imports are properly added to `PYTHONPATH`. The `test_module` attribute, on the other hand, is intended to contain only python source files. Internal-tag: [#46586] Signed-off-by: Robert Winkler --- cocotb/cocotb.bzl | 99 +++++++++++++++++++++++++++++++---------------- 1 file changed, 66 insertions(+), 33 deletions(-) diff --git a/cocotb/cocotb.bzl b/cocotb/cocotb.bzl index cdd8fad5..6d8e18b8 100644 --- a/cocotb/cocotb.bzl +++ b/cocotb/cocotb.bzl @@ -17,6 +17,8 @@ load("//verilog:providers.bzl", "VerilogInfo") load("@rules_python//python:defs.bzl", "PyInfo") +## Helpers for parsing arguments + def _list_to_argstring(data, argname, attr = None, operation = None): result = " --{}".format(argname) if data else "" for value in data: @@ -45,6 +47,8 @@ def _remove_duplicates_from_list(data): result.append(e) return result +# Helpers for collecting information from context + def _collect_verilog_files(ctx): transitive_srcs_list = [ dep @@ -59,21 +63,58 @@ def _collect_verilog_files(ctx): verilog_info_struct.srcs for verilog_info_struct in transitive_srcs_depset.to_list() ] - verilog_files = depset( + + return depset( [src for sub_tuple in verilog_srcs for src in sub_tuple] + ctx.files.verilog_sources, ) - return verilog_files.to_list() def _collect_vhdl_files(ctx): - return ctx.files.vhdl_sources + return depset(direct = ctx.files.vhdl_sources) -def _cocotb_test_impl(ctx): - # prepare arguments for the test command - vhdl_files = _collect_vhdl_files(ctx) - vhdl_sources_args = _files_to_argstring(vhdl_files, "vhdl_sources") +def _collect_python_transitive_imports(ctx): + return depset(transitive = [ + dep[PyInfo].imports + for dep in ctx.attr.deps + if PyInfo in dep + ]) + +def _collect_python_direct_imports(ctx): + return depset(direct = [module.dirname for module in ctx.files.test_module]) + +def _collect_transitive_files(ctx): + py_toolchain = ctx.toolchains["@bazel_tools//tools/python:toolchain_type"].py3_runtime + return depset( + direct = [py_toolchain.interpreter], + transitive = [dep[PyInfo].transitive_sources for dep in ctx.attr.deps] + + [ctx.attr.cocotb_wrapper[PyInfo].transitive_sources] + + [py_toolchain.files], + ) + +def _collect_transitive_runfiles(ctx): + return ctx.runfiles().merge_all( + [dep.default_runfiles for dep in ctx.attr.deps] + + [dep.default_runfiles for dep in ctx.attr.sim], + ) + +# Helpers for preparing test script and its environment + +def _get_pythonpath_to_set(ctx): + direct_imports = _collect_python_direct_imports(ctx).to_list() + transitive_imports = [ + "../" + path + for path in _collect_python_transitive_imports(ctx).to_list() + ] + imports = _remove_duplicates_from_list(transitive_imports + direct_imports) + return ":".join(imports) + +def _get_path_to_set(ctx): + sim_paths = _remove_duplicates_from_list([dep.label.workspace_root for dep in ctx.attr.sim]) + path = ":".join(["$PWD/" + str(p) for p in sim_paths]) + return path - verilog_files = _collect_verilog_files(ctx) +def _get_test_command(ctx, verilog_files, vhdl_files): + vhdl_sources_args = _files_to_argstring(vhdl_files, "vhdl_sources") verilog_sources_args = _files_to_argstring(verilog_files, "verilog_sources") includes_args = _list_to_argstring(ctx.attr.includes, "includes") @@ -86,21 +127,14 @@ def _cocotb_test_impl(ctx): defines_args = _dict_to_argstring(ctx.attr.defines, "defines") parameters_args = _dict_to_argstring(ctx.attr.parameters, "parameters") - verbose_args = " --verbose" if ctx.attr.verbose else "" waves_args = " --waves" if ctx.attr.waves else "" seed_args = " --seed {}".format(ctx.attr.seed) if ctx.attr.seed != "" else "" test_module_args = _pymodules_to_argstring(ctx.files.test_module, "test_module") - # define a script and a command - runner_script = ctx.actions.declare_file("cocotb_runner.sh") - - sim_paths = _remove_duplicates_from_list([dep.label.workspace_root for dep in ctx.attr.sim]) - path = ":".join(["$PWD/" + str(p) for p in sim_paths]) - command = ( - "PATH={}:$PATH ".format(path) + + "PATH={}:$PATH ".format(_get_path_to_set(ctx)) + "python {}".format(ctx.executable.cocotb_wrapper.short_path) + " --sim {}".format(ctx.attr.sim_name) + " --hdl_library {}".format(ctx.attr.hdl_library) + @@ -123,33 +157,32 @@ def _cocotb_test_impl(ctx): test_module_args ) - ctx.actions.write(output = runner_script, content = command) + return command - # specify dependencies for the script - py_toolchain = ctx.toolchains["@bazel_tools//tools/python:toolchain_type"].py3_runtime - transitive_files = depset( - direct = [py_toolchain.interpreter], - transitive = [dep[PyInfo].transitive_sources for dep in ctx.attr.deps] + - [ctx.attr.cocotb_wrapper[PyInfo].transitive_sources] + - [py_toolchain.files], +def _cocotb_test_impl(ctx): + verilog_files = _collect_verilog_files(ctx).to_list() + vhdl_files = _collect_vhdl_files(ctx).to_list() + + # create test script + runner_script = ctx.actions.declare_file("cocotb_runner.sh") + ctx.actions.write( + output = runner_script, + content = _get_test_command(ctx, verilog_files, vhdl_files), ) + # specify dependencies for the script runfiles = ctx.runfiles( files = ctx.files.cocotb_wrapper + verilog_files + vhdl_files + ctx.files.test_module, - transitive_files = transitive_files, - ).merge_all( - [dep.default_runfiles for dep in ctx.attr.deps] + - [dep.default_runfiles for dep in ctx.attr.test_module] + - [dep.default_runfiles for dep in ctx.attr.sim], + transitive_files = _collect_transitive_files(ctx), + ).merge( + _collect_transitive_runfiles(ctx), ) - # specify pythonpath for the script - test_module_paths = _remove_duplicates_from_list([module.dirname for module in ctx.files.test_module]) - pypath = ":".join([str(p) for p in test_module_paths]) - env = {"PYTHONPATH": pypath} + # specify PYTHONPATH for the script + env = {"PYTHONPATH": _get_pythonpath_to_set(ctx)} # return the information about testing script and its dependencies return [