verilator@5.036.bcr.4#6842
Conversation
Add missing files needed for using verilator outside Bazel-managed builds: 1. include/verilated.mk - Pre-configured Makefile generated from template with standard tool paths (ar, g++, perl, python3) and compiler flags. Required by Chisel's svsim when building verilated C++ code. 2. bin/verilator_includer - Python utility that combines multiple C++ files into one using #include directives. Required by verilated.mk for non-parallel builds (VM_PARALLEL_BUILDS=0). These additions enable verilator to work with tools like Chisel3's svsim that expect a standard verilator installation layout with makefile support. Changes to BUILD.bazel: - Add verilated_config.h to verilator_includes filegroup - Create verilator_bin_scripts filegroup for utility scripts - Add both filegroups to bin/verilator data dependencies This resolves missing file errors when external tools try to compile verilated code using the standard verilator build system.
|
Hello @UebelAndre, modules you maintain (verilator) have been updated in this PR. |
There was a problem hiding this comment.
Code Review
This pull request adds a new version verilator@5.036.bcr.4 with additional files to support using Verilator in non-Bazel environments. The overall approach of adding a new BCR-suffixed version and using an overlay for the build files is correct. However, I've found a critical issue in source.json that would cause the wrong module version to be published, and a high-severity issue in the test module configuration that causes a name collision. Please address these points to ensure the module is correct and the tests are robust.
| "overlay": { | ||
| ".bazelignore": "sha256-4Yz6+7DIt5CedRfM7s3dxN7HstNIP9KBMBXro1MaVu0=", | ||
| "BUILD.bazel": "sha256-DSmlNbdwm3/Z75KW5w8uHF42/PeWiq11h1rxAGvkO2Y=", | ||
| "MODULE.bazel": "sha256-Gf98apEz9AQVew1Jn/jJO/x+mOfo11abUeUPeaWjPRs=", |
There was a problem hiding this comment.
The MODULE.bazel file should not be part of the overlay in source.json. The MODULE.bazel for this module version is the one at modules/verilator/5.036.bcr.4/MODULE.bazel, which correctly defines the version as 5.036.bcr.4. By including overlay/MODULE.bazel here, you are overwriting it with a file that specifies version 5.036.bcr.3, which will break module resolution. The overlay/MODULE.bazel is intended for the test module only and should not be part of the published module's sources. Please remove this line.
| module( | ||
| name = "verilator", | ||
| version = "5.036.bcr.3", | ||
| bazel_compatibility = [">=7.2.1"], | ||
| ) |
There was a problem hiding this comment.
The test module defined in the overlay directory should not be named verilator. This creates a name collision with the module under test and is confusing. Please rename it to something unique, for example verilator_test. Also, the version attribute is unnecessary and confusing for a test module; it can be removed.
| module( | |
| name = "verilator", | |
| version = "5.036.bcr.3", | |
| bazel_compatibility = [">=7.2.1"], | |
| ) | |
| module( | |
| name = "verilator_test", | |
| # version is not needed for a test module | |
| bazel_compatibility = [">=7.2.1"], | |
| ) | |
Add verilator 5.036.bcr.4 with external use support
This version adds missing files needed when using verilator outside Bazel-managed builds:
Changes
include/verilated.mk - Pre-configured Makefile for compiling verilated code
bin/verilator_includer - Python utility for combining C++ files
BUILD.bazel updates:
verilated_config.htoverilator_includesfilegroupverilator_bin_scriptsfilegroup for utility scriptsbin/verilatordata dependenciesWhy these files are needed
Tools like Chisel3's svsim expect a standard verilator installation with makefile support. Previous BCR versions (5.036.bcr.3 and earlier) only provided files for Bazel-managed builds, requiring workarounds when external tools invoke verilator directly.
Testing
Resolves external use cases while maintaining full compatibility with rules_verilator.