-
Notifications
You must be signed in to change notification settings - Fork 7.2k
feat: add support for building with Bazel #8875
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
|
All contributors have signed the CLA ✍️ ✅ |
…8879) To support Bazelification in #8875, this PR introduces a new `find_resource!` macro that we use in place of our existing logic in tests that looks for resources relative to the compile-time `CARGO_MANIFEST_DIR` env var. To make this work, we plan to add the following to all `rust_library()` and `rust_test()` Bazel rules in the project: ``` rustc_env = { "BAZEL_PACKAGE": native.package_name(), }, ``` Our new `find_resource!` macro reads this value via `option_env!("BAZEL_PACKAGE")` so that the Bazel package _of the code using `find_resource!`_ is injected into the code expanded from the macro. (If `find_resource()` were a function, then `option_env!("BAZEL_PACKAGE")` would always be `codex-rs/utils/cargo-bin`, which is not what we want.) Note we only consider the `BAZEL_PACKAGE` value when the `RUNFILES_DIR` environment variable is set at runtime, indicating that the test is being run by Bazel. In this case, we have to concatenate the runtime `RUNFILES_DIR` with the compile-time `BAZEL_PACKAGE` value to build the path to the resource. In testing this change, I discovered one funky edge case in `codex-rs/exec-server/tests/common/lib.rs` where we have to _normalize_ (but not canonicalize!) the result from `find_resource!` because the path contains a `common/..` component that does not exist on disk when the test is run under Bazel, so it must be semantically normalized using the [`path-absolutize`](https://crates.io/crates/path-absolutize) crate before it is passed to `dotslash fetch`. Because this new behavior may be non-obvious, this PR also updates `AGENTS.md` to make humans/Codex aware that this API is preferred.
8058b29 to
d79f369
Compare
9c0be78 to
9e7f454
Compare
This helps prepare us for Bazel builds: #8875.
ff84352 to
c0cb029
Compare
| @@ -1,6 +1,8 @@ | |||
| #![allow(clippy::unwrap_used)] | |||
|
|
|||
| use codex_apply_patch::APPLY_PATCH_TOOL_INSTRUCTIONS; | |||
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.
Will go away once #8914 is in.
938273f to
f4d2a47
Compare
…t_rule() test (#8931) Because the path to `git` is used to construct `elicitations_to_accept`, we need to ensure that we resolve which `git` to use the same way our Bash process will: https://github.com/openai/codex/blob/c9c65606852c0cda9d983b4917359a0826a4b7f0/codex-rs/exec-server/tests/suite/accept_elicitation.rs#L59-L69 This fixes an issue when running the test on macOS using Bazel (#8875) where the login shell chose `/opt/homebrew/bin/git` whereas the non-login shell chose `/usr/bin/git`.
086e308 to
72d3a0a
Compare
This seems to be necessary to get the Bazel builds on ARM Linux to go green on #8875. I don't feel great about timeout-whack-a-mole, but we're still learning here...
f71fc60 to
4428924
Compare
.github/workflows/Dockerfile.bazel
Outdated
| RUN apt-get update && \ | ||
| apt-get install -y --no-install-recommends \ | ||
| build-essential curl git python3 ca-certificates \ | ||
| pkg-config clang musl-tools libssl-dev just && \ |
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.
don't need libssl-dev, it comes from bazel and is built hermetically :)
f9dc9e8 to
aeb3607
Compare
…8879) To support Bazelification in #8875, this PR introduces a new `find_resource!` macro that we use in place of our existing logic in tests that looks for resources relative to the compile-time `CARGO_MANIFEST_DIR` env var. To make this work, we plan to add the following to all `rust_library()` and `rust_test()` Bazel rules in the project: ``` rustc_env = { "BAZEL_PACKAGE": native.package_name(), }, ``` Our new `find_resource!` macro reads this value via `option_env!("BAZEL_PACKAGE")` so that the Bazel package _of the code using `find_resource!`_ is injected into the code expanded from the macro. (If `find_resource()` were a function, then `option_env!("BAZEL_PACKAGE")` would always be `codex-rs/utils/cargo-bin`, which is not what we want.) Note we only consider the `BAZEL_PACKAGE` value when the `RUNFILES_DIR` environment variable is set at runtime, indicating that the test is being run by Bazel. In this case, we have to concatenate the runtime `RUNFILES_DIR` with the compile-time `BAZEL_PACKAGE` value to build the path to the resource. In testing this change, I discovered one funky edge case in `codex-rs/exec-server/tests/common/lib.rs` where we have to _normalize_ (but not canonicalize!) the result from `find_resource!` because the path contains a `common/..` component that does not exist on disk when the test is run under Bazel, so it must be semantically normalized using the [`path-absolutize`](https://crates.io/crates/path-absolutize) crate before it is passed to `dotslash fetch`. Because this new behavior may be non-obvious, this PR also updates `AGENTS.md` to make humans/Codex aware that this API is preferred.
c4a26a7 to
7bf83c1
Compare
.github/workflows/Dockerfile.bazel
Outdated
| RUN apt-get update && \ | ||
| apt-get install -y --no-install-recommends \ | ||
| build-essential curl git python3 ca-certificates \ | ||
| pkg-config clang musl-tools libssl-dev && \ |
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.
can we try to remove these?
- pkg-config
- clang
- musl-tools
- libssl-dev
- (maybe, not sure) build-essential?
sorry to be a pain, but clang in particular is huge and i want to make sure we have no compilers in the base image so they're not used by accident :)
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.
I mentally associate pkg-config with "python stuff," but chat says ends up being needed by some common Python packages that contain C extensions, so I agree that the builder host should not rely on those. Will try removing it.
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.
in our case it's likely used by rust build scripts to find C libs on the host to link against. But we don't want that, we want the libs provided hermetically through bazel, as otherwise cross-compilation won't work
0a80ecf to
775ea36
Compare
bolinfest
left a comment
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.
This is game-changing!
|
🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 🚢 |
0e01c8c to
83debeb
Compare
83debeb to
458e07b
Compare
This PR configures Codex CLI so it can be built with Bazel in addition to Cargo. The
.bazelrcincludes configuration so that remote builds can be done using BuildBuddy.If you are familiar with Bazel, things should work as you expect, e.g., run
bazel test //... --keep-goingto run all the tests in the repo, but we have also added some new aliases in thejustfilefor convenience:just bazel-testto run tests locallyjust bazel-remote-testto run tests remotely (currently, the remote build is for x86_64 Linux regardless of your host platform). Note we are currently seeing the following test failures in the remote build, so we still need to figure out what is happening here:just build-for-releaseto build release binaries for all platforms/architectures remotelyTo setup remote execution:
@openai.comemail address.)~/.bazelrc(add the linebuild --remote_header=x-buildbuddy-api-key=YOUR_KEY)--config=remotein yourbazelinvocations (or addcommon --config=remoteto your~/.bazelrc, or use thejustcommands)CI
In terms of CI, this PR introduces
.github/workflows/bazel.yml, which uses Bazel to run the tests locally on Mac and Linux GitHub runners (we are working on supporting Windows, but that is not ready yet). Note that the failures we are seeing injust bazel-remote-testdo not occur on these GitHub CI jobs, so everything in.github/workflows/bazel.ymlis green right now.The
bazel.ymluses extra config in.github/workflows/ci.bazelrcso that macOS CI jobs build remotely on Linux hosts (using thedocker://docker.io/mbolin491/codex-bazelDocker image declared in the rootBUILD.bazel) using cross-compilation to build the macOS artifacts. Then these artifacts are downloaded locally to GitHub's macOS runner so the tests can be executed natively. This is the relevant config that enables this:Because of the remote caching benefits we get from BuildBuddy, these new CI jobs can be extremely fast! For example, consider these two jobs that ran all the tests on Linux x86_64:
For now, we will continue to run both the Bazel and Cargo jobs for PRs, but once we add support for Windows and running Clippy, we should be able to cutover to using Bazel exclusively for PRs, which should still speed things up considerably. We will probably continue to run the Cargo jobs post-merge for commits that land on
mainas a sanity check.Release builds will also continue to be done by Cargo for now.
Earlier attempt at this PR: #8832
Earlier attempt to add support for Buck2, now abandoned: #8504