Conversation
ef3dddc to
86a0496
Compare
86a0496 to
b0bfdb5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
b0bfdb5 to
efaf838
Compare
This comment was marked as outdated.
This comment was marked as outdated.
efaf838 to
960a941
Compare
This comment was marked as outdated.
This comment was marked as outdated.
960a941 to
458f87c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
2840557 to
b6ecb11
Compare
ci/OWNERS
Outdated
There was a problem hiding this comment.
Let's not burden people with more pings unless they confirm being okay with it. @Profpatsch do you even still want to help maintain Bazel? If not, you should probably be removed from here.
And independent of whether @Profpatsch stays listed here, I'm suggesting you add yourself @boltzmannrain, since you've done so much great Bazel maintenance recently :D
|
Since PR created, bazel updated to https://github.com/bazelbuild/bazel/releases/tag/8.1.1 |
|
The description includes the following item:
Where does that occur within the current patch? |
Oh, perhaps you meant |
pkgs/by-name/ba/bazel_8/package.nix
Outdated
There was a problem hiding this comment.
I found that for what's been ailing me trying to build the Abseil library within Bazel on macOS, just including --host_features=-module_maps in my .bazelrc finally got it all to work.
Thank you so much, Dmitry!
d770743 to
4e41ee3
Compare
4e41ee3 to
a896868
Compare
|
Updated to 8.1.1, removed ci/OWNERS change, applied couple more build workarounds that showed up. |
|
Can it be merged before 8.2.0? |
|
philiptaron
left a comment
There was a problem hiding this comment.
I read through this PR as a committer who
a. doesn't use Bazel
b. doesn't know Java that well
c. is just looking to unblock what looks like a stuck PR
So please bear that in mind.
I have a couple big pieces of feedback that would have made it much easier to say "merge immediately".
- Splitting the mega-derivation into chunks, each of which was
callPackaged from the main one. That makes it possible for me to say "yeah, that checks out" in chunks, rather than trying to keep the whole 600+ Nix derivation in my head as I'm reading it. - Use of patches throughout, so that all of the
sedcalls were reviewable through seeing them in context. Yes, this will be bigger in terms of bytes. Trying to figure out what a multi-linesedcall is going to do is just super hard. - Use of the new Darwin SDK and interaction with recent developments there. More broadly, a lot of this derivation is dedicated to changing Bazel behavior on Darwin, and I found myself wondering whether it was overfitting on a stdenv that no longer is recommended.
There are lots of smaller things I commented on as a read through this PR, but those were the big ones.
I don't actually know if any of that is actually blocking, because as previously stated, I'm just here trying to unstick a PR without traction.
| makeWrapper, | ||
| writeTextFile, | ||
| writeScript, | ||
| substituteAll, |
| (substituteAll { | ||
| src = ./strict_action_env.patch; | ||
| strictActionEnvPatch = defaultShellPath; | ||
| }) |
There was a problem hiding this comment.
| (substituteAll { | |
| src = ./strict_action_env.patch; | |
| strictActionEnvPatch = defaultShellPath; | |
| }) | |
| (replaceVars ./strict_action_env.patch { | |
| strictActionEnvPatch = defaultShellPath; | |
| }) |
| (substituteAll { | ||
| src = ./bazel_rc.patch; | ||
| bazelSystemBazelRCPath = bazelRC; | ||
| }) |
There was a problem hiding this comment.
| (substituteAll { | |
| src = ./bazel_rc.patch; | |
| bazelSystemBazelRCPath = bazelRC; | |
| }) | |
| (replaceVars ./bazel_rc.patch { | |
| bazelSystemBazelRCPath = bazelRC; | |
| }) |
| bazel_self = bazel_7; | ||
| }; | ||
|
|
||
| bazel_8 = darwin.apple_sdk_11_0.callPackage ../by-name/ba/bazel_8/package.nix { |
There was a problem hiding this comment.
Is this needed now @NixOS/darwin-core? I think not...
| # Bootstrap an existing Bazel so we can vendor deps with vendor mode | ||
| bazelBootstrap = stdenv.mkDerivation rec { | ||
| name = "bazelBootstrap"; | ||
|
|
||
| src = | ||
| { | ||
| x86_64-linux = fetchurl { | ||
| url = "https://github.com/bazelbuild/bazel/releases/download/${bootstrapVersion}/bazel_nojdk-${bootstrapVersion}-linux-x86_64"; | ||
| hash = "sha256-KLejW8XcVQ3xD+kP9EGCRrODmHZwX7Sq3etdrVBNXHI="; | ||
| }; | ||
| aarch64-linux = fetchurl { | ||
| url = "https://github.com/bazelbuild/bazel/releases/download/${bootstrapVersion}/bazel_nojdk-${bootstrapVersion}-linux-arm64"; | ||
| hash = "sha256-+4GTKurt1KBkdyfC2hOLI31gBDOm1wTn91+kDX1di9M="; | ||
| }; | ||
| x86_64-darwin = fetchurl { | ||
| url = "https://github.com/bazelbuild/bazel/releases/download/${bootstrapVersion}/bazel-${bootstrapVersion}-darwin-x86_64"; | ||
| hash = "sha256-MsqLv4ZssUGQv7AZzhrEp9YbjLs/B3ETeXTo0OXN8es="; | ||
| }; | ||
| aarch64-darwin = fetchurl { | ||
| url = "https://github.com/bazelbuild/bazel/releases/download/${bootstrapVersion}/bazel-${bootstrapVersion}-darwin-arm64"; | ||
| hash = "sha256-0QrGSIVQxSEa7SAIT0Dft39jZ+IpsVoc8ocFeUHZMys="; | ||
| }; | ||
| } | ||
| .${stdenv.hostPlatform.system}; | ||
|
|
||
| nativeBuildInputs = defaultShellUtils; | ||
| buildInputs = [ | ||
| stdenv.cc.cc | ||
| ] ++ lib.optional (!stdenv.hostPlatform.isDarwin) autoPatchelfHook; | ||
|
|
||
| dontUnpack = true; | ||
| dontPatch = true; | ||
| dontBuild = true; | ||
| dontStrip = true; | ||
| installPhase = '' | ||
| runHook preInstall | ||
|
|
||
| mkdir -p $out/bin | ||
| install -Dm755 $src $out/bin/bazel | ||
|
|
||
| runHook postInstall | ||
| ''; | ||
|
|
||
| postFixup = '' | ||
| wrapProgram $out/bin/bazel \ | ||
| --prefix PATH : ${lib.makeBinPath nativeBuildInputs} | ||
| ''; | ||
|
|
||
| meta.sourceProvenance = with lib.sourceTypes; [ binaryNativeCode ]; | ||
| }; |
There was a problem hiding this comment.
Prefer to extract this to its own package:
bazelBootstrap = callPackage ./bootstrap.nix { inherit whatever };
| } | ||
|
|
||
| # Also build parser_deploy.jar with bootstrap bazel | ||
| # TODO: Turn into a proper patch |
There was a problem hiding this comment.
I'd really appreciate this. Sed is great for very targeted fixes, but this wide-scale rework is better as a patch, or a separate derivation.
| # reconstruct the now patched builtins_bzl.zip | ||
| pushd src/main/java/com/google/devtools/build/lib/bazel/rules/builtins_bzl_zip &>/dev/null | ||
| zip ../builtins_bzl.zip $(find builtins_bzl -type f) >/dev/null | ||
| rm -rf builtins_bzl |
There was a problem hiding this comment.
This made me think that making a separate derivation for builtins_bzl would work and clarify this mega derivation.
| # $out/bin/bazel-{version}-{os_arch} The binary _must_ exist with this | ||
| # naming if your project contains a .bazelversion file. | ||
| cp ./bazel_src/scripts/packages/bazel.sh $out/bin/bazel | ||
| versionned_bazel="$out/bin/bazel-${version}-${system}-${arch}" |
There was a problem hiding this comment.
| versionned_bazel="$out/bin/bazel-${version}-${system}-${arch}" | |
| versioned_bazel="$out/bin/bazel-${version}-${system}-${arch}" |
minor typo
| # The string literal specifying the path to the bazel-rc file is sometimes | ||
| # stored non-contiguously in the binary due to gcc optimisations, which leads | ||
| # Nix to miss the hash when scanning for dependencies | ||
| echo "${bazelRC}" >> $out/nix-support/depends |
| # TODO add some tests to cover basic functionality, and also tests for enableNixHacks=true (buildBazelPackage tests) | ||
| # tests = ... |
There was a problem hiding this comment.
|
Suggestions make sense to me 👍 But will need some time to implement. Also looks like 8.0.1->8.1.1 bump broke |
|
FWIW, I tested 8.1.1 from this PR on two of projects I maintain ( bant and verible), both of which are compatible with a wide range of bazel versions from 6.5.0 to 8.1.1.
So I can only use 8.1.1 from this PR on these projects if I force upgrading to a working bazel_dep(name = "rules_cc", version = "0.1.1")
git_override(
module_name = "rules_cc",
commit = "d74915024017250e46d95e91a3defc34174effe0", # get commit of rules_cc PR 306
remote = "https://github.com/bazelbuild/rules_cc",
)With that, both I wonder if in the 8.1.1 somewhere has its MODULE.bazel that loads some older |
|
Uh, looking through the bazel 8.1.1 source code to see what bazel[release-8.1.1]$ rg rules_cc.*version
src/MODULE.tools:bazel_dep(name = "rules_cc", version = "0.0.17")
src/test/py/bazel/bazel_windows_test.py: 'bazel_dep(name = "rules_cc", version = "0.0.12")',
src/test/py/bazel/launcher_test.py: 'bazel_dep(name = "rules_cc", version = "0.0.12")',
src/test/py/bazel/testdata/runfiles_test/MODULE.bazel.mock:bazel_dep(name = "rules_cc", version = "0.0.9")
src/test/py/bazel/bazel_windows_cpp_test.py: 'bazel_dep(name = "rules_cc", version = "0.0.12")',
site/en/external/module.md:bazel_dep(name = "rules_cc", version = "0.0.1")
site/en/external/overview.md:bazel_dep(name = "rules_cc", version = "0.0.1")
MODULE.bazel:bazel_dep(name = "rules_cc", version = "0.0.17")... not sure if my suggestion to upgrade |
I am convinced now this is not something we should work around in bazel 8. Bazel 8 actually just introduces a minimal So LGTM to me. |
|
Marking as draft for now, will probably create a new one after refactoring and making tests work |
|
Super excited about this PR. Thanks for the effort! |
|
Bazel 8.2.1 is much easier to bootstrap-build, so hope to open a new improved PR soon |
|
Draft for 8.2.1 #400941 I will be refactoring and cleaning it up a fair bit but big-picture is settled for now |
|
8.2.1 PR supersedes 8.1.1 |
Started with copying
bazel_7and adapted it, notable changes:bazelDepsnow builds on Darwin insandbox=truewhich was previously failing on power management tweakslndir -silentfor less build logsmodule_depsfeature to avoid getting.. does not depend on a module exporting ..errors on clang/macos importing intrinsics-fno-aligned-allocationon sdk <10.13 to avoid build error saying it isn't supported on <10.13pkgs/by-nameto pass CI, but still calling frompkgs/top-level/all-packages.nixas not sure how to pass darwin sdk and other settings in by-name wayThings done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.