Hermetic nixpkgs_cc_toolchain #5976

aherrmann-da · 2020-05-14T08:39:22Z

So far we were using nixpkgs_cc_toolchain of rules_nixpkgs which uses Bazel's auto configured CC toolchain applied to a Nix provided compiler under the hood. This is not hermetic as the auto configure toolchain picks up header include paths defined in the environment. These may change for example within a nix-shell environment. These builtin include paths affect the cache keys of all actions that depend on the cc toolchain and cause cache misses on mismatch.

This updates rules_nixpkgs and uses the new nixpkgs_cc_toolchain_hermetic (via patch until tweag/rules_nixpkgs#128 is merged). This new toolchain is hermetic in that it configures the cc toolchain during a nix-build within Nix's sandbox.

Pull Request Checklist

Read and understand the contribution guidelines
Include appropriate tests
Set a descriptive title and thorough description
Add a reference to the issue this PR will solve, if appropriate
Include changelog additions in one or more commit message bodies between the CHANGELOG_BEGIN and CHANGELOG_END tags
Normal production system change, include purpose of change in description

NOTE: CI is not automatically run on non-members pull-requests for security
reasons. The reviewer will have to comment with /AzurePipelines run to
trigger the build.

cocreature

Great work, thank you so much!

deps.bzl

nix/tools/bazel-cc-toolchain/default.nix

cocreature · 2020-05-18T06:45:18Z

nix/tools/bazel-cc-toolchain/default.nix

@@ -70,5 +70,5 @@ let cc-darwin =
 in
 buildEnv {
  name = "bazel-cc-toolchain";
-  paths = [ customStdenv.cc ] ++ (if stdenv.isDarwin then [ ] else [ binutils ]);
+  paths = [ customStdenv.cc ] ++ (if stdenv.isDarwin then [ darwin.binutils ] else [ binutils ]);


Is this necessary now? For what?

Yes, the old toolchain just picked /usr/bin/strip and friends. To make these parts hermetic as well, we want to use nixpkgs provided tools. On MacOS one use to use darwin.binutils instead of binutils. The latter includes a bin/strip that chokes on MACH-O binaries. See https://github.com/NixOS/nixpkgs-channels/blob/10100a97c8964e82b30f180fda41ade8e6f69e41/pkgs/os-specific/darwin/binutils/default.nix#L32

cocreature · 2020-05-18T06:50:29Z

bazel_tools/nixpkgs-hermetic-cc-toolchain.patch

+Use `nixpkgs_cc_configure_hermetic` instead.
+
+This uses Bazel's autoconfigure toolchain under the hood, which is
+inhermetic. In particular, system include directories specified in the


The description says “overriding this autodetection makes builds more hermetic” while here you then (correctly) specify that it is “inhermetic”. While that’s not really contradictory it does sound a bit confusing (no need to address this here but maybe worth keeping in mind for the rules_nixpkgs PR).

Yeah, the docstrings could use a second pass. It is more hermetic than the builtin autodetection in that at least it doesn't use whatever cc is in PATH but the one provided by nixpkgs. However, it is still inhermetic in that it is influenced by some environment variables.

bazel_tools/nixpkgs-hermetic-cc-toolchain.patch

cocreature · 2020-05-18T06:53:29Z

bazel_tools/nixpkgs-hermetic-cc-toolchain.patch

+    Returns:
+      The string with all instances of `$(location )` replaced by paths.
+    """
+    num = string.count("$(location ")


This looks like it is inconsistent with how Bazel handles this kind of stuff where you have to escape the $ if you want it to not have special meaning. Probably not a big deal.

I don't follow. We do want $(location ) to have special meaning, just that here we define that special meaning with _expand_location. Note $(location ) expension is not automatic, a rule has to invoke ctx.expand_location explicitly. However, here we are within a repository rule, so ctx.expand_location is not available to us.

What I mean is that only $(location …) has special meaning. In other contexts in Bazel where $(location …) has special meaning, everything starting with $ has special meaning and you have to escape something like $abc whereas here you do not need the escaping.

Oh, I see, yes that would be more consistent. location expansion is not enabled by default, so it's not a big deal here. But, yes, this should be improved upstream.

CHANGELOG_BEGIN CHANGELOG_END

When using --incompatible_enable_cc_toolchain_resolution instead cc actions still depend on `external/local_config_cc/builtin_include_directory_paths` as well as `external/nixpkgs_cc_toolchain_config/builtin_include_directory_paths`.

The toolchain only considers `bin/cc` and having the other symlinks around could lead to confusion

ghost · 2020-05-18T12:51:15Z

I can confirm that this PR fixes rebuilding when within a Nix shell. Thanks!

aherrmann-da force-pushed the hermetic-cc-toolchain branch 4 times, most recently from fb972e2 to f61232f Compare May 15, 2020 15:34

aherrmann-da marked this pull request as ready for review May 15, 2020 16:19

aherrmann-da requested a review from garyverhaegen-da as a code owner May 15, 2020 16:19

aherrmann-da requested review from cocreature and a user May 15, 2020 16:19

cocreature approved these changes May 18, 2020

View reviewed changes

aherrmann added 12 commits May 18, 2020 11:38

update rules_nixpkgs

8433509

Use hermetic nixpkgs cc toolchain

9ecb86e

CHANGELOG_BEGIN CHANGELOG_END

Work around Bazel's cc toolchain autodetection

435fdd5

Use --crosstool_top for hermetic cc toolchain

8b1abc7

When using --incompatible_enable_cc_toolchain_resolution instead cc actions still depend on `external/local_config_cc/builtin_include_directory_paths` as well as `external/nixpkgs_cc_toolchain_config/builtin_include_directory_paths`.

override local_config_cc

28ed88f

remove unused attribute

0143c56

Fix posix toolchain on Windows

39e3906

nixpkgs cc toolchain not on Windows

5cf7582

Fix nixpkgs cc toolchain on MacOS

0336c89

nixpkgs cc toolchain uses bin/cc

7c637fa

Use darwin.binutils on MacOS

727f585

Remove clang(++) and gcc (g++) symlinks

4408eca

The toolchain only considers `bin/cc` and having the other symlinks around could lead to confusion

aherrmann-da force-pushed the hermetic-cc-toolchain branch from ac4476f to 4408eca Compare May 18, 2020 09:38

Use hermetic toolchain in compatibility workspace

f0fb522

aherrmann-da force-pushed the hermetic-cc-toolchain branch from 64e794d to f0fb522 Compare May 18, 2020 13:43

Avoid empty linker flags

bcdb916

aherrmann-da merged commit 1b711d0 into master May 18, 2020

aherrmann-da deleted the hermetic-cc-toolchain branch May 18, 2020 16:03

aherrmann mentioned this pull request Jul 29, 2020

Add optional location expansion for the nixopts attribute tweag/rules_nixpkgs#132

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hermetic nixpkgs_cc_toolchain #5976

Hermetic nixpkgs_cc_toolchain #5976

aherrmann-da commented May 14, 2020

cocreature left a comment

cocreature May 18, 2020

aherrmann-da May 18, 2020

cocreature May 18, 2020

aherrmann-da May 18, 2020

cocreature May 18, 2020

aherrmann-da May 18, 2020

cocreature May 18, 2020

aherrmann-da May 18, 2020

ghost commented May 18, 2020

Hermetic nixpkgs_cc_toolchain #5976

Hermetic nixpkgs_cc_toolchain #5976

Conversation

aherrmann-da commented May 14, 2020

Pull Request Checklist

cocreature left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented May 18, 2020