Skip to content

Conversation

rw1nkler
Copy link
Contributor

@rw1nkler rw1nkler commented Jul 10, 2023

This PR adds the following improvements to the existing cocotb_test rule:

  • uses short_path of files instead of path to correctly handle the generated files
  • removes the cocotb_wrapper.py dependency on the cocotb to prevent circular imports and double dependencies when the rule is imported into other repositories. I encountered that problem when I tried to use the cocotb_bus library in the PR to XLS repository. The cocotb_bus library itself relies on cocotb, as a result, I obtained one copy of cocotb from the bazel_hdl_rules repo and the second from the xls repository.
  • adds all Python packages and their dependencies to the PYTHONPATH to correctly handle imports from other libraries
  • makes the cocotb_wrapper attribute public to allow for specifying different entry for other cocotb versions or different build setups

rw1nkler added 2 commits July 10, 2023 13:12
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 <[email protected]>
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 <[email protected]>
@rw1nkler rw1nkler force-pushed the 46586-cocotb-improvements branch from 785e52a to b74f614 Compare July 10, 2023 14:22
@rw1nkler rw1nkler marked this pull request as draft July 12, 2023 18:00
rw1nkler added 3 commits July 14, 2023 13:47
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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
@rw1nkler rw1nkler force-pushed the 46586-cocotb-improvements branch 2 times, most recently from fe8d3c2 to c9d3592 Compare July 14, 2023 12:33
@QuantamHD
Copy link
Collaborator

Is this ready to go for merging?

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 <[email protected]>
@rw1nkler rw1nkler force-pushed the 46586-cocotb-improvements branch from c9d3592 to 83b51f4 Compare July 18, 2023 08:46
@rw1nkler rw1nkler marked this pull request as ready for review July 18, 2023 11:16
@rw1nkler
Copy link
Contributor Author

Is this ready to go for merging?

@QuantamHD Yes, it's ready for review and merging

@proppy
Copy link
Collaborator

proppy commented Jul 19, 2023

/gcbrun

@QuantamHD QuantamHD merged commit b58d34a into hdl:main Jul 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants