build(gui): isolate Qt dependency to GUI-only builds#10043
build(gui): isolate Qt dependency to GUI-only builds#10043alokkumardalei-wq wants to merge 1 commit 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 refactors the Bazel build configuration to isolate the @qt-bazel dependency. Qt-specific targets are moved from //src/gui to a new sub-package //src/gui/qt, ensuring that the external Qt dependency is only fetched and analyzed during GUI builds. Feedback includes addressing a missing dependency in the uic_lib target to prevent non-deterministic build failures and removing an unnecessary and fragile relative path from the includes attribute.
| cc_library( | ||
| name = "uic_lib", | ||
| hdrs = [ | ||
| ":ui_findDlg.h", | ||
| ":ui_gotoDlg.h", | ||
| ":ui_highlightGroupDlg.h", | ||
| ":ui_selectedWidget.h", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
The uic_lib target references generated headers (e.g., :ui_findDlg.h) but lacks a dependency on the target that produces them (:gui_qt). In Bazel, this can cause non-deterministic build failures because there is no guarantee that the headers will be generated before uic_lib is compiled. Since uic_lib is used by the headless build to access headers generated by the native build, it should explicitly depend on the native :gui_qt target.
| cc_library( | |
| name = "uic_lib", | |
| hdrs = [ | |
| ":ui_findDlg.h", | |
| ":ui_gotoDlg.h", | |
| ":ui_highlightGroupDlg.h", | |
| ":ui_selectedWidget.h", | |
| ], | |
| ) | |
| cc_library( | |
| name = "uic_lib", | |
| hdrs = [ | |
| ":ui_findDlg.h", | |
| ":ui_gotoDlg.h", | |
| ":ui_highlightGroupDlg.h", | |
| ":ui_selectedWidget.h", | |
| ], | |
| deps = [":gui_qt"], | |
| ) |
src/gui/qt/BUILD
Outdated
| "../include", | ||
| "../src", | ||
| "../ui", |
There was a problem hiding this comment.
Using .. in the includes attribute is a Bazel anti-pattern as it can lead to fragile builds and may not work correctly if the package is used as an external dependency. Additionally, the ../ui path (resolving to src/gui/ui) appears to be unnecessary here; the generated UIC headers are located in the current package's output directory, which is automatically included in the search path, and src/gui/ui contains only source .ui files, not headers. Consider removing ../ui and refactoring the other paths to use a helper cc_library in //src/gui that exports the necessary include directories.
| "../include", | |
| "../src", | |
| "../ui", | |
| "../include", | |
| "../src", |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
eb5f465 to
007e839
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
007e839 to
2afcf00
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
2 times, most recently
from
b2b99e4 to
2a2fd98
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
6d10cd5 to
b3c8e63
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
src/gui/qt/BUILD
Outdated
| # A Qt 'headless' build is added for environments that require GUI features | ||
| # but do not have an attached display. This requires setting 'qt_gui_platform' | ||
| # to `headless`, and substituting the dependency in the top-level BUILD. | ||
| alias(name = "resources/resource.qrc", actual = "//src/gui:resources/resource.qrc") |
There was a problem hiding this comment.
This set of files look like they want to be in a separate library that lives in src/gui and then is referenced here.
src/gui/BUILD
Outdated
| ] | ||
| # Source filegroups exposed for //src/gui/qt so that @qt-bazel only needs | ||
| # to be loaded in that sub-package (and therefore only for GUI builds). | ||
| exports_files( |
There was a problem hiding this comment.
avoid exports_files(). It is always only ever a last resort in bazel projects.
Instad, make a specific library (for the code) and possibly a filegroup() for the resources.
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
089e186 to
57d118c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
57d118c to
3f3d9b5
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
2 times, most recently
from
cf0bf17 to
2d17bd3
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Conceptually ok but there are issues to resolve |
|
This seems overly complicated and fragile to be honest. I see the value in not loading this dependency, but theoretically this is a one time cost, and the clang download is likely to take longer anyways. I'd advocate for keeping it simple and reliable |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
2d17bd3 to
c45f9eb
Compare
|
the problem is, that this prevent openroad to be put on BCR because we would require to always use the qt downlaod as well. |
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
116d22c to
c45f9eb
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
2 similar comments
|
clang-tidy review says "All clean, LGTM! 👍" |
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
1835d7f to
fcbd01c
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
fcbd01c to
3f78b27
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
…ndency (The-OpenROAD-Project#9936) Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
alokkumardalei-wq
force-pushed
the
fix/bazel-qt-cli-only-build
branch
from
3f78b27 to
630c376
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Got some conflict with this branch.. opening new PR sorry for the inconvenience. |
|
Hello everyone I am closing this PR because I am facing some conflict with this branch very sorry for the inconvenience. Here is my PR link : #10052 Thank you so much for your time and consideration. |
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