build: dev_dependency cleanup, visibility, and downstream test#9827
build: dev_dependency cleanup, visibility, and downstream test#9827oharboe wants to merge 17 commits intoThe-OpenROAD-Project:masterfrom
Conversation
These targets are internal implementation details, not part of the external API. External consumers should only depend on //:openroad (the CLI binary). Restricting visibility to //:__subpackages__ prevents downstream modules from accidentally coupling to internal libraries. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Give the Python bindings target a clean external name. This is the public API for scripting OpenROAD from downstream projects via Python. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
The root BUILD.bazel had load() statements for @rules_pkg and @rules_shell which are evaluated whenever any target in the root package is requested. This prevents making these deps dev_dependency, because downstream consumers loading @openroad//:openroad would fail. Moving the packaging targets to their own package isolates the load() statements so they are only evaluated when explicitly needed. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
…_dependency These deps are only needed for packaging (rules_shell, rules_pkg) and test/orfs simulation targets (rules_verilator, verilator). They are not required by the //:openroad binary. Marking them dev_dependency prevents MVS from forcing their versions on downstream consumers. Also split register_toolchains so the verilator toolchain registration is dev_dependency — it only needs to be active when building OpenROAD directly, not when consuming it as a dependency. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Replace @//:__subpackages__ with //:__subpackages__ in visibility declarations. The @// prefix resolves to the root module in bzlmod, not the current repository. When OpenROAD is used as a dependency, @// points to the downstream project, making internal targets invisible to OpenROAD's own subpackages. Also restore //visibility:public on openroad_py — this is part of the public API for scripting OpenROAD from downstream projects. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
toolchains_llvm enforces root-module-only extension usage, causing failures when OpenROAD is consumed as a dependency. The bazel_dep stays non-dev (llvm_prebuilt's build_file references it), but the use_extension and register_toolchains are now dev_dependency. Downstream consumers must configure their own C++ toolchain. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Add a mock downstream project (test/downstream/) that verifies: - Public targets (//:openroad, //:openroad_py) are visible - Internal targets (//:openroad_lib, //:_openroadpy.so) are restricted - Dev dependencies (rules_shell, rules_pkg, rules_verilator) are not leaked to downstream consumers The test uses local_path_override to consume OpenROAD as a dependency, matching how real downstream projects would use it. Wired as //test:downstream_visibility_test in the main build. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Add a section to Bazel.md covering the public API surface (//:openroad and //:openroad_py), the minimal MODULE.bazel boilerplate downstream consumers need (qt-bazel git_override, toolchains_llvm configuration), and which deps are dev_dependency. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Updated lockfile after making toolchains_llvm extension dev_dependency. Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Claude Code <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| clang-tidy review says "All clean, LGTM! 👍" |
| @hzeller Thoughts? |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request provides a comprehensive set of improvements for using OpenROAD as a downstream Bazel dependency. Marking appropriate dependencies as dev_dependency, cleaning up target visibility, and improving API naming are all valuable changes. The addition of a dedicated downstream test workspace is particularly commendable as it ensures these improvements are verifiable and maintained over time. The accompanying documentation is also clear and will be very helpful for users. I have one minor suggestion to improve the conciseness of the MODULE.bazel file.
I am having trouble creating individual review comments. Click here to see my feedback.
MODULE.bazel (113-121)
These two register_toolchains calls can be combined into a single, more concise call. The register_toolchains function accepts multiple toolchain labels, and the dev_dependency = True argument will apply to all of them.
register_toolchains( "@llvm_toolchain//:all", "@rules_verilator//verilator:verilator_toolchain", dev_dependency = True, ) 
| Thanks, very nice to document downstream use and making dev dependencies more clear. |
| @@ -0,0 +1,57 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
make this /usr/bin/env bash
There was a problem hiding this comment.
I will try to remember, but we'd be better of telling Claude :-)
This is just a throwaway investigation, it is quickly generated low accuracy information that may merit further investigation or not... I'm just not quite over the novelty of it.
Another example of using Claude to run the bazel-orfs flow: https://github.com/oharboe/openroad-demo
I predict bazel is going to be the most used build language in the universe: automatically generated AI plumbing that handles dependencies and caching...
There was a problem hiding this comment.
Does that mean you don't want this reviewed or merged?
There was a problem hiding this comment.
make this
/usr/bin/env bash
I want to address that separately, the code base is endemic with inconsistencies here.
# Conflicts: # BUILD.bazel
| clang-tidy review says "All clean, LGTM! 👍" |
The rules_chisel bazel_dep is declared as dev_dependency (line 22), but the corresponding use_extension was not. This caused downstream consumers to fail with "no repo visible as '@rules_chisel'". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
rules_shell is a dev_dependency and must not be loaded from the root BUILD.bazel, which downstream consumers parse. The native sh_test and sh_binary rules work without an external dependency. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Two issues prevented the test from working under bazelisk test: - dirname "$0" resolved to the runfiles tree, not the source tree; use readlink -f to follow the symlink to the real script location - HOME is not set in Bazel test environments; bazelisk needs it to locate its cache directory Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
| clang-tidy review says "All clean, LGTM! 👍" |
- Add buildifier disable comments for native sh_test/sh_binary in root BUILD.bazel (rules_shell is a dev_dependency, cannot be loaded by downstream consumers) - Patch rules_chisel to handle dev_dependency in extension_metadata, fixing "root_module_direct_deps must be empty" error - Regenerate MODULE.bazel.lock Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
| clang-tidy review says "All clean, LGTM! 👍" |
3 participants
TL;DR use-cases:
Summary
rules_shell,rules_pkg,rules_verilator,verilatorasdev_dependencyso MVS doesn't force their versions on downstream consumerstoolchains_llvmextension and toolchain registrationdev_dependency(avoids root-module-only enforcement failure)installandtarfiletargets topackaging/so theirload()statements don't break downstream consumers whenrules_shell/rules_pkgare dev deps//:openroad_libvisibility to//:__subpackages__— only//:openroadand//:openroad_pyare publicord_py→openroad_pyfor a clean external API name@//:__subpackages__→//:__subpackages__in visibility labels (@//resolves to root module in bzlmod, breaking internal visibility when OpenROAD is a dependency)test/downstream/mock project that verifies visibility and dev_dependency isolation (0.5s, runs withbazelisk test //test/...)docs/user/Bazel.mdMotivation
When downstream projects pull in OpenROAD as a
bazel_dep, MVS forces unnecessarily high versions of deps likerules_cc 0.2.17(viarules_shell 0.6.1etc.).rules_cc >= 0.2.14introducescc_compatibility_proxywhich creates a WORKSPACE cycle for any downstream project still usingWORKSPACE.bazel. Thetoolchains_llvmextension also fails with "Only the root module can use the 'llvm' extension" when OpenROAD is not root.Test plan
bazelisk build --nobuild //:openroadfrom OpenROAD rootbazelisk build --nobuild //packaging:installfrom OpenROAD roottest/downstream/visibility_test.sh— 7/7 pass in 0.5sbazelisk test //test/...picks up//test:downstream_visibility_test🤖 Generated with Claude Code