-
Notifications
You must be signed in to change notification settings - Fork 3
fix: build failure with bazel 8 #24
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
042628b to
624e319
Compare
CodSpeed Performance ReportMerging #24 will degrade performances by 39.19%Comparing Summary
Benchmarks breakdown
|
6700d5e to
5edaebc
Compare
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.
Pull Request Overview
This PR fixes a build failure with Bazel 8 by replacing the instrument-hooks git submodule with proper dependency fetching mechanisms. The root cause was that GitHub releases don't include submodule content, leading to build failures when trying to include non-existent files.
Key Changes:
- Replaced git submodule with
FetchContentfor CMake and a Bazel module extension - Removed recursive submodule cloning from CI workflows to catch this issue in the future
- Added testing across multiple Bazel versions (7.6.2, 8.4.2, and 9.0.0 pre-release)
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
core/instrument-hooks |
Removed the git submodule reference |
core/CMakeLists.txt |
Added FetchContent to dynamically fetch instrument-hooks dependency |
core/BUILD |
Removed local instrument_hooks library definition, now references external dependency |
MODULE.bazel |
Replaced git_override with module extension for instrument_hooks |
third_party/extensions.bzl |
Created Bazel extension to fetch instrument-hooks via git_repository |
third_party/instrument_hooks.BUILD |
Added BUILD file for instrument-hooks external dependency |
third_party/BUILD |
Created package to export third-party configuration files |
.github/workflows/ci.yml |
Removed recursive submodule checkout and added Bazel version matrix testing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
52c41d9 to
ec447a9
Compare
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.
olgtm, have we tested on workerd ?
Also, AFAI understand, ci did not fail because we were fetching with the submodule, so
--incompatible_disallow_empty_glob was not triggered right ?
7ee2167 to
e7f89ca
Compare
|
I recommend updating Bazel to the latest version as well (in bazel version file) |
It didn't probably catch it because bazel version used in this repository is 7, not 8. |
ecde422 to
5bccea4
Compare
Yeah, but we do have a .bazelrc rule to explicitly enable this error, because we had the issue in the past 🤔 |
ef11ae1 to
86049c6
Compare
86049c6 to
f615ab6
Compare
Root cause: The Github release doesn't contain the
instrument-hookssubmodule, which leads to errors when including a non-existent file. The same can happen for CMake.What changed:
FetchContent(CMake)EDIT: The bazel issue can be fixed using
git_repositoryinstead ofhttp_archive.