Conversation
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Bazel 8.4.0 and a new set of build helpers. The changes are extensive and well-structured, moving towards a more modern and less patched way of packaging Bazel in nixpkgs. My review focuses on ensuring the new build logic is correct and robust. I've found a few issues: a critical error in a helper script due to a variable mismatch, some high-severity issues in argument passing to Bazel and in the final package stripping configuration that would likely lead to build or runtime failures, and a medium-severity issue for improving the robustness of dependency pinning. Overall, this is a great contribution, and with these fixes, it should be in good shape.
|
@ofborg build bazel_8 |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Bazel 8.4.0 to nixpkgs, which is a significant update. The changes are well-structured, including new build support helpers, numerous patches to improve integration with Nix, and example packages. The approach to enable air-gapped builds and improve hermeticity is commendable. I've identified a couple of high-severity issues related to shell argument escaping that could cause build failures with specific inputs, and a medium-severity issue regarding inconsistent hash handling in the examples. Addressing these points will improve the robustness and maintainability of the new Bazel package and its helpers.
b75c219 to
2019770
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Bazel 8.4.0 into nixpkgs, along with a new set of build support helpers. The changes are extensive and well-structured, moving away from older bootstrapping methods and introducing modern Bazel features like bzlmod for a cleaner, more air-gapped build process. The use of patch files and dedicated helpers for repository caching and vendoring is a significant improvement. Overall, this is a high-quality contribution. I have one suggestion to improve the robustness of how build targets are passed to Bazel.
The setup is based on bazel 5,6,7 in nixpkgs but is significantly different codeline-wise. There's a bit less patching, all patches are via patch files and other small tweaks. ## Bazel 8 build With 8.4.0 bazel dist archive is enough for air-gapped build removing the need for FODs or `src-deps.json`, and in turn we no longer need existing `bazel` binary to bootstrap the build. `enableNixHacks` patch is removed, `bazel-examples` repo is used as `passthru.tests` (cpp, java, rust) via new `bazel_8/build-support` helpers. `sed` patches are converted to patch files, overall amount of patching is reduced, if there'd be need for more it can be added with links to upstream issues or patches, ideally with a regression `passthru.tests` item. Amount of patching may potentially be reduced further, this needs more feedback using this. Overall using Bazel on NixOS remains tricky, for non-packaged repos it may still be most practical to use FHS + extra hacks and patches, maybe even straight up using Bazelisk and upstream pre-built binaries in FHS. ## buildBazelPackage support `pkgs/build-support/build-bazel-package/default.nix` is fairly big with a set of tweaks that may not be necessary with recent Bazel or may have better alternatives. More work is needed to figure out compatibility with `bazel_8` or other things to unify with `bazel_8` helpers. ## bazel_8/build-support Here are fairly minimalistic helpers to aid building Bazel packages using Bazel 8 features. For now only testes on `bazel-examples` repo. `patching.nix`: just a helper to inject patches into Bazel external dependencies `bazelDerivation`: helps to compose Bazel commandline and consume `repository_cache`, `vendor_dir`, `registry` all related to external dependencies management `bazelPackage`: two-stage derivation similar to `buildBazelPackage`, having options for `repository_cache` (pure fetches) or `vendor_dir` (unpacked fetches, further patchable)
2019770 to
ebf9d44
Compare
|
8.4.1 is in, so this can be closed |
The setup is based on bazel 5,6,7 in nixpkgs but is significantly different codeline-wise. There's a bit less patching, all patches are via patch files and other small tweaks.
Bazel 8 build
With 8.4.0 bazel dist archive is enough for air-gapped build removing the need for FODs or
src-deps.json, and in turn we no longer need existingbazelbinary to bootstrap the build.enableNixHackspatch is removed,bazel-examplesrepo is used aspassthru.tests(cpp, java, rust) via newbazel_8/build-supporthelpers.sedpatches are converted to patch files, overall amount of patching is reduced, if there'd be need for more it can be added with links to upstream issues or patches, ideally with a regressionpassthru.testsitem.Amount of patching may potentially be reduced further, this needs more feedback using this.
Overall using Bazel on NixOS remains tricky, for non-packaged repos it may still be most practical to use FHS + extra hacks and patches, maybe even straight up using Bazelisk and upstream pre-built binaries in FHS.
buildBazelPackage support
pkgs/build-support/build-bazel-package/default.nixis fairly big with a set of tweaks that may not be necessary with recent Bazel or may have better alternatives. More work is needed to figure out compatibility withbazel_8or other things to unify withbazel_8helpers.bazel_8/build-support
Here are fairly minimalistic helpers to aid building Bazel packages using Bazel 8 features. For now only testes on
bazel-examplesrepo.patching.nix: just a helper to inject patches into Bazel external dependenciesbazelDerivation: helps to compose Bazel commandline and consumerepository_cache,vendor_dir,registryall related to external dependencies managementbazelPackage: two-stage derivation similar tobuildBazelPackage, having options forrepository_cache(pure fetches) orvendor_dir(unpacked fetches, further patchable)Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.