Duplicate of #40205

Oh cool, that wasn't there when I started working on this, let me rebase it.

Would be great if you and @rdnetto4 can merge your PRs into one.

What was wrong with the customBash hack? I fear your proposed new direction will be a lot harder to maintain by adding more work to upgrading Bazel to newer versions.

It doesn't work when building 0.13.0 on NixOS, and I haven't looked into why and how to fix it because I needed a non-customBash approach anyway to make it work on darwin: Bazel uses sandbox-exec to execute actions on darwin, which disallows non-binaries used as interpreters in shebangs.

Not using the customBash hack is harder to maintain, but it's also using dependencies correctly: unrelated to the customBash see how the build is now forced to use the correct versions of grep, sed, etc, and the compiled binary references the correct paths for the tools it uses: this is hard to do with PATH manipulation because Bazel will drop environment variables to do its own sandboxing, and relies on certain tools existing in /bin, /usr/bin, or bash being able to find them without propagating PATH.

Note this also means the projects being built with Bazel after the hack is removed will have to configure the build correctly using --action_env and use_default_shell_env=True, as is being done for Bazel in the patch.

It doesn't work when building 0.13.0 on NixOS

here's a simple fix for that:

@@ -37,9 +37,11 @@ stdenv.mkDerivation rec {
   postPatch = ''
     find src/main/java/com/google/devtools -type f -print0 | while IFS="" read -r -d "" path; do
       substituteInPlace "$path" \
-        --replace /bin/bash ${customBash}/bin/bash \
-        --replace /usr/bin/env ${coreutils}/bin/env
+        --replace /bin/bash ${customBash}/bin/bash
     done
+    # Fixup scripts that generate scripts. Not fixed up by patchShebangs below.
+    substituteInPlace scripts/bootstrap/compile.sh \
+        --replace /bin/sh ${customBash}/bin/bash
     patchShebangs .

unrelated to the customBash see how the build is now forced to use the correct versions of grep, sed, etc, and the compiled binary references the correct paths for the tools it uses

I'm not sure I understand this one. Even in a pure shell, Bazel uses the grep, sed etc provided by customBash. Do you have an example where this is not the case?

Note this also means the projects being built with Bazel after the hack is removed will have to configure the build correctly using --action_env and use_default_shell_env=True, as is being done for Bazel in the patch.

This is a big breaking change (all rules everywhere would have to be updated) that could make builds non-reproducible depending on whether the user uses Bazel through Nixpkgs or not. Why do you think this would be the "correct" thing to do? In https://github.com/tweag/rules_haskell, we try to avoid use_default_shell_env=True like the plague.

needed a non-customBash approach anyway to make it work on darwin: Bazel uses sandbox-exec to execute actions on darwin, which disallows non-binaries used as interpreters in shebangs.

This is a problem, though unrelated to the upgrade to 0.13 I guess, which can make do with the smaller change I outlined above. And there are other ways to work around this problem that potentially need less patching. Like creating a binary wrapper instead of script one.

For reference, Bazel 0.13 builds projects successfully with the following commit: mboes@40c18d9.

This is a big breaking change

Yeah I'm not convinced it's the best way to go.

there are other ways to work around this problem that potentially need less patching. Like creating a binary wrapper instead of script one.

Good idea!

I'm not sure I understand this one. Even in a pure shell, Bazel uses the grep, sed etc provided by customBash. Do you have an example where this is not the case?

customBash doesn't provide those tools, it only provides coreutils I think:

that could make builds non-reproducible depending on whether the user uses Bazel through Nixpkgs or not.

Builds with Bazel are already non-reproducible if you switch the installation method: Bazel installed with Nix will use binaries from the Nix store, including java and gcc, and Bazel installed from other package managers will use binaries installed in the system, whichever they might be.

Why do you think this would be the "correct" thing to do?

This lets you define the environment you're running Bazel in in your Bazel build definitions instead of relying on the environment specified in the Nix derivation: it would be quite strange if you had access to some tools but not others. For example assuming I understand customBash correctly you wouldn't get access to sed and grep and awk, which Bazel's own build definitions assume you have access to. On the other hand we could use the Nix stdenv as a baseline maybe.

we try to avoid use_default_shell_env=True like the plague

Yeah use_default_shell_env=True on its own is very problematic and can cause non-deterministic builds, but you can use it safely with --experimental_strict_action_env and --action_env.

The big difference between the wrapper and absolute paths approaches is the environment Bazel runs actions in by default:

1. Wrapper: bash and coreutils and possibly grep, sed, awk, tar, gzip, bzip2, xz, zip, unzip, perl, ???
2. Absolute paths: bash

Note that in the first case the actions should use binaries from nix, so assuming you pinned the nixpkgs commit, you should get reproducible builds.

It's true that the Bazel build does use sed and other tools, so there might be something I'm missing for customBash to be able to find them, maybe stdenv.shell already includes all tools available to nix derivations?

From what I can tell sed, grep and the like are only used in tests or other scripts not executed during build time of Bazel itself. I haven't tried running the tests, but for this reason, I'd be surprised if they work.

Now, Bazel might use sed from the user's PATH when building a user project, since PATH is not expunged. It might also use whatever gcc it finds on the path. Maybe not ideal, but that's a larger change to consider and not a blocker to upgrading to bazel-0.13.

Builds with Bazel are already non-reproducible if you switch the installation method: Bazel installed with Nix will use binaries from the Nix store, including java and gcc, and Bazel installed from other package managers will use binaries installed in the system, whichever they might be.

Different kind of reproducibility. I guess I should have said "portability" instead. Ultimately, as a rule author, I don't want to have to write rules in one way when in Nixpkgs (which I can't detect reliably) and another elsewhere. As a developer, I don't want my users to have to pass extra flags on the command-line, or not, depending on their distribution. It's unclear to me whether that's still the case with your proposed changes. In any case, I submit that these are interesting changes but ought to be considered in a separate PR.

I submit that these are interesting changes but ought to be considered in a separate PR.

Yeah absolutely, since there is a smaller change possible to upgrade the version that's what we should do first.

Ultimately, as a rule author, I don't want to have to write rules in one way when in Nixpkgs (which I can't detect reliably) and another elsewhere.

I see, that makes sense. I was interested in using Nix under macOS, so it's a very different situation: if Bazel works exactly like installed with brew, as in refers to the shell path either as defined in the calling environment or as defined as bash default, and uses a lot of binaries from there, then it's very hard to achieve reproducible builds when using it.

Will close this PR and create a new one for darwin support once the upgrade has been merged on its own in #40205 or #40525.