Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: distinguish build failures from store failures #416

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

CyberShadow
Copy link
Contributor

"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 #415

"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.
@CyberShadow
Copy link
Contributor Author

CyberShadow commented Sep 9, 2024

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
Copy link
Owner

Mic92 commented Sep 9, 2024

@mergify queue

Copy link
Contributor

mergify bot commented Sep 9, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 1097624

@mergify mergify bot merged commit 1097624 into Mic92:master Sep 9, 2024
4 checks passed
@Mic92
Copy link
Owner

Mic92 commented Sep 9, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

nixpkgs-review doesn't differentiate between build and store errors.
2 participants