Add -bazel and -bazel-dev options to DependencyInstaller and Build scripts#9762
Conversation
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request introduces Bazel support for building and dependency installation. The changes to Build.sh and DependencyInstaller.sh add new flags (-bazel, -bazel-dev) to use Bazel. My review focuses on improving the portability and robustness of these new scripts. I've suggested adding support for macOS in the dependency installer and improving shell script practices by removing an unnecessary eval. I also noted a point for future improvement regarding package manager support for GUI dependencies.
| _install_bazel() { | ||
| local bazel_prefix=${PREFIX:-"/usr/local"} | ||
| log "Checking Bazel (via bazelisk)" | ||
| if _command_exists "bazelisk"; then | ||
| log "bazelisk already installed, skipping." | ||
| INSTALL_SUMMARY+=("Bazel: system=found, required=any, status=skipped") | ||
| return | ||
| fi | ||
| local arch | ||
| arch=$(uname -m) | ||
| local bazelisk_arch="amd64" | ||
| if [[ "${arch}" == "aarch64" ]]; then | ||
| bazelisk_arch="arm64" | ||
| fi | ||
| ( | ||
| cd "${BASE_DIR}" | ||
| _execute "Downloading bazelisk..." curl -Lo bazelisk \ | ||
| "https://github.com/bazelbuild/bazelisk/releases/latest/download/bazelisk-linux-${bazelisk_arch}" | ||
| chmod +x bazelisk | ||
| _execute "Installing bazelisk..." mv bazelisk "${bazel_prefix}/bin/bazelisk" | ||
| ) | ||
| # Install xcb libraries needed for GUI support with Bazel builds | ||
| if _command_exists "apt-get"; then | ||
| _execute "Installing xcb libraries for GUI support..." \ | ||
| apt-get -y install --no-install-recommends \ | ||
| libxcb1-dev libxcb-util-dev libxcb-icccm4-dev libxcb-image0-dev \ | ||
| libxcb-keysyms1-dev libxcb-randr0-dev libxcb-render-util0-dev \ | ||
| libxcb-xinerama0-dev libxcb-xkb-dev | ||
| fi | ||
| INSTALL_SUMMARY+=("Bazel: system=none, required=latest, status=installed") | ||
| } |
There was a problem hiding this comment.
This function currently only supports installing bazelisk on Linux. On macOS, it will fail because it tries to download a Linux binary. Please add support for macOS, for example by using Homebrew (brew install bazelisk).
_install_bazel() {
local bazel_prefix=${PREFIX:-/usr/local}
log "Checking Bazel (via bazelisk)"
if _command_exists "bazelisk"; then
log "bazelisk already installed, skipping."
INSTALL_SUMMARY+=("Bazel: system=found, required=any, status=skipped")
return
fi
if [[ "$(uname -s)" == "Darwin" ]]; then
if ! _command_exists "brew"; then
error "Homebrew not found, which is required to install bazelisk on macOS."
fi
_execute "Installing bazelisk..." brew install bazelisk
else
local arch
arch=$(uname -m)
local bazelisk_arch="amd64"
if [[ "${arch}" == "aarch64" ]]; then
bazelisk_arch="arm64"
fi
(
cd "${BASE_DIR}"
_execute "Downloading bazelisk..." curl -Lo bazelisk \
"https://github.com/bazelbuild/bazelisk/releases/latest/download/bazelisk-linux-${bazelisk_arch}"
chmod +x bazelisk
_execute "Installing bazelisk..." mv bazelisk "${bazel_prefix}/bin/bazelisk"
)
# Install xcb libraries needed for GUI support with Bazel builds
if _command_exists "apt-get"; then
_execute "Installing xcb libraries for GUI support..." \
apt-get -y install --no-install-recommends \
libxcb1-dev libxcb-util-dev libxcb-icccm4-dev libxcb-image0-dev \
libxcb-keysyms1-dev libxcb-randr0-dev libxcb-render-util0-dev \
libxcb-xinerama0-dev libxcb-xkb-dev
fi
fi
INSTALL_SUMMARY+=("Bazel: system=none, required=latest, status=installed")
}| _install_bazel_dev() { | ||
| local bazel_prefix=${PREFIX:-"/usr/local"} | ||
| log "Checking Bazel dev tools (buildifier)" | ||
| if _command_exists "buildifier"; then | ||
| log "buildifier already installed, skipping." | ||
| INSTALL_SUMMARY+=("buildifier: system=found, required=any, status=skipped") | ||
| return | ||
| fi | ||
| local arch | ||
| arch=$(uname -m) | ||
| local buildifier_arch="amd64" | ||
| if [[ "${arch}" == "aarch64" ]]; then | ||
| buildifier_arch="arm64" | ||
| fi | ||
| ( | ||
| cd "${BASE_DIR}" | ||
| _execute "Downloading buildifier..." curl -Lo buildifier \ | ||
| "https://github.com/bazelbuild/buildtools/releases/latest/download/buildifier-linux-${buildifier_arch}" | ||
| chmod +x buildifier | ||
| _execute "Installing buildifier..." mv buildifier "${bazel_prefix}/bin/buildifier" | ||
| ) | ||
| INSTALL_SUMMARY+=("buildifier: system=none, required=latest, status=installed") | ||
| } |
There was a problem hiding this comment.
Similar to _install_bazel, this function only supports Linux. Please add support for macOS. buildifier can be installed with Homebrew (brew install buildifier).
_install_bazel_dev() {
local bazel_prefix=${PREFIX:-/usr/local}
log "Checking Bazel dev tools (buildifier)"
if _command_exists "buildifier"; then
log "buildifier already installed, skipping."
INSTALL_SUMMARY+=("buildifier: system=found, required=any, status=skipped")
return
fi
if [[ "$(uname -s)" == "Darwin" ]]; then
if ! _command_exists "brew"; then
error "Homebrew not found, which is required to install buildifier on macOS."
fi
_execute "Installing buildifier..." brew install buildifier
else
local arch
arch=$(uname -m)
local buildifier_arch="amd64"
if [[ "${arch}" == "aarch64" ]]; then
buildifier_arch="arm64"
fi
(
cd "${BASE_DIR}"
_execute "Downloading buildifier..." curl -Lo buildifier \
"https://github.com/bazelbuild/buildtools/releases/latest/download/buildifier-linux-${buildifier_arch}"
chmod +x buildifier
_execute "Installing buildifier..." mv buildifier "${bazel_prefix}/bin/buildifier"
)
fi
INSTALL_SUMMARY+=("buildifier: system=none, required=latest, status=installed")
}| if command -v bazelisk &> /dev/null; then | ||
| bazel_cmd="bazelisk" | ||
| fi | ||
| eval "${bazel_cmd}" build //:openroad |
| if _command_exists "apt-get"; then | ||
| _execute "Installing xcb libraries for GUI support..." \ | ||
| apt-get -y install --no-install-recommends \ | ||
| libxcb1-dev libxcb-util-dev libxcb-icccm4-dev libxcb-image0-dev \ | ||
| libxcb-keysyms1-dev libxcb-randr0-dev libxcb-render-util0-dev \ | ||
| libxcb-xinerama0-dev libxcb-xkb-dev | ||
| fi |
9a72b14 to
c2d59d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
7739166 to
c2d59d2
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @maliberty the https://jenkins.openroad.tools/job/OpenROAD-Public/job/PR-9762-merge/4/display/redirect is failing at build 4 as I checked but has passed in build 1, 2 and 3 . I have no idea why is it so . Could you please look into this when you have time. Thank you. |
| if command -v bazelisk &> /dev/null; then | ||
| bazel_cmd="bazelisk" | ||
| fi | ||
| "${bazel_cmd}" build //:openroad |
There was a problem hiding this comment.
this should honor the GUI flag and numThreads, and should build release or opt
|
I think this is a bad idea. We want to ultimately phase out DependencyInstaller and install only bazel and let it pull in dependencies. DependencyInstaller should be kept as is for the CMake use-case, IMO. |
|
@oharboe I think this will provide for a smoother transition and if bazel can handle it all then maybe it can be retired in the future. (although we still need to install bazel/bazelisk and buildifier, so something is needed to get folks onboarded quickly and easily). |
| cd "${BASE_DIR}" | ||
| _execute "Downloading bazelisk..." curl -Lo bazelisk \ | ||
| "https://github.com/bazelbuild/bazelisk/releases/latest/download/bazelisk-linux-${bazelisk_arch}" | ||
| chmod +x bazelisk |
There was a problem hiding this comment.
need to verify the checksum _verify_checksum
| cd "${BASE_DIR}" | ||
| _execute "Downloading buildifier..." curl -Lo buildifier \ | ||
| "https://github.com/bazelbuild/buildtools/releases/latest/download/buildifier-linux-${buildifier_arch}" | ||
| chmod +x buildifier |
There was a problem hiding this comment.
need to verify the checksum _verify_checksum
| error "You must use one of: -all, -base, -common, -bazel, or -bazel-dev." | ||
| fi | ||
|
|
||
| if [[ "${INSTALL_BAZEL}" == "yes" ]]; then |
There was a problem hiding this comment.
probably should include bazel if doing -bazel-dev
"|| ${INSTALL_BAZEL_DEV}" == "yes"
|
Being a user of bazel, what I would be more interested in is:
This would probably require framework inverting DependencyInstaller so that it calls one decomposed build per tool. Claude should do that automatically. |
|
as for dev dependencies and linters and such, I think a better path is to add e.g. lint targets and let those lint targets handle dependencies in a bazel idiomatic fashion #9734 |
|
in that same vein check docs with bazel #9733 |
|
I think that this PR rightly identifies some real pain-points that I have myself. |
|
@alokkumardalei-wq @gadfort Thougths? The two PRs below build on the findings in this PR and should addresses the concerns: |
|
@alokkumardalei-wq Did you see my comments? #9762 (comment) |
|
clang-tidy review says "All clean, LGTM! 👍" |
83b0442 to
d853468
Compare
|
Thanks for the feedback! I have updated the PR to address @gadfort 's review comments: I added checksum verification for the tool downloads, made -bazel-dev automatically include the base -bazel tools, and updated Build.sh to respect the -no-gui and -threads flags while defaulting to -c opt." |
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Thanks @oharboe for your suggestions, I see the architectural point about keeping DependencyInstaller.sh purely for CMake and moving Bazel dependency management entirely into bazel run targets (as proposed in PR #9772). Since @gadfort suggested this PR could serve as a useful transition step, I am open to whatever the core maintainers decide: either treating this as a temporary bridge while the Bazel-native targets mature, or closing this in favor of PR #9772. Thank you! |
|
@alokkumardalei-wq Please also read The-OpenROAD-Project/OpenROAD-flow-scripts#4003 and let me know what your thoughts are this PR in light of that PR getting accepted. |
|
Thanks @oharboe for linking that! I just reviewed OpenROAD-flow-scripts#4003. Yes, adding bazelisk run //:install to ORFS directly addresses the original paint-point I raised in #9594. It's a much cleaner, Bazel-native approach than trying to overload DependencyInstaller.sh and Build.sh. Thank you! |
|
@alokkumardalei-wq Is #9772 needed? I thought we were statically linked. Please advice if it should be re-opened with some specific instructions on what exactly it should test for. |
|
@alokkumardalei-wq @oharboe please keep in mind that not all users are using ORFS and we should not expecting that of users and developers. |
|
clang-tidy review says "All clean, LGTM! 👍" |
1 similar comment
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @maliberty Thanks for the feedback. I have resolved that as per suggestion please have a look when you have time and I would be happy to address any further feedback. You can find this in this commit: 0b2a66e Thank you! |
|
clang-tidy review says "All clean, LGTM! 👍" |
…ld scripts Fixes The-OpenROAD-Project#9594. Adds Bazel setup options to make it easier for developers to get started with the Bazel build. DependencyInstaller.sh: -bazel — installs bazelisk + xcb libs for GUI support -bazel-dev — installs buildifier for formatting BUILD/.bzl files (implies -bazel) Both flags work standalone or combined with -all/-base/-common. Supports Linux (apt/yum) and macOS (Homebrew). Downloads are verified with _verify_checksum. Build.sh: -bazel — builds with Bazel instead of CMake, using bazelisk if available. Honors -no-gui (--//:platform=cli) and uses --config=opt with -j for thread count. Also refactors cmakeOptions from a string to a Bash array to safely handle quoted strings without eval, as suggested in review. Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
cc0ab79 to
fce2187
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: Alok Kumar Dalei <alokkumardalei2@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Errors still need fixing, eg |
e8ea5ba to
b135cc8
Compare
The isNinja branch was never reachable (no code path sets it) and under 'set -euo pipefail' it terminated the script with 'isNinja: unbound variable' at line 358 once master's removal of the ninja option (commit 4e494df) was merged in. Remove the dead block. Reported by maliberty in The-OpenROAD-Project#9762. Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
b135cc8 to
e89327a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @maliberty fixed in e89327a ,dropped the dead isNinja block. It became unreachable when master removed the Please have a look when you get time . Thank you ! |
|
Hello @maliberty ,all checks are passing now ,please have a look when you get time . Thank you ! |
| chmod +x bazelisk | ||
| _execute "Installing bazelisk..." mv bazelisk "${bazel_prefix}/bin/bazelisk" | ||
| ) | ||
| # Install xcb libraries needed for GUI support with Bazel builds |
|
@maliberty done in 6464b7d , added a if [[ "${NO_GUI}" != "yes" ]]; then
# Install xcb libraries needed for GUI support with Bazel builds
if _command_exists "apt-get"; then
...
elif _command_exists "yum"; then
...
fi
fiKept the scope narrow to the xcb block per your comment. The Qt5 packages elsewhere in the script (_installCommonDev and per-distro blocks) have the same property but I didn't touch them,happy to extend -no-gui to gate those too if you'd like, in this PR or a follow-up. |
|
clang-tidy review says "All clean, LGTM! 👍" |
Signed-off-by: alokkumardalei-wq <alokkumardalei2@gmail.com>
6464b7d to
be6ca9a
Compare
|
clang-tidy review says "All clean, LGTM! 👍" |
|
Hello @maliberty could you please have a look on failing ci https://github.com/The-OpenROAD-Project/OpenROAD/actions/runs/25043425809/job/73352197611?pr=9762 and this https://jenkins.openroad.tools/job/OpenROAD-Public/job/PR-9762-merge/53/display/redirect Thank you ! |
|
@gadfort approve or ? |
What does this PR solves?
Fixes #9594
Adds Bazel setup options to DependencyInstaller.sh and Build.sh to make it easier for developers to get started with the Bazel build.
Changes
DependencyInstaller.sh:
Both flags work standalone or combined with -all/-base/-common
Build.sh:
Testing/Verification:
Expected output:
Expected output: