test/orfs/mock-array: verilator config test, disabled by default#9081
test/orfs/mock-array: verilator config test, disabled by default#9081oharboe wants to merge 3 commits intoThe-OpenROAD-Project:masterfrom
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
55f024d to
583fed3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
583fed3 to
41d97af
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@MrAMS some sort of java problem... rooted in scala_rules somehow...? Thoughts? |
|
fixed? |
|
Maybe we should install JDK on the test machine? I have heared a report from my collaborator that our bazel project will fail when there is no JDK on the OS. |
No, see CI failure. |
We dont control these CI machines, so minimally tags manual tests for local use must not pull in the jdk. |
|
Let's see if this has any adverse effects first... #9095 |
41d97af to
03725de
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
/gemini review |
|
@gemini-code-assist review |
There was a problem hiding this comment.
Code Review
This pull request introduces a new test for Verilator configuration and updates Bazel dependencies and configurations, particularly for rules_scala. The changes are well-structured and improve the build's hermeticity.
My review includes a couple of suggestions for improving code cleanliness and encapsulation:
- In
test/orfs/mock-array/src/test/scala/TinyAdderTest.scala, there are some redundant and unused imports that can be cleaned up. - In
test/orfs/BUILD, the visibility of the newscala_toolchainis public, which could be narrowed for better encapsulation.
Overall, the changes look good and serve their purpose of validating the Verilator setup.
There was a problem hiding this comment.
Pull request overview
This PR adds a Verilator configuration test using a simple TinyAdder module to validate the Verilator setup in the repository. The test is tagged as "manual" so it doesn't run in CI by default but can be executed locally by maintainers for validation purposes.
Key Changes:
- Migrated from git-based rules_scala dependency to BCR version 7.1.5
- Added hermetic JDK configuration for more reliable test execution
- Created TinyAdder test module with manual tag for Verilator validation
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/orfs/mock-array/src/test/scala/TinyAdderTest.scala | New test file implementing TinyAdder module and test suite |
| test/orfs/mock-array/BUILD | Added chisel_test target with manual tag for TinyAdder validation |
| test/orfs/BUILD | Added custom Scala toolchain configuration with SemanticDB support |
| MODULE.bazel | Migrated to BCR rules_scala 7.1.5 and updated Scala version to 2.13.17 |
| .bazelrc | Added hermetic JDK configuration for test reliability |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Code Review
This pull request updates Bazel dependencies, notably for rules_scala, and introduces a new test to validate the Verilator configuration. The changes to use versioned dependencies from the Bazel Central Registry and to ensure hermetic JDK usage are great improvements. I've found a minor issue in the new Scala test file regarding unused and duplicated imports, and I've provided a suggestion to clean it up.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@maliberty Just FYI. Surely this is an unrelated failure on master: |
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
f3fcaeb to
35373d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
revisit when regression master is fixed |
This to make sure there's less dependencies on local installs and that there are no rattling screws in the bazel config, and for collaboration with @MrAMS. No changes to CI or tests as such.
This will be used to validate the verilator configuration and setup. The test is for local bazel maintainer testing, not for CI:
Result: