bazel: make downstream-facing targets publicly visible#10099
bazel: make downstream-facing targets publicly visible#10099oharboe wants to merge 4 commits intoThe-OpenROAD-Project:masterfrom
Conversation
openroad_lib, ord, error_swig, error_swig-py, and options_swig were restricted to //:__subpackages__. Downstream Bazel consumers that depend on @openroad need these targets. The CMake build already exposes them unconditionally. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@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 updates the visibility of several Bazel targets in the BUILD.bazel file, changing them from subpackage-restricted to public. This change affects multiple cc_library and filegroup targets, making them accessible outside of the current package hierarchy. I have no feedback to provide as there are no review comments.
|
Looks good to me in general. We mostly need to make sure to not have things that are internal artifacts and are likely to change are kept private, but this looks like a good set of candiates that look like they want to be public. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
How do you use the .i files downstream? |
Minimal bzlmod workspace in test/visibility/ that aliases the five OpenROAD targets downstream consumers need (openroad_lib, ord, error_swig, error_swig-py, options_swig). `bazel build --nobuild :all` fails at analysis time if any target is not publicly visible. Not run in CI — manual verification only. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
Added a test and the best explanation I have. |
|
clang-tidy review says "All clean, LGTM! 👍" |
test/visibility/WORKSPACE.bazel
Outdated
| @@ -0,0 +1 @@ | |||
| # Empty — bzlmod only. | |||
There was a problem hiding this comment.
Don't add a WORKSPACE. That is only needed in pre bazel 7 era times that we have passed.
test/visibility/BUILD.bazel
Outdated
| ) for t in [ | ||
| "openroad_lib", | ||
| "ord", | ||
| "error_swig", |
There was a problem hiding this comment.
while the openroad_lib and ord seem reasonable, I wonder why you need the swigged versions
test/visibility/README.md
Outdated
|
|
||
| ## Problem | ||
|
|
||
| OpenROAD's `BUILD.bazel` restricted several targets to |
There was a problem hiding this comment.
this looks like an LLM explanation: full of words, but not explaining anything.
What we expect here is something like: libraries x, y and z are needed downstream for x-reason, y-reason and z-reason. This is why they are set public.
There was a problem hiding this comment.
Trying to get rid of docker, this was one fix that Claude needed to make it compile. I expect that when the dust settles and we get other things merged, clarity will come.
There was a problem hiding this comment.
if claude ran into the issue, it might be able to explain why it needed that visibility.
There was a problem hiding this comment.
I prefer starting with something that works even if I dont know why and then whittling away at it. Claude changes nothing: I use OpenROAD and ORFS without understanding them fully. I start with something that works.
ideally claude was mistaken here and nothing is needed here... We dont yet know what is going on and why claude decided it needed this. It will come. Thats the beauty of patching openroad in bazel-orfs and downstream projects, we can deal with the pain first with a patch and then figure out what is going on and what is needed in openroad
Thank you for sheding light on this so far. Even a conclusive "I have no idea" is a strong signal.
There was a problem hiding this comment.
I am worried about this-was-needed-at-some-time-but-nobody-knew-why resulting in a bunch of leftover things that might just hang in there forever because someone later thinks "oh, there was probably a good reason".
Maybe add a cleanup issue that collects all the pull requests to get things going, so that it is possible to re-viisit later and clean up things that are not needed. We humans need to use these tools to keep the software maintainable which the LLM tools don't understand or care about - they just are text completion tools.
There was a problem hiding this comment.
I think we have a good process now. This change is suspect, whereas others sail straight through, we know they are good. I'm fine to leave this hanging until we are sure exactly what is going on. There are many other pull requests in flight that are much less questionable to sort out first.
There was a problem hiding this comment.
Now if you do want a more holistic approach to this, check out origin/main of bazel-orfs and figure out how to get it to build without patches...
I'm thinking that's not an ideal solution either. Standalone, sometimes a bit mysterious, PRs and investing in understanding the context when there's a reason to, is a better way to do it...
There was a problem hiding this comment.
The horrific thing about getting rid of docker are the enormous turnaround times. When I tinker with something that makes the openroad/yosys bazel cache useless, 20000 files have to be rebuilt + hours of ORFS flows.
Only openroad_lib and ord need public visibility for downstream consumers. The SWIG filegroups (error_swig, error_swig-py, options_swig) are only referenced by internal subpackages, which already satisfy __subpackages__ visibility. No downstream consumer references them directly. Also remove WORKSPACE.bazel (pre-Bazel 7 artifact) and rewrite README.md with verified explanations instead of hallucinated ones. 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! 👍" |
3 participants
openroad_lib, ord, error_swig, error_swig-py, and options_swig were restricted to //:subpackages. Downstream Bazel consumers that depend on @openroad need these targets. The CMake build already exposes them unconditionally.