add an openroad-qt as an alternative to GUI Qt config to reduce build times#10384
add an openroad-qt as an alternative to GUI Qt config to reduce build times#10384oharboe wants to merge 7 commits into
Conversation
This is a report and proposal for discussion. The intent is to surface the OpenROAD-side mechanics of an idiom shift; downstream changes follow once the team agrees on direction. Replace the :platform string_flag (cli/gui) with two cc_binary targets: :openroad (CLI, default — what build-time tool consumers should pull) and :openroad-qt (Qt GUI variant — opt-in for interactive use). The choice between GUI variants becomes a target choice, not a build configuration. Why this change --------------- A skylib string_flag has no --host_ form, so the platform setting is silently dropped under Bazel's exec transition. Rule libraries that pull OpenROAD as a build-time tool (cfg = "exec") therefore got a *different* OpenROAD binary than `bazelisk build //:openroad` produced under a downstream that set `--@openroad//:platform=gui` -- same label, two binaries. With LTO, that is two ~30 min link steps for any consumer whose CI builds both directly and via flow stages. Beyond compile cost, the conceptual problem is target-name overload: @openroad//:openroad should denote one binary. Today, the binary it denotes depends on the consumer's transition. Qt is on a deprecation path -- a web GUI replacement that depends only on boost is planned. When Qt goes away, deleting one cc_binary is cleaner than removing a string_flag, two config_settings, every select() arm, and every downstream `--@openroad//:platform=gui` line. Why this lands first (and not the broader exec/target story) ------------------------------------------------------------ The conflation users feel in `bazelisk build` commands has two layers: 1. The GUI flag asymmetry -- addressed here. After this change, :openroad means one thing, regardless of who pulls it or in what configuration. 2. The deeper exec/target distinction. Bazel maintains separate target and exec configurations even when the build host is the runtime host (which is always true for ASIC EDA tools -- no chip runs OpenROAD). With this PR landed and downstream flag mirroring (--config=release plus symmetric --host_* copts) in place, target-config and exec-config compile actions hash identically and the action cache dedups the actual compile work. The visible cost shrinks to two analysis passes -- cheap. Layer (2) is conceptually thornier and less load-bearing once layer (1) is fixed: action-cache dedup makes "two analyses" cheap, and a label-disambiguation convention (e.g. @openroad//tools:openroad as an exec-transitioned alias) is available if the visible duality ever bites in practice. Layer (1) is shipping first because: - It produces *two different binaries*. Layer (2) produces the same binary in two configs, which the action cache already deduplicates. - It eliminates the only flag that cannot be `--host_*`-mirrored. - It is a strict no-regression improvement under every workflow we measured. Downstream CI scenarios that build :openroad directly plus flow stages go from 2x to 1x. Flow-stage-only workflows (gallery's typical use) are unchanged in compile count and gain conceptual clarity. We expect to revisit (2) next, focused on whether `cfg = "exec"` in downstream rule libraries earns its abstraction cost in a domain that does not cross-compile, or whether a label-disambiguation convention suffices. No code change there is required until the team agrees on which framing fits. Effect on the public consumer (bazel-orfs gallery) -------------------------------------------------- Gallery's bazel-out today contains openroad only in the exec config (k8-opt-exec-*) -- it is built once, not twice, in the typical flow- stage-only workflow. That remains true after this change. Gallery does not currently set --@openroad//:platform=gui, so its observable behavior is unchanged; it just stops having a string_flag in scope that could ever produce a phantom GUI build if the flag were added. What changes ------------ - BUILD.bazel only. - :openroad continues to be the CLI binary (the existing default of the old string_flag). Downstreams that did NOT set --@openroad//:platform=gui see no change. - :openroad-qt is new. Downstreams that previously set --@openroad//:platform=gui should drop that flag and build :openroad-qt directly when they want the Qt UI. - :openroad_lib_qt is a second cc_library variant, identical to :openroad_lib except for BUILD_GUI=true and the Qt gui dep. When both binaries are requested in the same invocation, the shared sources are compiled twice -- the same compile count as today's two-config setup, no regression. A future optimization can split the lone BUILD_GUI consumer into a tiny per-binary cc_library so openroad_lib is shared; deferred because the "build both binaries simultaneously" workload is rare in practice. What does not change -------------------- - CMake build (intentional; CMake is on its own deprecation path, minimum-churn principle applies). - C++ source. The lone BUILD_GUI consumer (OpenRoad::getGUICompileOption() at src/OpenRoad.cc:753-756) is unmodified -- its define is now driven by the cc_binary's lib variant rather than the string_flag. - :_openroadpy.so -- already CLI-only in practice; now references :openroad_lib (the CLI variant). Migration for downstreams ------------------------- - Remove `build --@openroad//:platform=gui` from .bazelrc. - For interactive Qt use: `bazelisk build @openroad//:openroad-qt` (or add `bazelisk build --config=...` aliases as fits the workflow). - For build-time tool use (rule libraries pulling OpenROAD via cfg = "exec"): no change -- @openroad//:openroad continues to be the right label. Open questions for the next round --------------------------------- - Do we agree :openroad-qt is the right shape, or is the label naming off (:openroad_qt vs :openroad-qt vs :openroad_gui)? - Should :openroad_lib_qt be visible publicly or stay internal? - Should the future web GUI variant (:openroad-web?) follow the same pattern when it lands? - For (2) above (exec/target conflation): is a label-disambiguation alias warranted, or do we accept the two-analysis cost as a property of Bazel's tool abstraction? Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
`:openroad` is implicitly exercised in CI through downstream rule libraries that pull it as a build-time tool, but `:openroad-qt` has no such consumer. Pin its compilability with a skylib build_test so a future change can't silently break the Qt variant. Run with: bazelisk test //:openroad_qt_build_test Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
![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 replace the flag-based platform selection with explicit targets for CLI and GUI builds. It introduces the openroad-qt binary and splits the library into openroad_lib and openroad_lib_qt variants. A build test was also added to ensure the Qt variant remains compilable. Feedback indicates that the openroad-qt target is missing the OPENROAD_DEFINES attribute, which is necessary for successful compilation of the entry point.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
this looks like a step backwards
|
The pain that motivates this change is openroad being built 2x |
|
Let this dog lie for now |
|
It is only built twice if you consider building it with and without gui. |
Which happens, too high cognitive load. Users dont have a strong mental model of what is going on and I think POLA they shouldnt have. |
|
Example of pain:
|
|
you just need the 20min to build the gui version once and feed the artifacts you build offline before. Most of the servers you run the stuff don't have any UI, X libraries etc. The gui version really is only useful for the interactive exploration of whatever you generated offline. |
exactly! which us why two binaries rather than a config expresses this idiomatically. |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Since Qt is on a deprecation path, I'll see if I can handle this in bazel-orfs without polluting OpenROAD. After looking at this for a bit, it just looks painful and OpenROAD still needs patching to support it. I'd much prefer if there was an //:openroad-qt binary that I can use for the viewing use-case and an //:openroad headless I use for the build use-case. I'll update bazel-orfs to use //:openroad-qt for the viewing use-case until it is sunset by the new web GUI. |
The Mac-Build workflow invoked `--//:platform=gui //:openroad`, but the previous commit removed the `:platform` string_flag in favor of the dedicated `//:openroad-qt` target. Switch the workflow to build that target directly. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
hzeller flagged on The-OpenROAD-Project#10384 that the prior commit moved srcs and shared deps for the CLI/Qt library variants into module-level Starlark lists (OPENROAD_LIB_SRCS, OPENROAD_LIB_COMMON_DEPS), hiding the dep set from the cc_library that owns it. Inline both lists into each cc_library so every dep is visible at the target that uses it. The Bazel deps graph is unchanged. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Gemini code assist on The-OpenROAD-Project#10384 flagged that :openroad-qt does not declare `defines = OPENROAD_DEFINES` even though src/Main.cc uses BUILD_TYPE from that list. In practice the macros propagated via the transitive :openroad_lib_qt dep, so the build worked, but the cc_binary was relying on a non-local invariant. Declare the defines on both top-level binaries so each is self-contained at the attribute level — the symmetry also makes future dep-graph reshuffles safe. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request refactors the build system by replacing the platform-based configuration flag with explicit targets for the CLI and GUI versions of OpenROAD. It introduces openroad-qt and openroad_lib_qt for the GUI variant and adds a build test to ensure the GUI version remains compilable. The reviewer suggested refactoring the duplicated dependencies between the CLI and GUI binary targets into a shared list to improve maintainability and prevent configuration drift.
|
CC @QuantamHD as he originally introduced the compilation flag. |
Both top-level cc_binaries (:openroad CLI, :openroad-qt) link the same runtime libraries (openroad_version, opt_notification, ord, tcl_readline_setup, tcl_library_init, cut, sta, utl, web, boost.stacktrace, runfiles, tcl). The previous form listed all 12 deps twice and duplicated the cc_binary attrs around them, which is what prompted hzeller's "hides more dependencies in variables" feedback and what Gemini's missing-OPENROAD_DEFINES comment indirectly flagged. Introduce :openroad_main_deps that owns the shared link deps as a proper Bazel target (not a Starlark list variable), and shrink both cc_binary deps lists to three explicit entries: the variant openroad library, :openroad_main_deps, and the variant gui impl. The variant choice is unambiguous and inline at each binary; everything else is named once. Bazel's layering_check requires direct deps for header visibility, so Main.cc cannot reach headers through an aggregator. Disable layering_check on each cc_binary so the single .cc file may consume the aggregator's transitive headers — the safety this gives up is bounded (one file per binary), and the alternative is to keep listing every dep inline at both binaries. Also add `defines = OPENROAD_DEFINES` to both cc_binaries (per Gemini code assist on The-OpenROAD-Project#10384): each binary is now self-contained at the attribute level rather than relying on transitive propagation from :openroad_lib*. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bazel build configuration by replacing dynamic platform selection with explicit binary targets for the CLI and GUI versions. The changes include splitting the core library into platform-specific variants and introducing a shared dependency target. Review feedback identifies several redundant dependencies and preprocessor definitions in the new targets that should be removed to simplify the build graph.
src/Main.cc uses BUILD_TYPE (from OPENROAD_DEFINES at line 127). Under the IWYU principle that hzeller has been enforcing across //src/* — see the recent "Fix superfluous dependencies" and "IWYU: Fix some missing includes" commits — a target should declare what its sources actually use rather than rely on a transitive propagation chain. Add the defines directly to both cc_binary targets so each is self-contained at the attribute level. (This also closes the Gemini code-assist high-priority comment on PR The-OpenROAD-Project#10384 about the missing attribute, while staying consistent with the IWYU direction the rest of the codebase is moving in.) Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
Address hzeller's "hides more dependencies in variables" feedback by deleting the OPENROAD_LIB_SRCS and OPENROAD_LIB_COMMON_DEPS Starlark variables and inlining each list at the cc_library that owns it. The Bazel deps graph is unchanged. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
@hzeller Trying to respect all concerns here with DRY, IWYU, SOT and whatnot :-) I'm unsure that there's anything that will please man and bot both here... |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the build system by replacing the flag-based configuration for CLI and GUI variants with explicit separate targets, specifically introducing openroad-qt and openroad_lib_qt. Feedback focuses on reducing the resulting code duplication by refactoring shared attributes and dependencies into variables to ensure consistency. Additionally, it was noted that the @boost.stacktrace dependency is redundant in the library targets and should be removed to keep the dependency graph clean.
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@hzeller Any advice on how to please man and bot here? |
oharboe
changed the title
The earlier commits in this branch removed the `platform` string flag, which broke existing `bazelisk build --//:platform=gui //:openroad` muscle memory. Restore the flag and have `:openroad`'s deps select between `openroad_lib`/`openroad_lib_qt` (and the matching gui dep) based on it. `:openroad-qt` is unchanged and remains the recommended ad-hoc GUI build target because it does not invalidate `:openroad`'s cache. macOS CI also goes back to `--//:platform=gui //:openroad` so that path keeps getting exercised. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
/gemini review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bazel build configuration to support distinct CLI and Qt GUI variants of the OpenROAD binary and library. It introduces a new openroad-qt binary target and a corresponding openroad_lib_qt library, while also adding a build test to ensure the Qt variant remains compilable. Feedback focuses on reducing duplication by extracting common sources and dependencies into shared variables and maintaining consistency in inline comments.
|
@hzeller I updated openroad-qt to be an addition instead of replacement of the config option. We're on a deprecation trajectory for qt, so leaving current use-cases alone. Do you agree or disagree with Gemini here? |
|
(I still don't understand the premise of the 16h, it is only a few minutes to add the qt buld, maybe tens of minutes on an old machine) Can you make the same thing without duplicating all the things ? On the highlevel, to have a switch what is default built I am thinking of something like cc_library(name = "most_the_deps",
# no src, just a collect-the-deps library
deps = [ ... ]
)
# Very simple binaries, just a few lines
cc_binary(name = "openroad-headless",
srcs = ["Main.cc"],
deps = [ ":most_the_deps", ":deps_for_headless"],
)
cc_binary(name = "openroad-qt"
srcs = ["Main.cc"],
deps = [":most_the_deps", ":deps_for_qt_ui"]);
alias(
name = "openroad",
actual = select(
{
":platform_cli": [":openroad-headless"],
":platform_gui": [":openroad-qt"],
},
) |
|
@hzeller Updated use-case description at top. Working on a fix on the PR. |
Address hzeller's review of The-OpenROAD-Project#10384 — "Can you make the same thing without duplicating all the things?". Pull the deps both binaries share into a single OPENROAD_MAIN_DEPS list so each cc_binary block only enumerates the variant-specific lib + gui target. Tried hzeller's exact cc_library "most_the_deps" pattern first, but the package's layering_check feature rejects it: a deps-only cc_library does not re-export its deps' headers to consumers, so Main.cc's includes (ord/, utl/, sta/, web/, tcl_readline_setup.h) fail to resolve. A Starlark list is the practical equivalent here, matching the existing OPENROAD_LIBRARY_DEPS pattern in the same file. No rename of :openroad, no flag changes, no CI changes. Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the Bazel build configuration to separate the CLI and Qt GUI variants of OpenROAD into distinct targets, :openroad and :openroad-qt. It introduces shared dependency lists and a build test to ensure the Qt variant's compilability in CI. Feedback from the reviewer focuses on improving maintainability by using macros and variables to reduce duplication in the binary and library definitions.
| cc_binary( | ||
| name = "openroad-qt", | ||
| srcs = [ | ||
| "src/Main.cc", | ||
| ], | ||
| copts = OPENROAD_COPTS, | ||
| defines = OPENROAD_DEFINES, | ||
| features = ["-use_header_modules"], | ||
| malloc = select({ | ||
| "@platforms//os:linux": "@tcmalloc//tcmalloc", | ||
| "@platforms//os:macos": "@bazel_tools//tools/cpp:malloc", | ||
| }), | ||
| visibility = ["//visibility:public"], | ||
| deps = OPENROAD_MAIN_DEPS + [ | ||
| ":openroad_lib_qt", | ||
| "//src/gui:gui_qt", | ||
| ], | ||
| ) |
There was a problem hiding this comment.
There's significant duplication between the openroad and openroad-qt cc_binary targets. To improve maintainability, you could define a macro that captures the common attributes and then call it for each binary.
For example:
def openroad_binary(name, deps, **kwargs):
native.cc_binary(
name = name,
srcs = ["src/Main.cc"],
copts = OPENROAD_COPTS,
defines = OPENROAD_DEFINES,
features = ["-use_header_modules"],
malloc = select({
"@platforms//os:linux": "@tcmalloc//tcmalloc",
"@platforms//os:macos": "@bazel_tools//tools/cpp:malloc",
}),
visibility = ["//visibility:public"],
deps = deps,
**kwargs,
)
openroad_binary(
name = "openroad",
deps = OPENROAD_MAIN_DEPS + select({
":platform_cli": [
":openroad_lib",
"//src/gui",
],
":platform_gui": [
":openroad_lib_qt",
"//src/gui:gui_qt",
],
}),
)
openroad_binary(
name = "openroad-qt",
deps = OPENROAD_MAIN_DEPS + [
":openroad_lib_qt",
"//src/gui:gui_qt",
],
)This would require moving the binary definitions into a .bzl file or defining the macro at the top of this BUILD.bazel file.
| srcs = [ | ||
| "src/Design.cc", | ||
| "src/OpenRoad.cc", | ||
| "src/Tech.cc", | ||
| "src/Timing.cc", | ||
| ":openroad_swig", | ||
| ":openroad_tcl", | ||
| ], |
There was a problem hiding this comment.
To improve maintainability and avoid duplicating the list of source files, consider defining it in a variable and reusing it for both openroad_lib and openroad_lib_qt.
For example:
OPENROAD_LIB_SRCS = [
"src/Design.cc",
"src/OpenRoad.cc",
"src/Tech.cc",
"src/Timing.cc",
":openroad_swig",
":openroad_tcl",
]
cc_library(
name = "openroad_lib",
srcs = OPENROAD_LIB_SRCS,
...
)
cc_library(
name = "openroad_lib_qt",
srcs = OPENROAD_LIB_SRCS,
...
)
|
@hzeller Better? |
|
I asked @QuantamHD about the history of the flag; he remembers that @maliberty wanted to have this choice. Note, you need a third kind: the 'headless' one. This is the typical one that you'd run on a machine that does not hae all the X libraries or working Qt libs, but still want to emit a die-shot as *.png (which is what I always like to have). |
|
@hzeller Happy to add openroad-headless, but I do believe just "openroad" is intended to be headless @maliberty Ok? |
|
I believe the first version of qt in bazel didn't support all platforms so that flag was mandatory to not break them. We have the same option in cmake and I recall there was some demand for it but it has been a long time. I think the simplest thing is to just make the default with gui and leave the no-gui for history (moot once we have web). I started on that in #8532 but ran into problems and never got back to it. I think that is simpler than this approach. |
|
Note you can already save an image through web without needing qt at all fwiw. |
What about what @hzeller pointed out, then you need a openroad-headless to avoid all the X dependencies on a headless machine...? A bare bones CI environment with some light-weight distribution with bazel pulling in all dependencies is a really neat use-case.... |
|
Headless doesn't mean there are no X libraries, just that no display is available. @hzeller is this a real concern? |
|
Usually, there are no X libraries on servers and one wouldn't install them, so this is a real concern. Also, bazel tests will not work with qt enabled on a hermetic system (NixOS), as that attempts to reach out to the system for shared libraries, but bazel redacts the LD_LIBRARY_PATH. |
|
@maliberty @hzeller The immediate pain with qt is that it is hard to get an idempotent openroad binary across systems for CI. qt libraries are also nasty paper-cuts for nix+bare bones CI containers. We do have some nix users and qt is an example of the problems we've had rolling out nix more broadly. Also, we want to be able to hack the webgui without having to re-run ORFS flows or hack the build system, just:
=> rebuild only webgui and re-start GUI. |
3 participants
Add an openroad-qt binary for users who prefer that over config.
Leave
--//:platform=guiuse-case unchanged.openroad-qt fixes the following use-case, POLA:
bazelisk foo_finalfor 16 hours. This artifact is placed in a remote cache and it is built in CI.bazelisk --//:platform=gui run foo_final gui_finallocally to open design in GUI; this would take another 16 hours as the final artifact has to be rebuilt because the Qt GUI is now an accidental dependency of the build artifact.bazelisk run foo_final gui_finaluse openroad-qt rather than openroad, No need to re-build for 16 hours due to config change.webgui will eventually sunset Qt, but meanwhile we need a better way to handle this use-case.
I'm already ditching the config flag in bazel-orfs to avoid multi-rebuilds of openroad The-OpenROAD-Project/bazel-orfs#722