-
Notifications
You must be signed in to change notification settings - Fork 633
Use Docker for Bazel Build #1232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
utils/bazel/.bazelrc
Outdated
build --action_env=CC=clang-10 | ||
build --action_env=CXX=clang-cpp-10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asaadaldien PTAL. Previously we'd decided to not send this upstream because clang on GHA VMs was already new, but with Ubuntu 18 container the default clang is old and fails with the AVX errors.
git \ | ||
python3-pip \ | ||
wget \ | ||
clang-10 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asaadaldien PTAL. Previously we'd decided to not send this upstream because clang on GHA VMs was already new, but with Ubuntu 18 container the default clang is old and fails with the AVX errors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this breaks CMake workflow because it is looking for clang
and not clang-10
:) Looking into a way to symbolic link so as to not update every -DCMAKE_C_COMPILER=clang
with -DCMAKE_C_COMPILER=clang-10
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this is what I need: https://man7.org/linux/man-pages/man1/update-alternatives.1.html (and we're already doing this for python).
RUN update-alternatives --install /usr/bin/clang clang /usr/bin/clang-10 10 | ||
RUN update-alternatives --install /usr/bin/clang++ clang++ /usr/bin/clang++-10 10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asaadaldien with this we can avoid having to configure both bazel and cmake builds to pick the specific version.
Do you have a hard requirement for Ubuntu 18.04 ? The reason we moved to 22.04 in the CI is to get newer tools and python version by default |
You could add Bazel as an option in #1234 and then anyone building other CIs / Releases can also test Bazel builds locally via docker. |
@powderluv we'd like to avoid coupling Bazel CI with the CMake release builds and merge gating CI because it is a very orthogonal flow with almost nothing in common (deps, build, test scripts etc). It adds to the complexity but with little to no ROI. We believe this is acceptable since users do not have to fix a broken bazel build (requiring them to run things locally), but if users want to help fix bazel builds, we'll provide easy-to-run scripts to repro bazel CI locally. Please LMK if you see any strong reasons against this. |
Closing as this was coupled with the earlier Dockerize CI PR (#1225 ). Will send a separate one out for dockerizing bazel CI. |
+1 to not coupling Bazel. With our current policy, it should definitely be kept as an orthogonal thing. |
* - Doxygen Java API calls - Fix onnx.ai/onnx-mlir page frame - Fix JNI wrapper input/output signature functions Signed-off-by: Gong Su <[email protected]> * Fix doxygen-docs-publish.yml Signed-off-by: Gong Su <[email protected]> Co-authored-by: Ettore Tiotto <[email protected]>
A follow-on to #1225 to launch bazel builds in GHA (as well as locally) inside a docker container. Requires #1225 to land first.
Review just the following files (non bazel diffs are split into #1225 for ease of review):
.github/workflows/bazelBuildAndTest.yml
build_tools/docker/run_bazel_build.sh
utils/bazel/.bazelrc