Mic92 / nixpkgs-review

Review pull-requests on https://github.com/NixOS/nixpkgs
MIT License
410 stars 66 forks source link

fix: distinguish build failures from store failures #416

Closed CyberShadow closed 2 months ago

CyberShadow commented 2 months ago

"nix-store --verify-path" exits with status 1 in various situations, including when the path does not exist (due to having failed to build) or when the checksum doesn't match the Nix database.

Since nixpkgs-review tries to report build failures specifically, use "nix store verify" instead with its "--no-contents" flag. This avoids classifying checksum mismatches as build failures, which can be misleading to nixpkgs-review users.

Though still an abnormal situation that might be useful to report, we now ignore store path errors; these are generally an abnormal situation, and detecting them is out-of-scope for nixpkgs-review. Should it become clearer that it would be useful to report them, it could be done by removing --no-contents and checking "nix store verify"'s exit status, which indicates the nature of the problem.

Fixes https://github.com/Mic92/nixpkgs-review/issues/415

CyberShadow commented 2 months ago

I tested the two approaches with this script:

#!/usr/bin/env bash
set -eEuo pipefail

expr='
let
  name = builtins.getEnv "name";  # https://github.com/NixOS/nix/issues/2678
  pkgs = import <nixpkgs> {};
  pkg = pkgs.stdenv.mkDerivation {
    inherit name;
    dontUnpack = true;
    buildCommand = if name == "failed" then "false" else "mkdir $out";
  };
in {
  path = "${pkgs.lib.getOutput "out" pkg}";
  inherit pkg;
}
'

for f in good corrupted failed ; do
  name="$f" \
  nix-build \
    -A pkg \
    --expr "$expr" || true

  path=$(
    name="$f" \
    nix \
      --extra-experimental-features nix-command \
      eval \
      --expr "$expr" \
      path \
      --impure \
      --raw
  )
  if [[ "$f" == corrupted ]] ; then
    sudo touch     "$path"/oops.txt
    sudo chmod 666 "$path"/oops.txt
  fi

  status=0
  # old method:
  # nix-store --verify-path "$path" || status=$?
  # new method:
  nix --extra-experimental-features nix-command store verify --no-contents --no-trust "$path" || status=$?
  echo "$f: $status"
done

The "old method" fails on failed and corrupted. The "new method" fails only on failed, as desired.

Mic92 commented 2 months ago

@mergify queue

mergify[bot] commented 2 months ago

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at *1097624569a7de712ef266d773d3dc8d2ba7aec9*
Mic92 commented 2 months ago

Thanks!