Merge pull request #196 from NixOS/nixpkgs-diff-for-forks

Nixpkgs diff for forks

Author: infinisil

.github/workflows/main.yml

@@ -1,41 +1,48 @@
 name: CI
 on:
-  pull_request:
+  # We use pull_request_target such that Nixpkgs diff processing also works,
+  # because we need repository secrets for that, which pull_request doesn't allow from forks.
+  # However, it's very important that we don't run code from forks without sandboxing it,
+  # because that way anybody could potentially extract repository secrets!
+  pull_request_target:
   push:
     branches:
       - master
 
-concurrency:
-  group: ${{ github.ref }}
-  cancel-in-progress: true
-
 jobs:
   check:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v4
+        if: github.event_name != 'pull_request_target'
 
-      - uses: cachix/install-nix-action@v26
-
-      - name: reuse lint
-        run: nix-build -A packages.reuse && result/bin/reuse lint
-
-      - name: hlint
-        run: nix-build -A checks.hlint
+      - uses: actions/checkout@v4
+        if: github.event_name == 'pull_request_target'
+        with:
+          # To prevent running untrusted code from forks,
+          # pull_request_target will cause the base branch to be checked out, not the PR branch.
+          # In our case we check out the PR branch regardless,
+          # because we're sandboxing all untrusted code with a `nix-build`.
+          # (and the sandbox is enabled by default at least on Linux)
+          ref: refs/pull/${{ github.event.pull_request.number }}/merge
 
-      - name: treefmt
-        run: nix-build -A checks.treefmt
+      - uses: cachix/install-nix-action@v26
 
-      - name: build nixfmt
-        run: nix-build
-        if: success() || failure()
+      - uses: cachix/cachix-action@v14
+        with:
+          name: nixos-nixfmt
+          authToken: '${{ secrets.CACHIX_AUTH_TOKEN }}'
 
-      - name: run tests
-        run: nix-shell --run ./test/test.sh
+      - name: checks
+        run: nix-build -A ci
 
   nixpkgs-diff:
     runs-on: ubuntu-latest
-    if: github.event_name == 'pull_request'
+    if: github.event_name == 'pull_request_target'
+    # Ensures that we don't run two comment-posting workflows at the same time
+    concurrency:
+      group: ${{ github.workflow_ref }}-${{ github.event.pull_request.number }}
+      cancel-in-progress: true
     steps:
       - name: Find Comment
         uses: peter-evans/find-comment@v3
@@ -57,10 +64,20 @@ jobs:
 
             Will be available [here](https://github.com/${{ vars.MACHINE_USER }}/nixpkgs/commits/nixfmt-${{ github.event.pull_request.number }})
 
+      # To prevent running untrusted code from forks,
+      # pull_request_target will cause the base branch to be checked out, not the PR branch.
+      # This is exactly what we want in this case,
+      # because the sync-pr.sh script cannot be run sandboxed since it needs to have side effects.
+      # Instead, the script itself fetches the PR, but then runs its code within sandboxed derivations.
       - uses: actions/checkout@v4
 
       - uses: cachix/install-nix-action@v26
 
+      - uses: cachix/cachix-action@v14
+        with:
+          name: nixos-nixfmt
+          authToken: '${{ secrets.CACHIX_AUTH_TOKEN }}'
+
       - run: |
           ./scripts/sync-pr.sh \
             https://github.com/${{ github.repository }} \

.gitignore

@@ -3,5 +3,5 @@
 /dist/
 /dist-newstyle
 /.ghc.environment.*
-/result
+/result*
 /.direnv

default.nix

@@ -9,8 +9,18 @@ in
 let
   overlay = self: super: {
     haskell = super.haskell // {
-      packageOverrides = self: super: { nixfmt = self.callCabal2nix "nixfmt" src { }; };
+      packageOverrides = self: super: { nixfmt = self.callCabal2nix "nixfmt" haskellSource { }; };
     };
+
+    treefmt = super.treefmt.overrideAttrs (old: {
+      patches = [
+        # Makes it work in parallel: https://github.com/numtide/treefmt/pull/282
+        (self.fetchpatch {
+          url = "https://github.com/numtide/treefmt/commit/f596795cd24b50f048cc395866bb90a89d99152d.patch";
+          hash = "sha256-EPn+JAT3aZLSWmpdi9ULZ8o8RvrX+UFp0cQWfBcQgVg=";
+        })
+      ];
+    });
   };
 
   pkgs = import nixpkgs {
@@ -23,15 +33,27 @@ let
   };
 
   inherit (pkgs) haskell lib;
+  fs = lib.fileset;
+
+  allFiles = fs.gitTracked ./.;
 
-  src = lib.fileset.toSource {
+  # Used for source-wide checks
+  source = fs.toSource {
     root = ./.;
-    fileset = lib.fileset.unions [
-      ./nixfmt.cabal
-      ./src
-      ./main
-      ./LICENSE
-    ];
+    fileset = allFiles;
+  };
+
+  haskellSource = fs.toSource {
+    root = ./.;
+    # Limit to only files needed for the Haskell build
+    fileset = fs.intersection allFiles (
+      fs.unions [
+        ./nixfmt.cabal
+        ./src
+        ./main
+        ./LICENSE
+      ]
+    );
   };
 
   build = lib.pipe pkgs.haskellPackages.nixfmt [
@@ -53,13 +75,39 @@ let
     # Haskell formatter
     programs.fourmolu.enable = true;
   };
+
+  checks = {
+    inherit build;
+    hlint = pkgs.build.haskell.hlint haskellSource;
+    reuse = pkgs.stdenvNoCC.mkDerivation {
+      name = "nixfmt-reuse";
+      src = source;
+      nativeBuildInputs = with pkgs; [ reuse ];
+      buildPhase = "reuse lint";
+      installPhase = "touch $out";
+    };
+    tests = pkgs.stdenvNoCC.mkDerivation {
+      name = "nixfmt-tests";
+      src = fs.toSource {
+        root = ./.;
+        fileset = fs.intersection allFiles ./test;
+      };
+      nativeBuildInputs = with pkgs; [
+        shellcheck
+        build
+      ];
+      patchPhase = "patchShebangs .";
+      buildPhase = "./test/test.sh";
+      installPhase = "touch $out";
+    };
+    treefmt = treefmtEval.config.build.check source;
+  };
 in
 build
 // {
-  packages = {
-    nixfmt = build;
-    inherit (pkgs) reuse;
-  };
+  packages.nixfmt = build;
+
+  inherit pkgs;
 
   shell = pkgs.haskellPackages.shellFor {
     packages = p: [ p.nixfmt ];
@@ -74,13 +122,7 @@ build
     ];
   };
 
-  checks = {
-    hlint = pkgs.build.haskell.hlint src;
-    treefmt = treefmtEval.config.build.check (
-      lib.fileset.toSource {
-        root = ./.;
-        fileset = lib.fileset.gitTracked ./.;
-      }
-    );
-  };
+  inherit checks;
+
+  ci = pkgs.linkFarm "ci" checks;
 }

scripts/sync-pr-support.nix

@@ -0,0 +1,49 @@
+/*
+  This file supports the ./sync-pr.sh script with some Nix code.
+  Note that we are not using the nixfmt from the tree of this file,
+  instead the nixfmt to use is passed into the necessary function.
+  This way we don't need to rely on this internal file being stable.
+*/
+let
+  pkgs = (import ../default.nix { }).pkgs;
+  inherit (pkgs) lib;
+in
+{
+  # Filters a repo into a store path only containing its Git-tracked Nix files
+  repoNixFiles =
+    { repo }:
+    lib.fileset.toSource {
+      root = repo;
+      fileset = lib.fileset.intersection (lib.fileset.gitTracked repo) (
+        lib.fileset.fileFilter (file: file.hasExt "nix") repo
+      );
+    };
+
+  # Returns a derivation that contains the passed store path (e.g. from the above function)
+  # but with all Nix files formatted with the given nixfmt
+  formattedGitRepo =
+    { storePath, nixfmtPath }:
+    let
+      nixfmt = (import nixfmtPath { }).packages.nixfmt;
+    in
+    pkgs.runCommand "formatted"
+      {
+        nativeBuildInputs = with pkgs; [
+          treefmt
+          nixfmt
+        ];
+        treefmtConfig = ''
+          [formatter.nixfmt-rfc-style]
+          command = "nixfmt"
+          includes = [ "*.nix" ]
+        '';
+        passAsFile = [ "treefmtConfig" ];
+      }
+      ''
+        cp -r --no-preserve=mode ${builtins.storePath storePath} $out
+        treefmt \
+          --config-file "$treefmtConfigPath" \
+          --tree-root "$out" \
+          --no-cache
+      '';
+}

scripts/sync-pr.sh

@@ -4,7 +4,7 @@
 # - It only minimally depends on GitHub: All operations are done using Git directly
 # - Reuse of previous work: Formatting all of Nixpkgs takes some time, we don't want to recompute it if not necessary
 # - Handles force pushes gracefully and linearises merge commits
-#
+# - Runs all external code in Nix derivations, so this is safe to use for PRs from forks too
 
 set -euo pipefail
 
@@ -24,6 +24,8 @@ nixpkgsUrl=$3
 nixpkgsUpstreamUrl=https://github.com/NixOS/nixpkgs
 nixpkgsMirrorBranch=nixfmt-$nixfmtPrNumber
 
+SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )
+
 tmp=$(mktemp -d)
 cd "$tmp"
 trap 'rm -rf "$tmp"' exit
@@ -47,7 +49,7 @@ isLinear() {
 }
 
 step "Fetching nixfmt pull request and creating a branch for the head commit"
-git init nixfmt
+git init nixfmt -b unused
 git -C nixfmt fetch "$nixfmtUrl" "refs/pull/$nixfmtPrNumber/merge"
 nixfmtBaseCommit=$(git -C nixfmt rev-parse FETCH_HEAD^1)
 nixfmtHeadCommit=$(git -C nixfmt rev-parse FETCH_HEAD^2)
@@ -95,7 +97,7 @@ bodyForCommitIndex() {
 }
 
 step "Fetching upstream Nixpkgs commit history"
-git init --bare nixpkgs.git
+git init --bare nixpkgs.git -b unused
 
 git -C nixpkgs.git remote add upstream "$nixpkgsUpstreamUrl"
 # This makes sure that we don't actually have to fetch any contents, otherwise we'd wait forever!
@@ -179,9 +181,12 @@ else
           echo "Checking whether commit with index $(( nixpkgsCommitCount + 1 )) ($nixpkgsCommit) in nixpkgs corresponds to commit with index $nixpkgsCommitCount ($nixfmtCommit) in nixfmt"
 
           # We generate the bodies of the commits to contain the nixfmt commit so we can check against it here to verify it's the same
-          body=$(git -C nixpkgs.git log -1 "$nixpkgsCommit" --pretty=%B)
-          expectedBody=$(bodyForCommitIndex "$nixpkgsCommitCount")
-          if [[ "$body" == "$expectedBody" ]]; then
+          # Note that `--format=format:` makes it so that there's no extra newline
+          git -C nixpkgs.git log -1 "$nixpkgsCommit" --format=format:%B > "$tmp/body"
+          bodyForCommitIndex "$nixpkgsCommitCount" > "$tmp/expected-body"
+
+          # Ignore case so that we don't need to be worried about things like NixOS vs nixos in the URL
+          if diff --ignore-case "$tmp/body" "$tmp/expected-body"; then
             echo "It does!"
           else
             echo "It does not, this indicates a force push was done"
@@ -199,13 +204,23 @@ else
 fi
 
 
-git init nixpkgs
+git init nixpkgs -b unused
 git -C nixpkgs config user.name "GitHub Actions"
 git -C nixpkgs config user.email "[email protected]"
 
 step "Fetching contents of Nixpkgs base commit $nixpkgsBaseCommit"
 # This is needed because for every commit we reset Nixpkgs to the base branch before formatting
-git -C nixpkgs fetch --no-tags --depth 1 "$nixpkgsUpstreamUrl" "$nixpkgsBaseCommit"
+git -C nixpkgs fetch --no-tags --depth 1 "$nixpkgsUpstreamUrl" "$nixpkgsBaseCommit":base
+
+step "Checking out Nixpkgs at the base commit"
+git -C nixpkgs checkout base
+
+# Because we run the formatter in a Nix derivation, we need to get its input files into the Nix store.
+# Since they never change, it would be wasteful to import them multiple times for each nixfmt run.
+# So instead we just import them once and reuse that result throughout
+step "Importing Nix files from base commit into the Nix store"
+baseStorePath=$(nix-instantiate --eval --read-write-mode "$SCRIPT_DIR/sync-pr-support.nix" -A repoNixFiles.outPath --arg repo "$PWD/nixpkgs" | tr -d '"')
+echo "$baseStorePath"
 
 step "Fetching contents of the starting commit and updating the mirror branch"
 # This is only needed so we can push the resulting diff after formatting
@@ -234,9 +249,6 @@ update() {
 
   step "Checking out nixfmt at $nixfmtCommit"
   git -C nixfmt checkout -q "$nixfmtCommit"
-
-  step "Building nixfmt"
-  nix-build nixfmt
 }
 
 # Format Nixpkgs with a specific nixfmt version and push the result.
@@ -247,21 +259,21 @@ next() {
 
   update "$index"
 
-  if [[ -n "$appliedNixfmtPath" && "$appliedNixfmtPath" == "$(realpath result)" ]]; then
-    echo "The nixfmt store path didn't change, saving ourselves a formatting"
-  else
-    step "Checking out Nixpkgs at the base commit"
-    git -C nixpkgs checkout "$nixpkgsBaseCommit" -- .
+  step "Checking out Nixpkgs at the base commit"
+  git -C nixpkgs checkout base -- .
 
-    step "Running nixfmt on nixpkgs"
-    if ! time xargs -r -0 -P"$(nproc)" -n1 -a <(find nixpkgs -type f -name '*.nix' -print0) result/bin/nixfmt; then
-      echo -e "\e[31mFailed to run nixfmt on some files\e[0m"
-      exit 1
-    fi
-    git -C nixpkgs add -A
+  step "Running nixfmt on nixpkgs in a derivation"
 
-    appliedNixfmtPath=$(realpath result)
+  # This uses always the same sync-pr-support.nix file from the same nixfmt branch that this script is in,
+  # but doesn't use anything else from that nixfmt branch. Instead the nixfmtPath is used for the formatting.
+  if ! nix-build "$SCRIPT_DIR/sync-pr-support.nix" -A formattedGitRepo --arg storePath "$baseStorePath" --arg nixfmtPath "$PWD/nixfmt"; then
+    echo -e "\e[31mFailed to run nixfmt on some files\e[0m"
+    exit 1
   fi
+  step "Syncing changes into the Nixpkgs tree"
+  # We need to move the changed files in result/ back into the tree (without messing up the permissions and other files)
+  rsync --archive --no-perms result/ nixpkgs
+  git -C nixpkgs add -A
 
   step "Committing the formatted result"
   git -C nixpkgs commit --allow-empty -m "$(bodyForCommitIndex "$index")"
@@ -271,14 +283,12 @@ next() {
   nixpkgsCommitCount=$(( nixpkgsCommitCount + 1 ))
 }
 
-appliedNixfmtPath=
 if (( nixpkgsCommitCount == 0 )); then
   # If we don't have a base-formatted Nixpkgs commit yet, create it
   next 0
 else
   # Otherwise, just build the nixfmt that was used for the current commit, such that we know the store path
   update "$(( nixpkgsCommitCount - 1 ))"
-  appliedNixfmtPath=$(realpath result)
 fi
 
 while (( nixpkgsCommitCount - 1 < nixfmtCommitCount )); do