From d8239640a9fa26c932a4c234ee2d263837159388 Mon Sep 17 00:00:00 2001 From: Aaron Siddhartha Mondal Date: Thu, 21 Dec 2023 00:18:46 +0100 Subject: [PATCH] Add Nix formatters and linters to pre-commit hooks (#561) Alejandra formats, statix lints antipatterns, deadnix scans dead code. --- flake.nix | 215 +++++++++++---------- local-remote-execution/image.nix | 58 +++--- local-remote-execution/rbe-configs-gen.nix | 5 +- tools/customClang.nix | 6 +- tools/generate-toolchains.nix | 71 ++++--- tools/llvmStdenv.nix | 103 +++++----- tools/local-image-test.nix | 3 +- tools/pre-commit-hooks.nix | 41 ++-- tools/publish-ghcr.nix | 3 +- 9 files changed, 261 insertions(+), 244 deletions(-) diff --git a/flake.nix b/flake.nix index 1a3374af6..36d5c176a 100644 --- a/flake.nix +++ b/flake.nix @@ -13,114 +13,126 @@ }; }; - outputs = inputs @ { self, flake-parts, crane, ... }: - flake-parts.lib.mkFlake { inherit inputs; } { + outputs = inputs @ { + self, + flake-parts, + crane, + ... + }: + flake-parts.lib.mkFlake {inherit inputs;} { systems = [ "x86_64-linux" "x86_64-darwin" "aarch64-darwin" ]; - imports = [ inputs.pre-commit-hooks.flakeModule ]; - perSystem = { config, pkgs, system, ... }: - let - isDarwin = builtins.elem system [ - "x86_64-darwin" - "aarch64-darwin" - ]; - - maybeDarwinDeps = pkgs.lib.optionals isDarwin [ - pkgs.darwin.apple_sdk.frameworks.Security - pkgs.libiconv - ]; - - customStdenv = import ./tools/llvmStdenv.nix { inherit pkgs isDarwin; }; - - # TODO(aaronmondal): This doesn't work with rules_rust yet. - # Tracked in https://github.com/TraceMachina/nativelink/issues/477. - customClang = pkgs.callPackage ./tools/customClang.nix { - inherit pkgs; - stdenv = customStdenv; - }; + imports = [inputs.pre-commit-hooks.flakeModule]; + perSystem = { + config, + pkgs, + system, + ... + }: let + isDarwin = builtins.elem system [ + "x86_64-darwin" + "aarch64-darwin" + ]; + + maybeDarwinDeps = pkgs.lib.optionals isDarwin [ + pkgs.darwin.apple_sdk.frameworks.Security + pkgs.libiconv + ]; + + customStdenv = import ./tools/llvmStdenv.nix {inherit pkgs isDarwin;}; + + # TODO(aaronmondal): This doesn't work with rules_rust yet. + # Tracked in https://github.com/TraceMachina/nativelink/issues/477. + customClang = pkgs.callPackage ./tools/customClang.nix { + inherit pkgs; + stdenv = customStdenv; + }; - craneLib = crane.lib.${system}; + craneLib = crane.lib.${system}; - src = pkgs.lib.cleanSourceWith { - src = craneLib.path ./.; - filter = path: type: - (builtins.match "^.+/data/SekienAkashita\\.jpg" path != null) || - (craneLib.filterCargoSources path type); - }; + src = pkgs.lib.cleanSourceWith { + src = craneLib.path ./.; + filter = path: type: + (builtins.match "^.+/data/SekienAkashita\\.jpg" path != null) + || (craneLib.filterCargoSources path type); + }; - commonArgs = { - inherit src; - strictDeps = true; - buildInputs = maybeDarwinDeps; - nativeBuildInputs = [ + commonArgs = { + inherit src; + strictDeps = true; + buildInputs = maybeDarwinDeps; + nativeBuildInputs = + [ pkgs.cacert - ] ++ maybeDarwinDeps ++ pkgs.lib.optionals (!isDarwin) [ pkgs.autoPatchelfHook ]; - stdenv = customStdenv; - }; + ] + ++ maybeDarwinDeps + ++ pkgs.lib.optionals (!isDarwin) [pkgs.autoPatchelfHook]; + stdenv = customStdenv; + }; - # Additional target for external dependencies to simplify caching. - cargoArtifacts = craneLib.buildDepsOnly commonArgs; + # Additional target for external dependencies to simplify caching. + cargoArtifacts = craneLib.buildDepsOnly commonArgs; - nativelink = craneLib.buildPackage (commonArgs - // { + nativelink = craneLib.buildPackage (commonArgs + // { inherit cargoArtifacts; }); - hooks = import ./tools/pre-commit-hooks.nix { inherit pkgs; }; + hooks = import ./tools/pre-commit-hooks.nix {inherit pkgs;}; - publish-ghcr = import ./tools/publish-ghcr.nix { inherit pkgs; }; + publish-ghcr = import ./tools/publish-ghcr.nix {inherit pkgs;}; - local-image-test = import ./tools/local-image-test.nix { inherit pkgs; }; + local-image-test = import ./tools/local-image-test.nix {inherit pkgs;}; - generate-toolchains = import ./tools/generate-toolchains.nix { inherit pkgs; }; - in - { - apps = { - default = { - type = "app"; - program = "${nativelink}/bin/cas"; - }; + generate-toolchains = import ./tools/generate-toolchains.nix {inherit pkgs;}; + in { + apps = { + default = { + type = "app"; + program = "${nativelink}/bin/cas"; }; - packages = { - inherit publish-ghcr local-image-test; - default = nativelink; - lre = import ./local-remote-execution/image.nix { inherit pkgs nativelink; }; - image = pkgs.dockerTools.streamLayeredImage { - name = "nativelink"; - contents = [ - nativelink - pkgs.dockerTools.caCertificates - ]; - config = { - Entrypoint = [ "/bin/cas" ]; - Labels = { - "org.opencontainers.image.description" = "An RBE compatible, high-performance cache and remote executor."; - "org.opencontainers.image.documentation" = "https://github.com/TraceMachina/nativelink"; - "org.opencontainers.image.licenses" = "Apache-2.0"; - "org.opencontainers.image.revision" = "${self.rev or self.dirtyRev or "dirty"}"; - "org.opencontainers.image.source" = "https://github.com/TraceMachina/nativelink"; - "org.opencontainers.image.title" = "Native Link"; - "org.opencontainers.image.vendor" = "Trace Machina, Inc."; - }; + }; + packages = { + inherit publish-ghcr local-image-test; + default = nativelink; + lre = import ./local-remote-execution/image.nix {inherit pkgs nativelink;}; + image = pkgs.dockerTools.streamLayeredImage { + name = "nativelink"; + contents = [ + nativelink + pkgs.dockerTools.caCertificates + ]; + config = { + Entrypoint = ["/bin/cas"]; + Labels = { + "org.opencontainers.image.description" = "An RBE compatible, high-performance cache and remote executor."; + "org.opencontainers.image.documentation" = "https://github.com/TraceMachina/nativelink"; + "org.opencontainers.image.licenses" = "Apache-2.0"; + "org.opencontainers.image.revision" = "${self.rev or self.dirtyRev or "dirty"}"; + "org.opencontainers.image.source" = "https://github.com/TraceMachina/nativelink"; + "org.opencontainers.image.title" = "Native Link"; + "org.opencontainers.image.vendor" = "Trace Machina, Inc."; }; }; }; - checks = { - # TODO(aaronmondal): Fix the tests. - # tests = craneLib.cargoNextest (commonArgs - # // { - # inherit cargoArtifacts; - # cargoNextestExtraArgs = "--all"; - # partitions = 1; - # partitionType = "count"; - # }); - }; - pre-commit.settings = { inherit hooks; }; - devShells.default = pkgs.mkShell { - nativeBuildInputs = [ + }; + checks = { + # TODO(aaronmondal): Fix the tests. + # tests = craneLib.cargoNextest (commonArgs + # // { + # inherit cargoArtifacts; + # cargoNextestExtraArgs = "--all"; + # partitions = 1; + # partitionType = "count"; + # }); + }; + pre-commit.settings = {inherit hooks;}; + devShells.default = pkgs.mkShell { + nativeBuildInputs = + [ # Development tooling goes here. pkgs.cargo pkgs.rustc @@ -138,20 +150,21 @@ local-image-test generate-toolchains customClang - ] ++ maybeDarwinDeps; - shellHook = '' - # Generate the .pre-commit-config.yaml symlink when entering the - # development shell. - ${config.pre-commit.installationScript} - - # The Bazel and Cargo builds in nix require a Clang toolchain. - # TODO(aaronmondal): The Bazel build currently uses the - # irreproducible host C++ toolchain. Provide - # this toolchain via nix for bitwise identical - # binaries across machines. - export CC=clang - ''; - }; + ] + ++ maybeDarwinDeps; + shellHook = '' + # Generate the .pre-commit-config.yaml symlink when entering the + # development shell. + ${config.pre-commit.installationScript} + + # The Bazel and Cargo builds in nix require a Clang toolchain. + # TODO(aaronmondal): The Bazel build currently uses the + # irreproducible host C++ toolchain. Provide + # this toolchain via nix for bitwise identical + # binaries across machines. + export CC=clang + ''; }; + }; }; } diff --git a/local-remote-execution/image.nix b/local-remote-execution/image.nix index fabc535ab..8aa69bf79 100644 --- a/local-remote-execution/image.nix +++ b/local-remote-execution/image.nix @@ -1,7 +1,9 @@ -{ pkgs, nativelink, ... }: - -let - customStdenv = import ../tools/llvmStdenv.nix { inherit pkgs; }; +{ + pkgs, + nativelink, + ... +}: let + customStdenv = import ../tools/llvmStdenv.nix {inherit pkgs;}; customClang = pkgs.callPackage ../tools/customClang.nix { inherit pkgs; stdenv = customStdenv; @@ -49,14 +51,15 @@ let # Add all tooling here so that the generated toolchains use `/nix/store/*` # paths instead of `/bin` or `/usr/bin`. This way we're guaranteed to use # binary identical toolchains during local and remote execution. - ("PATH=" + (pkgs.lib.strings.concatStringsSep ":" [ - "${customStdenv.cc.bintools}/bin" - "${customClang}/bin" - "${customStdenv}/bin" - "${pkgs.coreutils}/bin" - "${pkgs.findutils}/bin" - "${pkgs.gnutar}/bin" - ])) + ("PATH=" + + (pkgs.lib.strings.concatStringsSep ":" [ + "${customStdenv.cc.bintools}/bin" + "${customClang}/bin" + "${customStdenv}/bin" + "${pkgs.coreutils}/bin" + "${pkgs.findutils}/bin" + "${pkgs.gnutar}/bin" + ])) "JAVA_HOME=${pkgs.jdk11_headless}/lib/openjdk" "CC=${customClang}/bin/customClang" @@ -74,11 +77,12 @@ let "-L${pkgs.llvmPackages_17.libcxxabi}/lib" "-L${pkgs.llvmPackages_17.libunwind}/lib" "-lc++" - ("-Wl," + - "-rpath,${pkgs.llvmPackages_17.libcxx}/lib," + - "-rpath,${pkgs.llvmPackages_17.libcxxabi}/lib," + - "-rpath,${pkgs.llvmPackages_17.libunwind}/lib," + - "-rpath,${pkgs.glibc}/lib" + ( + "-Wl," + + "-rpath,${pkgs.llvmPackages_17.libcxx}/lib," + + "-rpath,${pkgs.llvmPackages_17.libcxxabi}/lib," + + "-rpath,${pkgs.llvmPackages_17.libunwind}/lib," + + "-rpath,${pkgs.glibc}/lib" ) ]}" ]; @@ -91,18 +95,16 @@ let contents = autogenDeps; }; - in + pkgs.dockerTools.streamLayeredImage { + name = "nativelink-toolchain"; -pkgs.dockerTools.streamLayeredImage { - name = "nativelink-toolchain"; + # Override the toolchain container tag with the one from the autogen + # container. This way the nativelink doesn't influence this tag and and + # changes to its codebase don't invalidate existing toolchain containers. + tag = autogenContainer.imageTag; - # Override the toolchain container tag with the one from the autogen - # container. This way the nativelink doesn't influence this tag and and - # changes to its codebase don't invalidate existing toolchain containers. - tag = autogenContainer.imageTag; - - inherit extraCommands config; + inherit extraCommands config; - contents = autogenDeps ++ [ nativelink ]; -} + contents = autogenDeps ++ [nativelink]; + } diff --git a/local-remote-execution/rbe-configs-gen.nix b/local-remote-execution/rbe-configs-gen.nix index a2598bfcd..e9569c8d3 100644 --- a/local-remote-execution/rbe-configs-gen.nix +++ b/local-remote-execution/rbe-configs-gen.nix @@ -1,5 +1,4 @@ -{ pkgs, ... }: - +{pkgs, ...}: pkgs.buildGoModule rec { pname = "bazel-toolchains"; version = "5.1.2"; @@ -21,6 +20,6 @@ pkgs.buildGoModule rec { description = "Generate Bazel toolchain configs for remote execution."; homepage = "https://github.com/bazelbuild/bazel-toolchains"; license = licenses.asl20; - maintainers = [ maintainers.aaronmondal ]; + maintainers = [maintainers.aaronmondal]; }; } diff --git a/tools/customClang.nix b/tools/customClang.nix index 6246c902c..1827af3e0 100644 --- a/tools/customClang.nix +++ b/tools/customClang.nix @@ -1,5 +1,7 @@ -{ pkgs, stdenv }: - +{ + pkgs, + stdenv, +}: # Bazel expects a single frontend for both C and C++. That works for GCC but # not for clang. This wrapper selects `clang` or `clang++` depending on file # ending. diff --git a/tools/generate-toolchains.nix b/tools/generate-toolchains.nix index 2883720dc..5a93b4e1b 100644 --- a/tools/generate-toolchains.nix +++ b/tools/generate-toolchains.nix @@ -1,39 +1,34 @@ -{ pkgs, }: - -let - -rbeConfigsGen = import ../local-remote-execution/rbe-configs-gen.nix { - inherit pkgs; -}; - +{pkgs}: let + rbeConfigsGen = import ../local-remote-execution/rbe-configs-gen.nix { + inherit pkgs; + }; in - -pkgs.writeShellScriptBin "generate-toolchains" '' - #!{pkgs.bash}/bin/bash - set -xeuo pipefail - - SRC_ROOT=$(git rev-parse --show-toplevel) - - cd "''${SRC_ROOT}" - - IMAGE_TAG=$(nix eval .#lre.imageTag --raw) - - $(nix build .#lre --print-build-logs --verbose) \ - && ./result \ - | ${pkgs.skopeo}/bin/skopeo \ - copy \ - docker-archive:/dev/stdin \ - docker-daemon:nativelink-toolchain:''${IMAGE_TAG} - - ${rbeConfigsGen}/bin/rbe_configs_gen \ - --toolchain_container=nativelink-toolchain:''${IMAGE_TAG} \ - --exec_os=linux \ - --target_os=linux \ - --bazel_version=${pkgs.bazel.version} \ - --output_src_root=''${SRC_ROOT} \ - --output_config_path=local-remote-execution/generated \ - --bazel_path=${pkgs.bazel}/bin/bazel \ - --cpp_env_json=local-remote-execution/cpp_env.json - - pre-commit run -a -'' + pkgs.writeShellScriptBin "generate-toolchains" '' + #!{pkgs.bash}/bin/bash + set -xeuo pipefail + + SRC_ROOT=$(git rev-parse --show-toplevel) + + cd "''${SRC_ROOT}" + + IMAGE_TAG=$(nix eval .#lre.imageTag --raw) + + $(nix build .#lre --print-build-logs --verbose) \ + && ./result \ + | ${pkgs.skopeo}/bin/skopeo \ + copy \ + docker-archive:/dev/stdin \ + docker-daemon:nativelink-toolchain:''${IMAGE_TAG} + + ${rbeConfigsGen}/bin/rbe_configs_gen \ + --toolchain_container=nativelink-toolchain:''${IMAGE_TAG} \ + --exec_os=linux \ + --target_os=linux \ + --bazel_version=${pkgs.bazel.version} \ + --output_src_root=''${SRC_ROOT} \ + --output_config_path=local-remote-execution/generated \ + --bazel_path=${pkgs.bazel}/bin/bazel \ + --cpp_env_json=local-remote-execution/cpp_env.json + + pre-commit run -a + '' diff --git a/tools/llvmStdenv.nix b/tools/llvmStdenv.nix index 590ce3279..d3b475bcc 100644 --- a/tools/llvmStdenv.nix +++ b/tools/llvmStdenv.nix @@ -1,51 +1,56 @@ -{ pkgs, isDarwin ? false, ... }: - -let -llvmPackages = pkgs.llvmPackages_17; - -toolchain = if isDarwin then ( - pkgs.overrideCC ( - llvmPackages.libcxxStdenv.override { - targetPlatform.useLLVM = true; - } - ) - llvmPackages.clangUseLLVM -) else (pkgs.useMoldLinker ( - pkgs.overrideCC ( - llvmPackages.libcxxStdenv.override { - targetPlatform.useLLVM = true; - } - ) - llvmPackages.clangUseLLVM -)); +{ + pkgs, + isDarwin ? false, + ... +}: let + llvmPackages = pkgs.llvmPackages_17; + toolchain = + if isDarwin + then + ( + pkgs.overrideCC ( + llvmPackages.libcxxStdenv.override { + targetPlatform.useLLVM = true; + } + ) + llvmPackages.clangUseLLVM + ) + else + (pkgs.useMoldLinker ( + pkgs.overrideCC ( + llvmPackages.libcxxStdenv.override { + targetPlatform.useLLVM = true; + } + ) + llvmPackages.clangUseLLVM + )); in - -# This toolchain uses Clang as compiler, Mold as linker, libc++ as C++ standard -# library and compiler-rt as compiler runtime. Resulting rust binaries depend -# dynamically linked on the nixpkgs distribution of glibc. C++ binaries -# additionally depend dynamically on libc++, libunwind and libcompiler-rt. Due -# to a bug we also depend on libgcc_s. -# -# TODO(aaronmondal): At the moment this toolchain is only used for the Cargo -# build. The Bazel build uses a different mostly hermetic LLVM toolchain. We -# should merge the two by generating the Bazel cc_toolchain from this stdenv. -# This likely requires a rewrite of -# https://github.com/bazelbuild/bazel-toolchains as the current implementation -# has poor compatibility with custom container images and doesn't support -# generating toolchain configs from image archives. -# -# TODO(aaronmondal): Due to various issues in the nixpkgs LLVM toolchains we're -# not getting a pure Clang/LLVM toolchain here. My guess is that the runtimes -# were not built with the degenerate LLVM toolchain but with the regular GCC -# stdenv from nixpkgs. -# -# For instance, outputs depend on libgcc_s since libcxx seems to have been was -# built with a GCC toolchain. We're also not using builtin atomics, or at least -# we're redundantly linking libatomic. -# -# Fix this as it fixes a large number of issues, including better cross-platform -# compatibility, reduced closure size, and static-linking-friendly licensing. -# This requires building the llvm project with the correct multistage -# bootstrapping process. -toolchain + # This toolchain uses Clang as compiler, Mold as linker, libc++ as C++ + # standard library and compiler-rt as compiler runtime. Resulting rust + # binaries depend dynamically linked on the nixpkgs distribution of glibc. + # C++ binaries additionally depend dynamically on libc++, libunwind and + # libcompiler-rt. Due to a bug we also depend on libgcc_s. + # + # TODO(aaronmondal): At the moment this toolchain is only used for the Cargo + # build. The Bazel build uses a different mostly hermetic LLVM toolchain. We + # should merge the two by generating the Bazel cc_toolchain from this stdenv. + # This likely requires a rewrite of + # https://github.com/bazelbuild/bazel-toolchains as the current implementation + # has poor compatibility with custom container images and doesn't support + # generating toolchain configs from image archives. + # + # TODO(aaronmondal): Due to various issues in the nixpkgs LLVM toolchains + # we're not getting a pure Clang/LLVM toolchain here. My guess is that the + # runtimes were not built with the degenerate LLVM toolchain but with the + # regular GCC stdenv from nixpkgs. + # + # For instance, outputs depend on libgcc_s since libcxx seems to have been was + # built with a GCC toolchain. We're also not using builtin atomics, or at + # least we're redundantly linking libatomic. + # + # Fix this as it fixes a large number of issues, including better + # cross-platform compatibility, reduced closure size, and + # static-linking-friendly licensing. This requires building the llvm project + # with the correct multistage bootstrapping process. + toolchain diff --git a/tools/local-image-test.nix b/tools/local-image-test.nix index 74f7fd16e..fc9ed0f99 100644 --- a/tools/local-image-test.nix +++ b/tools/local-image-test.nix @@ -1,5 +1,4 @@ -{ pkgs, ... }: - +{pkgs, ...}: pkgs.writeShellScriptBin "local-image-test" '' set -xeuo pipefail diff --git a/tools/pre-commit-hooks.nix b/tools/pre-commit-hooks.nix index a6d217966..f00282fd4 100644 --- a/tools/pre-commit-hooks.nix +++ b/tools/pre-commit-hooks.nix @@ -1,10 +1,6 @@ -{ pkgs, ... }: -let - -excludes = ["^gencargo/" "^nativelink-proto/genproto"]; - -in -{ +{pkgs, ...}: let + excludes = ["^gencargo/" "^nativelink-proto/genproto"]; +in { # Default hooks trailing-whitespace-fixer = { inherit excludes; @@ -12,7 +8,7 @@ in name = "trailing-whitespace"; description = "Remove trailing whitespace"; entry = "${pkgs.python311Packages.pre-commit-hooks}/bin/trailing-whitespace-fixer"; - types = [ "text" ]; + types = ["text"]; }; end-of-file-fixer = { inherit excludes; @@ -20,7 +16,7 @@ in name = "end-of-file-fixer"; description = "Remove trailing whitespace"; entry = "${pkgs.python311Packages.pre-commit-hooks}/bin/end-of-file-fixer"; - types = [ "text" ]; + types = ["text"]; }; fix-byte-order-marker = { inherit excludes; @@ -33,40 +29,47 @@ in enable = true; name = "mixed-line-ending"; entry = "${pkgs.python311Packages.pre-commit-hooks}/bin/mixed-line-ending"; - types = [ "text" ]; + types = ["text"]; }; check-case-conflict = { inherit excludes; enable = true; name = "check-case-conflict"; entry = "${pkgs.python311Packages.pre-commit-hooks}/bin/check-case-conflict"; - types = [ "text" ]; + types = ["text"]; }; detect-private-key = { - excludes = excludes ++ [ - # Integration testfiles not intended for production. - "deployment-examples/docker-compose/example-do-not-use-in-prod-key.pem" - "deployment-examples/kubernetes/example-do-not-use-in-prod-key.pem" - ]; + excludes = + excludes + ++ [ + # Integration testfiles not intended for production. + "deployment-examples/docker-compose/example-do-not-use-in-prod-key.pem" + "deployment-examples/kubernetes/example-do-not-use-in-prod-key.pem" + ]; enable = true; name = "detect-private-key"; entry = "${pkgs.python311Packages.pre-commit-hooks}/bin/detect-private-key"; - types = [ "text" ]; + types = ["text"]; }; + # Nix + alejandra.enable = true; + statix.enable = true; + deadnix.enable = true; + # Starlark bazel-buildifier-format = { enable = true; name = "buildifier format"; description = "Format Starlark"; entry = "${pkgs.bazel-buildtools}/bin/buildifier -lint=fix"; - types = [ "bazel" ]; + types = ["bazel"]; }; bazel-buildifier-lint = { enable = true; name = "buildifier lint"; description = "Lint Starlark"; entry = "${pkgs.bazel-buildtools}/bin/buildifier -lint=warn"; - types = [ "bazel" ]; + types = ["bazel"]; }; } diff --git a/tools/publish-ghcr.nix b/tools/publish-ghcr.nix index 070377c90..53e3682a9 100644 --- a/tools/publish-ghcr.nix +++ b/tools/publish-ghcr.nix @@ -1,5 +1,4 @@ -{ pkgs, ... }: - +{pkgs, ...}: pkgs.writeShellScriptBin "publish-ghcr" '' set -xeuo pipefail