build(gui): isolate Qt dependency to GUI-only builds#10052
build(gui): isolate Qt dependency to GUI-only builds#10052alokkumardalei-wq wants to merge 2 commits intoThe-OpenROAD-Project:masterfrom
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
![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 restructures the GUI build system by moving Qt-specific source files and Bazel targets into a new src/gui/qt package, which prevents the @qt-bazel dependency from being loaded in non-GUI builds. Feedback points out a potential link error in CLI builds due to the separation of descriptor files, identifies redundant header labels in the new Bazel configuration, and suggests using a recursive glob for resource files to ensure all assets are included.
| "src/heatMapPinDensity.h", | ||
| "src/heatMapPlacementDensity.cpp", | ||
| "src/heatMapPlacementDensity.h", | ||
| "src/init_descriptors.cpp", |
There was a problem hiding this comment.
Moving init_descriptors.cpp to the shared gui_descriptors library while moving bufferTreeDescriptor.cpp to the Qt-only gui_qt library may introduce link errors in CLI-only builds. If init_descriptors.cpp contains code that instantiates or references symbols defined in bufferTreeDescriptor.cpp, the CLI build (which does not include gui_qt) will fail to link. Please ensure that any Qt-specific descriptor logic in init_descriptors.cpp is guarded by #ifdef ENABLE_QT or moved to a GUI-specific initialization file.
src/gui/qt/BUILD
Outdated
| "//src/gui:include/gui/MakeGui.h", | ||
| "//src/gui:include/gui/descriptor_registry.h", | ||
| "//src/gui:include/gui/gui.h", | ||
| "//src/gui:include/gui/heatMap.h", |
There was a problem hiding this comment.
These header labels are redundant because gui_qt already depends on //src/gui:gui_descriptors, which includes these files in its hdrs. Additionally, referring to files in another package using labels like //src/gui:path/to/file is generally not allowed in Bazel unless those files are explicitly exported via exports_files in the source package. Since they are already available via the dependency, they should be removed here.
There was a problem hiding this comment.
yeah, just depend on the library that exports these heaers.
src/gui/qt/BUILD
Outdated
| "resources/*.png", | ||
| "resources/google_icons/*.png", |
There was a problem hiding this comment.
The new glob is more restrictive than the previous resources/**/*.png. If new subdirectories are added to resources/, or if the .qrc file references non-PNG files (such as the LICENSE file found in google_icons/), they will be missed by this glob, potentially leading to build failures. It is safer to use a recursive glob to ensure all resources are captured.
| "resources/*.png", | |
| "resources/google_icons/*.png", | |
| "resources/**/*.png", | |
| "resources/**/LICENSE", |
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
2f42a77 to
1599820
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
1599820 to
b388833
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
OpenROAD Qt Isolation Review -- Consolidated StatusDate: 2026-04-04 1. The Original Request (Issue #9936)Filed by: @hzeller (Henner Zeller) on 2026-03-25 Problem: Proposed solution: Create a Strategic importance (from @hzeller's follow-up): This separation is vital for getting OpenROAD onto the Bazel Central Registry (BCR). Without it, anyone using Stakeholder positions:
2. PR #10043 (First Attempt) -- Timeline
Open review feedback from PR #10043:
3. @oharboe's Review Checklist (posted on issue #9936)This is the most detailed review guidance provided. None of these items have been addressed in either PR. 3.1 Prove it buildsPost build output showing:
3.2 Run buildifierbuildifier src/gui/BUILD src/gui/qt/BUILD BUILD.bazel3.3
|
| # | Item | Source | Status |
|---|---|---|---|
| 1 | Prove CLI builds without fetching @qt-bazel -- post build log |
@oharboe on #9936 | NOT DONE |
| 2 | Prove GUI build still works -- post build log | @oharboe on #9936 | NOT DONE |
| 3 | Run buildifier on all modified BUILD files |
@oharboe on #9936 | NOT DONE |
| 4 | Remove exports_files redundancy -- use filegroup alone |
@oharboe on #9936, @hzeller on #10043 | NOT DONE |
| 5 | Don't use exports_files() as a pattern -- use cc_library + filegroup |
@hzeller on #10043 | UNCLEAR |
| 6 | Remove unnecessary "ui" from includes if build passes without it |
@oharboe on #9936 | NOT DONE |
| 7 | Test for circular dependency in uic_lib <-> gui_qt headless path |
@oharboe on #9936 | NOT DONE |
| 8 | Test qt6_resource_library with filegroup labels in data_srcs |
@oharboe on #9936 | NOT DONE |
| 9 | Resources should be a separate library in src/gui, not aliases |
@hzeller on #10043 | UNCLEAR |
Must-fix (from bot reviewers on #10052)
| # | Item | Source | Status |
|---|---|---|---|
| 10 | Check init_descriptors.cpp / bufferTreeDescriptor.cpp split for link errors in CLI mode |
gemini-code-assist on #10052 | NOT DONE |
| 11 | Remove redundant header labels in src/gui/qt/BUILD lines 41-44 |
gemini-code-assist on #10052 | NOT DONE |
| 12 | Fix resource glob to be recursive (resources/**/*.png) and include LICENSE |
gemini-code-assist on #10052 | NOT DONE |
Process items
| # | Item |
|---|---|
| 13 | Close PR #10043 with a comment linking to #10052 as the replacement |
| 14 | On #10052, add a comment summarizing which feedback items from #10043 and #9936 have been addressed |
| 15 | For future iterations: push fixup commits to the existing PR branch rather than opening new PRs -- this preserves review history and thread continuity |
| 16 | Post build evidence (copy-paste terminal output) directly on the PR for items 1 and 2 |
7. Reference Links
| Resource | URL |
|---|---|
| Feature request | #9936 |
| PR #10043 (abandoned) | #10043 |
| PR #10052 (current) | #10052 |
| BCR context (referenced) | #9702 |
| @oharboe's 6-point review | #9936 (comment) |
|
Thanks for the detailed review, @oharboe! Here's a summary of how everything's been addressed: Qt Isolation
Shared Code Architecture
layering_check & Headers
Headless Support
TestingCMake: All Jenkins tests pass |
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
b388833 to
5b8f2e6
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
I think this looks a lot better already than the first version that relied on export_files for instance. In general, we want that every header is only mentioned in exactly one place. So if needed, make a separate library that can encapsualte everything needed and then link that library in the different places. |
|
Thanks @hzeller! I agree, each header should be declared in exactly one place. My plan is to create a header-only gui_headers cc_library in src/gui/BUILD that owns the 4 public headers (MakeGui.h, gui.h, descriptor_registry.h, heatMap.h), then have gui_descriptors, gui_heatmap_core, gui_qt, and openroad_lib all depend on it. Since it's header-only with no Qt dependency, it's safe for both CLI and GUI builds. One thing I want to verify- Are these headers include other headers themselves (e.g. gui.h pulls in ODB/STA types), so I need to make sure gui_headers either has the right deps or those are resolved through the depending targets. I'll test and update the PR. |
|
if the headers include other headers of other libraries, you need to add these deps to the library. |
…ndency (The-OpenROAD-Project#9936) Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
5b8f2e6 to
75a27bb
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @hzeller , it's done. Please have a look when you have time and let me know if I have missed something and would be happy to address that . Thank you! |
src/gui/BUILD
Outdated
| "include/gui/heatMap.h", | ||
| ], | ||
| includes = [ | ||
| ".", |
There was a problem hiding this comment.
Thanks @hzeller it's a good catch , will remove it and update my PR
|
mmh, I just tried if this will allow us to not need the bazel dependency in the MODULE.bazel of downstream projects (which is what I am after with #9936 So the litmus test is: cd test/downstream
vi MODULE.bazel # comment out all the qt-bazel stuff here
bazel build -c opt @openroad//:openroad... but that results in an error, as it seems to still want to locate the qt bazel: So to make this work wothwhile, we need to see if we can achievve that... |
Thanks for testing this @hzeller! You're right — the sub-package isolation alone doesn't solve the downstream problem since MODULE.bazel still declares qt-bazel as a regular bazel_dep. I think we can achieve this by marking qt-bazel as dev_dependency = True along with its git_override. That would make it invisible to downstream consumers while still available for the root project's GUI builds. I can update test/downstream/MODULE.bazel to remove the qt-bazel lines and verify the litmus test passes. Does that sound right, or do you see issues with the dev_dependency approach? |
|
while using |
Got it, makes sense- marking it as dev_dependency would be semantically wrong since it's a real dependency for GUI builds. I'll drop that idea. I'll address your other feedback (removing "." from gui_headers includes) and update the PR. Thank you ! |
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
75a27bb to
15c7f7b
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
15c7f7b to
6c1d600
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
bazel/isolate-qt-subpackage
branch
from
6c1d600 to
64e942a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Should we close this based on "we actually can't achieve what we want with this"? |
|
Hello @maliberty you’re right. This PR only isolates the Qt targets at the BUILD level, so non-GUI builds avoid analyzing //src/gui/qt, but it does not make Qt fully optional because qt-bazel is still declared in MODULE.bazel. If the goal is true optional Qt support, we likely need a different design, such as separating the GUI into its own module or repo. I’m okay with closing this PR . Thank you ! |
4 participants
Problem
src/gui/BUILD unconditionally loads @qt-bazel at the package level. Since Bazel evaluates all load() statements before resolving targets, this forces @qt-bazel to be fetched and analyzed regardless of whether you're building with --//:platform=cli or --//:platform=gui. CLI-only users end up downloading a Qt dependency they never use.
What this PR does?
Fixes #9936
Moves the Qt-specific targets (gui_qt, qt_resources, uic_lib) into a new Bazel package at //src/gui/qt. The @qt-bazel load now lives exclusively in src/gui/qt/BUILD, which only enters the dependency graph for --//:platform=gui builds. Source files are exposed from //src/gui via filegroup and exports_files rules with visibility scoped to the new sub-package.
Changes made
src/gui/BUILD — removed @qt-bazel load and Qt targets; added filegroups to share sources with //src/gui/qt
src/gui/qt/BUILD — new package containing all Qt-dependent targets
BUILD.bazel — updated //src/gui:gui_qt → //src/gui/qt:gui_qt
Testing
Terminal output for CLI bulid:
bazelisk build :openroad INFO: Invocation ID: ff149393-b623-4060-834e-11e3f45027ff WARNING: Build option --//:platform has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed). DEBUG: /Users/alokkumardalei/Desktop/open-road/OpenROAD/bazel/notification.bzl:11:14: ================================================================================ NOTIFICATION: Use --config=opt to build with LTO (Link Time Optimization) and get better performance at the expense of longer build time. ================================================================================ INFO: Analyzed target //:openroad (0 packages loaded, 37134 targets configured). ERROR: /Users/alokkumardalei/Desktop/open-road/OpenROAD/BUILD.bazel:163:10: Linking openroad failed: (Exit 1): cc_wrapper.sh failed: error executing CppLink command (from target //:openroad) external/toolchains_llvm++llvm+llvm_toolchain/bin/cc_wrapper.sh @bazel-out/darwin_arm64-opt/bin/openroad-0.params Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging ld: warning: ignoring duplicate libraries: '-lm', '-lpthread' Undefined symbols for architecture arm64: "std::__1::__from_chars_result<float> std::__1::__from_chars_floating_point<float>(char const*, char const*, std::__1::chars_format)", referenced from: sta::stringFloat(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in libopensta_lib.a[158](StringUtil.o) ld: symbol(s) not found for architecture arm64 clang: error: linker command failed with exit code 1 (use -v to see invocation) Target //:openroad failed to build Use --verbose_failures to see the command lines of failed build steps. INFO: Elapsed time: 5.701s, Critical Path: 2.36s INFO: 12 processes: 7287 action cache hit, 8 disk cache hit, 4 internal. ERROR: Build did NOT complete successfullyTerminal output for GUI buld:
bazelisk build --//:platform=gui :openroad INFO: Invocation ID: 1f194354-3010-4a45-995c-251910393f88 WARNING: Build option --//:platform has changed, discarding analysis cache (this can be expensive, see https://bazel.build/advanced/performance/iteration-speed). DEBUG: /Users/alokkumardalei/Desktop/open-road/OpenROAD/bazel/notification.bzl:11:14: ================================================================================ NOTIFICATION: Use --config=opt to build with LTO (Link Time Optimization) and get better performance at the expense of longer build time. ================================================================================ INFO: Analyzed target //:openroad (0 packages loaded, 44224 targets configured). ERROR: /Users/alokkumardalei/Desktop/open-road/OpenROAD/BUILD.bazel:163:10: Linking openroad failed: (Exit 1): cc_wrapper.sh failed: error executing CppLink command (from target //:openroad) external/toolchains_llvm++llvm+llvm_toolchain/bin/cc_wrapper.sh @bazel-out/darwin_arm64-opt/bin/openroad-0.params Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging ld: warning: ignoring duplicate libraries: '-lm', '-lobjc', '-lpthread' Undefined symbols for architecture arm64: "std::__1::__from_chars_result<float> std::__1::__from_chars_floating_point<float>(char const*, char const*, std::__1::chars_format)", referenced from: sta::stringFloat(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char>> const&) in libopensta_lib.a[158](StringUtil.o) ld: symbol(s) not found for architecture arm64 clang: error: linker command failed with exit code 1 (use -v to see invocation) Target //:openroad failed to build Use --verbose_failures to see the command lines of failed build steps. INFO: Elapsed time: 4.562s, Critical Path: 2.42s INFO: 12 processes: 9431 action cache hit, 8 disk cache hit, 4 internal. ERROR: Build did NOT complete successfullyHere in both build processes you can see the same error i.e the linker error, the
std::from_chars_floating_pointfailure inlibopensta_lib.areproduces on upstream master, it's a pre-existing toolchain issue on macOS ARM64, unrelated to changes made in this PR (I can be wrong here).