-
Notifications
You must be signed in to change notification settings - Fork 10
Migrate to Bzlmod, upgrade toolchains, and cleanup #280
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
base: main
Are you sure you want to change the base?
Conversation
willyzha
commented
Dec 20, 2025
- Migrate dependencies to MODULE.bazel; bump Bazel to 7.3.1, Go to 1.23.4.
- Update C++ to C++17/C11; configure LLVM and aspect_rules_lint.
- Fix build/tests in src/ate, proxy_buffer, and config/spm.
- Move patches to third_party/ and remove legacy repo config.
27a2921 to
a4de307
Compare
570eb38 to
c22f8ce
Compare
timothytrippel
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.
Nice work @willyzha !
Some improvements to make in a follow up / outside this PR:
- make sure we document how to update various dependency config files that are atuogenerated:
- python deps
- go deps
- file an issue on this repo to track the remaining work required to complete the bazel upgrade
| @@ -0,0 +1,48 @@ | |||
| diff --git a/extensions/bindgen/private/bindgen.bzl b/extensions/bindgen/private/bindgen.bzl | |||
| --- private/bindgen.bzl | |||
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.
nice job updating !
Should we have a README somewhere that describe how to update this patch?
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 broke up the patch into 3 separate patches and added git commit descriptions for each of them. There isn't really anything special other than checking out the rules_rust repo and making the changes and converting them to patches.
third_party/go/deps.bzl
Outdated
| # go_packages_ is an automatically generated macro that lists repositories | ||
| # corresponding to //:go.mod's requirements. | ||
| # | ||
| # Use `bazel run //:update-go-repos` to update it, after changing //:go.mod. |
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.
is this still how this file is updated?
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.
Yes
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 pushed a new commit to migrate the remaining go, docker and lint deps. Now this file gone and I updated comments go.mod to explain how to update the go deps.
| name = "lowrisc_opentitan", | ||
| ) | ||
|
|
||
| archive_override( |
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.
nit: should we group this next to the bazel_dep it is associated with?
or is this the result of buildifier autoformatting?
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.
Done
rules/repos.bzl
Outdated
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| package(default_visibility = ["//visibility:public"]) | ||
| """Custom repository rules for bzlmod migration.""" |
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.
just curious: is the goal to delete this file entirely when the bazel upgrade is completed?
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.
Deleted. It was already not needed since I imported the rule directly in the top level MODULE.bazel
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| # This is just a dummy config file to enable the third party @lowrisc_opentitan | ||
| # Bazel repo dependencies to build cleanly without having to close the repo. |
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 this file just be deleted entirely?
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.
same with the corresponding WORKSPACE.bazel file
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.
Done. Deleted
| ), | ||
| ] | ||
|
|
||
| windows_binary = rule( |
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.
just curious: is the windows binary build still tested in CI?
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.
does the CI run against the the pkg_win target at //src/ate:windows? I think that target should still be the same after this change
|
Can this be marked ready for review? |
2465e27 to
3aefffc
Compare
Thanks, I added a new commit that completes more of the migration, and filed issue #283 to track the remaining work after this pull request and to update and review the documentation. |
- Migrate dependencies to MODULE.bazel (LLVM, Python, Rust); bump Bazel to 7.3.1. - Update C++ to C++17/C11; configure LLVM and aspect_rules_lint. - Fix build/tests in src/ate, proxy_buffer, and config/spm. - Move patches to third_party/ and remove legacy repo config. Signed-off-by: Willy Zhang <[email protected]>
This change completes the Bzlmod migration for Google-hosted dependencies. Specific changes: - Move re2 and protobuf-matchers to MODULE.bazel. - Remove legacy third_party/google/repos.bzl. - Drop unused boringssl-windows-constraints.patch. Signed-off-by: Willy Zhang <[email protected]>
Migrate rules_foreign_cc and upb from WORKSPACE to MODULE.bazel. - Add rules_foreign_cc and upb (required by gRPC) to MODULE.bazel. - Remove legacy third_party/bazel macros and files. - Remove protocol_compiler bind from WORKSPACE. Signed-off-by: Willy Zhang <[email protected]>
- Move softhsm2 definition from WORKSPACE to MODULE.bazel. - Update src/pk11/test_support.go to use bazel.Runfile for locating softhsm2-util. - Remove third_party/softhsm2/repos.bzl. Signed-off-by: Willy Zhang <[email protected]>
- Move lowrisc repository definitions from WORKSPACE to MODULE.bazel. - Remove legacy third_party/lowrisc/repos.bzl. - Update documentation to reference MODULE.bazel. Signed-off-by: Willy Zhang <[email protected]>
- Remove unused third_party/protobuf/BUILD.bazel. - Remove third_party/protobuf/grpc-go-toolchain.patch. - Remove third_party/protobuf/grpc-windows-constraints.patch. - Remove third_party/protobuf/repos.bzl. - Remove third_party/google/BUILD.bazel. Signed-off-by: Willy Zhang <[email protected]>
Signed-off-by: Willy Zhang <[email protected]>
This change updates the `rules_rust_bindgen` patching strategy to support building on modern Linux hosts that enforce PIE. Split `bindgen_static_lib.patch` into three focused patches: - `bindgen_remove_static_check.patch`: Supports header-only deps. - `bindgen_prioritize_pic.patch`: Prioritizes PIC libs for host PIE builds. - `bindgen_fix_resource_dir.patch`: Fixes Clang resource dir detection. Signed-off-by: Willy Zhang <[email protected]>
This commit completes the Bzlmod migration for core dependencies, Go toolchains, and Docker rules. It includes the following key changes: - Migrated `rules_docker` and `rules_go` dependencies to MODULE.bazel. - Updated `rules/vendor.bzl` to use `default_vendor_label` instead of `dummy_label` for clarity. - Added `third_party/docker/rules_docker_fix.patch` to fix `image_transition` platform constraints (forcing x86_64/linux). - Added `third_party/lowrisc/opentitan_llvm.patch` to remove conflicting LLVM toolchain configurations. - Updated `third_party/docker` and `third_party/lint` to use module extensions.
11beee8 to
2a2903e
Compare