Skip to content
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

bazel_7: init at 7.0.0 #262152

Merged
merged 40 commits into from
Jan 1, 2024
Merged

bazel_7: init at 7.0.0 #262152

merged 40 commits into from
Jan 1, 2024

Conversation

layus
Copy link
Member

@layus layus commented Oct 19, 2023

Description of changes

Introduce bazel 7

What works ?

  • building a functional bazel binary and most tests for
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Extracting MODULE.bazel.lock into a repository_cache / bindir nix derivation 🎉

What does not work ?

  • Trimming bazel external repos required for tests
  • Protobuf tests (full rewrite I think)
  • MODULE.lock generation is broken, a GSON issue I guess.
  • Java toolchain is hardcoded using aggressive system bazelrc.
  • cleanup of useless files
  • Comment every piece for future me/us
  • (re)Implement updater
  • Split the bootstrap steps into different derivations (I have loost too much time failing in the last ten seconds after 10 minutes of compiling)

More details in a follow-up comment, but here is a PR with two weeks of work, to avoid duplicating work.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Oct 19, 2023
@layus layus changed the title bazel_7: init at 7.0.0-pre.20230917.3 bazel_7: init at 7.0.0rc2 Oct 30, 2023
@layus layus marked this pull request as ready for review October 30, 2023 09:10
@layus layus requested a review from Profpatsch as a code owner October 30, 2023 09:10
@layus
Copy link
Member Author

layus commented Oct 30, 2023

This is now ready for review. Any insight welcome.

I did test it with

nix-build -E'(import ./. { system = "'{{x86_64,aarch64}-darwin,x86_64-linux}'"; })'.bazel_{4,5,6,7}.tests

And while bazel_6 fails to build on darwin, I consider this pretty unrelated, as it fails to find codesign, so probably a sandbox / patching issue.

Tests do pass on linux for all bazel versions, and on darwin for bazel_7.

@layus layus self-assigned this Oct 30, 2023
Copy link
Contributor

@uri-canva uri-canva left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work! I'm afraid the nix functions are a bit beyond my skills, I can understand them and I believe they're correct but I don't know if there's ways to make them better / more idiomatic. Hopefully someone else can take a look as well, but either way I don't think there's any blockers there.

@boltzmannrain
Copy link
Contributor

aarch64-linux compiles too here 👍

@layus
Copy link
Member Author

layus commented Nov 17, 2023

I know I am long overdue here, but there are still some pending issues, and we are very close to bazel 7.0.0 official release. I will try to release that directly.

@boltzmannrain
Copy link
Contributor

If merged with latest master it starts to fail on darwin like so
x86_64-darwin:

...
ld: warning: dylib (/nix/store/x4s0wi6d2z38wa0jb4yf769sl1c0c50z-libcxx-16.0.6/lib/libc++.dylib) was built for newer macOS version (10.12) than being linked (10.11)
Undefined symbols for architecture x86_64:
  "vtable for std::out_of_range", referenced from:
      std::out_of_range::out_of_range[abi:v160006](char const*) in libijar_cc_binary_main.lo(classfile.o)
  NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
  "___cxa_end_catch", referenced from:
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char>>::overflow(int) in libijar_cc_binary_main.lo(classfile.o)
      std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned long) in libijar_cc_binary_main.lo(classfile.o)
...

aarch64-darwin:

...
Undefined symbols for architecture arm64:
  "vtable for std::out_of_range", referenced from:
      std::out_of_range::out_of_range[abi:v160006](char const*) in libijar_cc_binary_main.lo(classfile.o)
  NOTE: a missing vtable usually means the first non-inline virtual member function has no definition.
  "___cxa_end_catch", referenced from:
      std::__1::basic_stringbuf<char, std::__1::char_traits<char>, std::__1::allocator<char>>::overflow(int) in libijar_cc_binary_main.lo(classfile.o)
      std::__1::basic_ostream<char, std::__1::char_traits<char>>& std::__1::__put_character_sequence[abi:v160006]<char, std::__1::char_traits<char>>(std::__1::basic_ostream<char, std::__1::char_traits<char>>&, char const*, unsigned long) in libijar_cc_binary_main.lo(classfile.o)
  "typeinfo for std::bad_array_new_length", referenced from:
      std::__throw_bad_array_new_length[abi:v160006]() in libijar_cc_binary_main.lo(classfile.o)
...

@layus
Copy link
Member Author

layus commented Dec 9, 2023 via email

@aaronmondal
Copy link
Contributor

7.0.0 was released: https://github.com/bazelbuild/bazel/releases

@boltzmannrain
Copy link
Contributor

Looks like 7.x darwin build breaks after #263535

@boltzmannrain
Copy link
Contributor

boltzmannrain commented Dec 12, 2023

INFO: ToolchainResolution: Target platform @@local_config_platform//:host: Selected execution platform //:default_host_platform, type @@bazel_tools//tools/cpp:toolchain_type -> toolchain @@bazel_tools~cc_configure_extension~local_config_cc//:cc-compiler-darwin_arm64, type @@bazel_tools//tools/jdk:toolchain_type -> toolchain @@bazel_tools//tools/jdk:nonprebuilt_toolchain_java17, type @@bazel_tools//tools/jdk:runtime_toolchain_type -> toolchain @@rules_java~7.1.0~toolchains~local_jdk//:jdk
...
 (cd /private/tmp/nix-build-bazel-7.0.0.drv-3/bazel_eVl8TlHS/out/execroot/_main && \
  exec env - \
    PATH=/nix/store/ai4hd8f1xhr0rfjdr17bxx1rwi42sx97-python3-3.11.6/bin
/nix/store/bg77k830j2z1xjslll7d31y55d3hnmip-unzip-6.0/bin
/nix/store/79yyc6yisbxkcpgavm2c756ysy5r3jbm-which-2.21/bin
/nix/store/pwhi3qhn5fi7m64hxyn2wkgy3b4l91zi-zip-3.0/bin
/nix/store/xd8i1lilimkhn4zcxyd3icddzb7r088z-cctools-port-973.0.1/bin
/nix/store/3fpl3hs6ii820yxp0rbafx5phnmj8ng7-clang-wrapper-16.0.6/bin
/nix/store/a5v30qll5i02vr9y97bk1rdx3mm6kvlm-clang-16.0.6/bin
/nix/store/h0fgpyxfav7ybnw5sdg8jpyv9f95x4w0-coreutils-9.3/bin
/nix/store/ydcx9rj4gf3dxm1kc0lx7l5d5vrjdwp2-cctools-binutils-darwin-wrapper-16.0.6-973.0.1/bin
/nix/store/vpnp0c420cjmx82g05jq3nm5skdrblvk-cctools-binutils-darwin-16.0.6-973.0.1/bin
/nix/store/k5xqpf63xm8m0v8m0bsb34w4w4l7yids-zulu17.44.53-ca-jdk-17.0.8.1/bin
/nix/store/x1xxsh1gp6y389hyl40a0i74dkxiprl7-bash-5.2-p15/bin
/nix/store/94j70ziq1zp2mj0fs2fzi49j7q3mdhba-diffutils-3.10/bin
/nix/store/ckbx2qla8zxic5k2d549qdhkbwrpa4q6-file-5.45/bin
/nix/store/zr6klxfjzpdr2674ly1f4fix7ig57mjr-findutils-4.9.0/bin
/nix/store/blggp459hvq6swvr8nlxblkdn7ayw8y7-gawk-5.2.2/bin
/nix/store/98v0v3bwzgkcwsz4anjip62d2k3gz352-gnugrep-3.11/bin
/nix/store/zapwwg8vnc47j7ciyrd7hb7rhwfrrs5c-patch-2.7.6/bin
/nix/store/nddzbf5pifm77kksh25xwiy2g9kqqm9f-gnused-4.9/bin
/nix/store/g9z0hg6snhg5834p14yw4bx6ilcffr8b-gnutar-1.35/bin
/nix/store/6g3nq5hply5p2v3xzlg0i9qqgzr30w1d-gzip-1.13/bin
/nix/store/h0fgpyxfav7ybnw5sdg8jpyv9f95x4w0-coreutils-9.3/bin
/nix/store/zr6klxfjzpdr2674ly1f4fix7ig57mjr-findutils-4.9.0/bin
/nix/store/94j70ziq1zp2mj0fs2fzi49j7q3mdhba-diffutils-3.10/bin
/nix/store/nddzbf5pifm77kksh25xwiy2g9kqqm9f-gnused-4.9/bin
/nix/store/98v0v3bwzgkcwsz4anjip62d2k3gz352-gnugrep-3.11/bin
/nix/store/blggp459hvq6swvr8nlxblkdn7ayw8y7-gawk-5.2.2/bin
/nix/store/g9z0hg6snhg5834p14yw4bx6ilcffr8b-gnutar-1.35/bin
/nix/store/6g3nq5hply5p2v3xzlg0i9qqgzr30w1d-gzip-1.13/bin
/nix/store/dq6dsprg57r5wqlc4yzcnkbfxdhfcd3f-bzip2-1.0.8-bin/bin
/nix/store/6kxdgmbgwdq4y42r9qhgdky312v5d7x0-gnumake-4.4.1/bin
/nix/store/x1xxsh1gp6y389hyl40a0i74dkxiprl7-bash-5.2-p15/bin
/nix/store/j34fimsig5s1zsf2f5dkw9a8sv5zpskg-patch-2.7.6/bin
/nix/store/96bznf6bjdghshgj3a2hm16gk3nngbcx-xz-5.4.4-bin/bin
/nix/store/ckbx2qla8zxic5k2d549qdhkbwrpa4q6-file-5.45/bin \
    PWD=/proc/self/cwd \
    ZERO_AR_DATE=1 \
  external/bazel_tools~cc_configure_extension~local_config_cc/cc_wrapper.sh @bazel-out/darwin_arm64-opt-exec-ST-fad1763555eb/bin/external/rules_java~7.1.0~toolchains~remote_java_tools/ijar_cc_binary-2.params)
# Configuration: ab192274ab21bdc5bb8e345e4cc8bd36536328aa117b8df888873da443d114d6
# Execution platform: //:default_host_platform
Undefined symbols for architecture arm64:
  "vtable for std::out_of_range", referenced from:
      std::out_of_range::out_of_range[abi:v160006](char const*) in libijar_cc_binary_main.lo(classfile.o)
...  

It looks like there's some instability around libcxx and apple sdk version that is going to be addressed in master (after 23.11 release, so ~now). But not exactly sure what goes wrong here

@dmivankov
Copy link
Contributor

Seems to be #166205
And might be fixable with something like 51451ac

Copy link
Contributor

@malt3 malt3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind testing it for remote execution.

This backport looks good to me and appears to work fine. Thank you!

@avdv avdv mentioned this pull request Dec 20, 2023
13 tasks
@name-snrl
Copy link
Contributor

name-snrl commented Dec 20, 2023

@layus Hello, when I tried to use bazel-repository-cache.nix without passing the requiredDepNamePredicate argument, it threw an error that the value is a set, while a string was expected. The problem is in _main~bazel_bazel_build_deps~workspace_repo_cache, because sha256 here is an object with the pair archive -> hash, instead of str.

I don't know how to fix this in a better way or if it should be done. But I figured it should be reported.

@layus
Copy link
Member Author

layus commented Dec 31, 2023

@name-snrl I have fixed that use case, because I needed the files it bundled. I cannot however support all repository_rule attributes, so the script should be made more robust to failure. I have improved that a bit.

@layus
Copy link
Member Author

layus commented Dec 31, 2023

Okay, so now all tests pass 🎉

As long as ofborg is happy, I am going to merge (or let anyone else merge).

@ofborg ofborg bot requested a review from uri-canva December 31, 2023 15:45
@uri-canva
Copy link
Contributor

Awesome! Looks good!

@uri-canva uri-canva merged commit dc594fa into NixOS:master Jan 1, 2024
21 of 22 checks passed
Copy link
Contributor

github-actions bot commented Jan 2, 2024

Successfully created backport PR for release-23.11:

@timothyklim
Copy link
Contributor

Is there any reason why remote_java_tools_linux not patcheelf'ed?

Use --sandbox_debug to see verbose messages from the sandbox and retain the sandbox build root for debugging
src/main/tools/linux-sandbox-pid1.cc:530: "execvp(external/remote_java_tools_linux/java_tools/ijar/ijar, 0x1a6db30)": No such file or directory

@layus
Copy link
Member Author

layus commented Jan 3, 2024 via email

@Strum355
Copy link
Contributor

Strum355 commented Jan 5, 2024

Is bazel7 in nixpkgs missing the action_env patch that exists in bazel6? I seem to be hitting the case that was solved in bazel 6 by #146389 after upgrading to bazel7, where build --action_env=PATH=/nix/store/...coreutils:/nix/store/...stuff isnt being propagated to ctx.action.run

@rickvanprim
Copy link
Contributor

@Strum355 I hit a few cases last night during my upgrade to Bazel 7 where things in actions.run() is missing coreutils and setting use_default_shell_env=True fixes it but I'm not sure if that's correct. Without it, it looks like the command is being run with zero environment (whereas in Bazel 6, the patched PATH was still there). I suspect needing use_default_shell_env=True is wrong though because I'm not sure how non-Nix usages would work either without some base amount of PATH given how many things assume the presence of coreutils.

@layus
Copy link
Member Author

layus commented Jan 5, 2024

I had the same problem with my deployment. And I just re-added the patch (locally) for now. What bugs me is that the environment ends up empty, but I see no path yet in the code where it can happen.

Re-adding the patch is the quickest fix.

My best guess is that the patch made us use run() in an incorrect way, assuming that coreutils is present when it is not by default. Bazel add PATH to the action digest, so for tools that need no path, it's better to have none. The patch fooled us into thinking that run() can be used that way. Not too sure yet, I need to investigate using a vanilla bazel.

Oh, and not gonna work on that in the next few days sadly, but probably end of next week.

@Strum355
Copy link
Contributor

Strum355 commented Jan 5, 2024

Is bazel7 in nixpkgs missing the action_env patch that exists in bazel6? I seem to be hitting the case that was solved in bazel 6 by #146389 after upgrading to bazel7, where build --action_env=PATH=/nix/store/...coreutils:/nix/store/...stuff isnt being propagated to ctx.action.run

Correction on this, it appears that what I thought was a case of this patch causing the passing through action_env wasnt actually the case, but it was masking the fact that action_env wasnt being passed through by hard-coding just enough to make some rules that depend on e.g. coreutils run successfully. A lot of convolution around action_env and where/what passes through $PATH, but the patch is just "good enough" for me to keep working. Ideally everything would pass down action_env, as it would mean I could avoid rebuilding bazel by having to to set a different defaultShellPath :/

@boltzmannrain
Copy link
Contributor

boltzmannrain commented Jan 5, 2024

With --repo_env and --action_env it's possible to set PATH and even LD_LIBRARY_PATH but some rules override that and pretend to be in control while in reality they rely on implicit non-hermetic dependencies in FHS locations.

Another rabbit hole with this is it's possible to undo nixos glibc&bazel patches, tweak bazel build once, and then run builds in specially-crafted buildFHSEnv. So rules that expect to work with empty env/path will fallback to fhs through glibc (hence need to unpatch it, nixos disables fhs fallback) and find whatever is put into fhs (and needs to be in fhs as this is where glibc would look on empty env). Also for dynamic linking it might be needed to also undo ld.so.cache fallback patch and put libs into cache in fhs env. To make it bazel cache friendly probably still makes sense to add --repo_env and --action_env to bazelrc

Strum355 added a commit to sourcegraph/sourcegraph-public-snapshot that referenced this pull request Jan 16, 2024
bazel (and its community of rules) is a bit of a mess when it comes to passing around env values, including $PATH, resulting in some actions not receiving the $PATH value set via `--action_path=PATH=...` that we rely on. Bazel 6 in nixpkgs had [the following patch](https://sourcegraph.com/github.com/NixOS/nixpkgs/-/blob/pkgs/development/tools/build-managers/bazel/bazel_6/actions_path.patch) that hardcodes a set of packages to include in that case, which is currently missing from Bazel 7 in nixpkgs, so we (temporarily, hopefully) reintroduce that patch locally. 

See: NixOS/nixpkgs#262152 (comment) and NixOS/nixpkgs#94222 for some reading

I plan to move more bazel stuff out of shell.nix perhaps, into bazel.nix, as its a tad messy all-in-one

## Test plan

`bazel build //client/web/dist` is successful
@uri-canva
Copy link
Contributor

You probably want to open a new issue for that. Using bazel from nixpkgs correctly is very hard, both with or without buildBazelPackage, I've opened an issue for the overarching problems that affect all build tools from nix when used outside of nix (#185742), but if you have a more scoped down issue in mind that would be good to open that too.

@Strum355 Strum355 mentioned this pull request Feb 17, 2024
13 tasks
@layus layus mentioned this pull request Feb 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.