diff options
Diffstat (limited to 'pkgs/test')
27 files changed, 680 insertions, 108 deletions
diff --git a/pkgs/test/checkpointBuild/default.nix b/pkgs/test/checkpointBuild/default.nix new file mode 100644 index 0000000000000..4a59760230a6e --- /dev/null +++ b/pkgs/test/checkpointBuild/default.nix @@ -0,0 +1,57 @@ +{ hello, checkpointBuildTools, runCommandNoCC, texinfo, stdenv, rsync }: +let + baseHelloArtifacts = checkpointBuildTools.prepareCheckpointBuild hello; + patchedHello = hello.overrideAttrs (old: { + buildInputs = [ texinfo ]; + src = runCommandNoCC "patch-hello-src" { } '' + mkdir -p $out + cd $out + tar xf ${hello.src} --strip-components=1 + patch -p1 < ${./hello.patch} + ''; + }); + checkpointBuiltHello = checkpointBuildTools.mkCheckpointedBuild patchedHello baseHelloArtifacts; + + checkpointBuiltHelloWithCheck = checkpointBuiltHello.overrideAttrs (old: { + doCheck = true; + checkPhase = '' + echo "checking if unchanged source file is not recompiled" + [ "$(stat --format="%Y" lib/exitfail.o)" = "$(stat --format="%Y" ${baseHelloArtifacts}/outputs/lib/exitfail.o)" ] + ''; + }); + + baseHelloRemoveFileArtifacts = checkpointBuildTools.prepareCheckpointBuild (hello.overrideAttrs (old: { + patches = [ ./hello-additionalFile.patch ]; + })); + + preparedHelloRemoveFileSrc = runCommandNoCC "patch-hello-src" { } '' + mkdir -p $out + cd $out + tar xf ${hello.src} --strip-components=1 + patch -p1 < ${./hello-additionalFile.patch} + ''; + + patchedHelloRemoveFile = hello.overrideAttrs (old: { + buildInputs = [ texinfo ]; + src = runCommandNoCC "patch-hello-src" { } '' + mkdir -p $out + cd $out + ${rsync}/bin/rsync -cutU --chown=$USER:$USER --chmod=+w -r ${preparedHelloRemoveFileSrc}/* . + patch -p1 < ${./hello-removeFile.patch} + ''; + }); + + checkpointBuiltHelloWithRemovedFile = checkpointBuildTools.mkCheckpointedBuild patchedHelloRemoveFile baseHelloRemoveFileArtifacts; +in +stdenv.mkDerivation { + name = "patched-hello-returns-correct-output"; + buildCommand = '' + touch $out + + echo "testing output of hello binary" + [ "$(${checkpointBuiltHelloWithCheck}/bin/hello)" = "Hello, incremental world!" ] + echo "testing output of hello with removed file" + [ "$(${checkpointBuiltHelloWithRemovedFile}/bin/hello)" = "Hello, incremental world!" ] + ''; +} + diff --git a/pkgs/test/checkpointBuild/hello-additionalFile.patch b/pkgs/test/checkpointBuild/hello-additionalFile.patch new file mode 100644 index 0000000000000..345bc10ee49e5 --- /dev/null +++ b/pkgs/test/checkpointBuild/hello-additionalFile.patch @@ -0,0 +1,67 @@ +:100644 100644 0000000 0000000 M Makefile.in +:000000 100644 0000000 0000000 A src/additionalFile.c +:100644 100644 0000000 0000000 M src/hello.c +:100644 100644 0000000 0000000 M src/system.h + +diff --git a/Makefile.in b/Makefile.in +index 1597d39..f63f830 100644 +--- a/Makefile.in ++++ b/Makefile.in +@@ -312,7 +312,7 @@ am_lib_libhello_a_OBJECTS = lib/basename-lgpl.$(OBJEXT) \ + lib/version-etc.$(OBJEXT) lib/version-etc-fsf.$(OBJEXT) \ + lib/wctype-h.$(OBJEXT) lib/xmalloc.$(OBJEXT) \ + lib/xalloc-die.$(OBJEXT) lib/xstriconv.$(OBJEXT) \ +- lib/xstrndup.$(OBJEXT) ++ lib/xstrndup.$(OBJEXT) src/additionalFile.$(OBJEXT) + lib_libhello_a_OBJECTS = $(am_lib_libhello_a_OBJECTS) + am_hello_OBJECTS = src/hello.$(OBJEXT) + hello_OBJECTS = $(am_hello_OBJECTS) +@@ -1842,7 +1842,7 @@ lib_libhello_a_SOURCES = lib/basename-lgpl.c lib/c-ctype.h \ + $(am__append_4) $(am__append_5) lib/version-etc.h \ + lib/version-etc.c lib/version-etc-fsf.c lib/wctype-h.c \ + lib/xmalloc.c lib/xalloc-die.c lib/xstriconv.h lib/xstriconv.c \ +- lib/xstrndup.h lib/xstrndup.c ++ lib/xstrndup.h lib/xstrndup.c src/additionalFile.c + lib_libhello_a_LIBADD = $(gl_LIBOBJS) + lib_libhello_a_DEPENDENCIES = $(gl_LIBOBJS) + EXTRA_lib_libhello_a_SOURCES = lib/close.c lib/stripslash.c lib/dup2.c \ +diff --git a/src/additionalFile.c b/src/additionalFile.c +new file mode 100644 +index 0000000..34d683d +--- /dev/null ++++ b/src/additionalFile.c +@@ -0,0 +1,6 @@ ++#include "config.h" ++#include "system.h" ++ ++int somefunc() { ++ return 0; ++} +diff --git a/src/hello.c b/src/hello.c +index 2e7d38e..a8e36dc 100644 +--- a/src/hello.c ++++ b/src/hello.c +@@ -146,7 +146,11 @@ main (int argc, char *argv[]) + #endif + + /* Having initialized gettext, get the default message. */ +- greeting_msg = _("Hello, world!"); ++ if (somefunc() == 0) { ++ greeting_msg = _("Hello, world!"); ++ } else { ++ greeting_msg = _("Hello, incremental world!"); ++ } + + /* Even exiting has subtleties. On exit, if any writes failed, change + the exit status. The /dev/full device on GNU/Linux can be used for +diff --git a/src/system.h b/src/system.h +index d39cdb9..dc425d2 100644 +--- a/src/system.h ++++ b/src/system.h +@@ -59,4 +59,6 @@ + } \ + while (0) + ++int somefunc(); ++ + #endif /* HELLO_SYSTEM_H */ diff --git a/pkgs/test/checkpointBuild/hello-removeFile.patch b/pkgs/test/checkpointBuild/hello-removeFile.patch new file mode 100644 index 0000000000000..2939790dabce1 --- /dev/null +++ b/pkgs/test/checkpointBuild/hello-removeFile.patch @@ -0,0 +1,67 @@ +:100644 100644 0000000 0000000 M Makefile.in +:100644 000000 0000000 0000000 D src/additionalFile.c +:100644 100644 0000000 0000000 M src/hello.c +:100755 100755 0000000 0000000 M tests/hello-1 + +diff --git a/Makefile.in b/Makefile.in +index f63f830..1597d39 100644 +--- a/Makefile.in ++++ b/Makefile.in +@@ -312,7 +312,7 @@ am_lib_libhello_a_OBJECTS = lib/basename-lgpl.$(OBJEXT) \ + lib/version-etc.$(OBJEXT) lib/version-etc-fsf.$(OBJEXT) \ + lib/wctype-h.$(OBJEXT) lib/xmalloc.$(OBJEXT) \ + lib/xalloc-die.$(OBJEXT) lib/xstriconv.$(OBJEXT) \ +- lib/xstrndup.$(OBJEXT) src/additionalFile.$(OBJEXT) ++ lib/xstrndup.$(OBJEXT) + lib_libhello_a_OBJECTS = $(am_lib_libhello_a_OBJECTS) + am_hello_OBJECTS = src/hello.$(OBJEXT) + hello_OBJECTS = $(am_hello_OBJECTS) +@@ -1842,7 +1842,7 @@ lib_libhello_a_SOURCES = lib/basename-lgpl.c lib/c-ctype.h \ + $(am__append_4) $(am__append_5) lib/version-etc.h \ + lib/version-etc.c lib/version-etc-fsf.c lib/wctype-h.c \ + lib/xmalloc.c lib/xalloc-die.c lib/xstriconv.h lib/xstriconv.c \ +- lib/xstrndup.h lib/xstrndup.c src/additionalFile.c ++ lib/xstrndup.h lib/xstrndup.c + lib_libhello_a_LIBADD = $(gl_LIBOBJS) + lib_libhello_a_DEPENDENCIES = $(gl_LIBOBJS) + EXTRA_lib_libhello_a_SOURCES = lib/close.c lib/stripslash.c lib/dup2.c \ +diff --git a/src/additionalFile.c b/src/additionalFile.c +deleted file mode 100644 +index 34d683d..0000000 +--- a/src/additionalFile.c ++++ /dev/null +@@ -1,6 +0,0 @@ +-#include "config.h" +-#include "system.h" +- +-int somefunc() { +- return 0; +-} +diff --git a/src/hello.c b/src/hello.c +index a8e36dc..53722d9 100644 +--- a/src/hello.c ++++ b/src/hello.c +@@ -126,6 +126,10 @@ parse_options (int argc, char *argv[], const char **greeting_msg) + } + } + ++int somefunc() { ++ return 1; ++} ++ + int + main (int argc, char *argv[]) + { +diff --git a/tests/hello-1 b/tests/hello-1 +index 96ffef8..f0b9f8d 100755 +--- a/tests/hello-1 ++++ b/tests/hello-1 +@@ -21,7 +21,7 @@ export LANGUAGE LC_ALL LC_MESSAGES LANG + + tmpfiles="hello-test1.ok" + cat <<EOF > hello-test1.ok +-Hello, world! ++Hello, incremental world! + EOF + + tmpfiles="$tmpfiles hello-test1.out" diff --git a/pkgs/test/checkpointBuild/hello.patch b/pkgs/test/checkpointBuild/hello.patch new file mode 100644 index 0000000000000..3d0d50c2f20e4 --- /dev/null +++ b/pkgs/test/checkpointBuild/hello.patch @@ -0,0 +1,26 @@ +diff --git a/src/hello.c b/src/hello.c +index 182303c..453962f 100644 +--- a/src/hello.c ++++ b/src/hello.c +@@ -57,7 +57,7 @@ main (int argc, char *argv[]) + #endif + + /* Having initialized gettext, get the default message. */ +- greeting_msg = _("Hello, world!"); ++ greeting_msg = _("Hello, incremental world!"); + + /* Even exiting has subtleties. On exit, if any writes failed, change + the exit status. The /dev/full device on GNU/Linux can be used for +diff --git a/tests/hello-1 b/tests/hello-1 +index 3b7a815..e15fa95 100755 +--- a/tests/hello-1 ++++ b/tests/hello-1 +@@ -21,7 +21,7 @@ export LANGUAGE LC_ALL LC_MESSAGES LANG + + tmpfiles="hello-test1.ok" + cat <<EOF > hello-test1.ok +-Hello, world! ++Hello, incremental world! + EOF + + tmpfiles="$tmpfiles hello-test1.out" diff --git a/pkgs/test/cuda/default.nix b/pkgs/test/cuda/default.nix index be88bd3820a90..8431e4b9207db 100644 --- a/pkgs/test/cuda/default.nix +++ b/pkgs/test/cuda/default.nix @@ -27,4 +27,6 @@ rec { cuda-library-samples_cudatoolkit_11_3 cuda-library-samples_cudatoolkit_11_4 ; + + __attrsFailEvaluation = true; } diff --git a/pkgs/test/default.nix b/pkgs/test/default.nix index 3aed10deacf24..0fb367a13253a 100644 --- a/pkgs/test/default.nix +++ b/pkgs/test/default.nix @@ -113,17 +113,19 @@ with pkgs; install-shell-files = callPackage ./install-shell-files {}; + checkpoint-build = callPackage ./checkpointBuild {}; + kernel-config = callPackage ./kernel.nix {}; ld-library-path = callPackage ./ld-library-path {}; macOSSierraShared = callPackage ./macos-sierra-shared {}; - cross = callPackage ./cross {}; + cross = callPackage ./cross {} // { __attrsFailEvaluation = true; }; php = recurseIntoAttrs (callPackages ./php {}); - pkg-config = recurseIntoAttrs (callPackage ../top-level/pkg-config/tests.nix { }); + pkg-config = recurseIntoAttrs (callPackage ../top-level/pkg-config/tests.nix { }) // { __recurseIntoDerivationForReleaseJobs = true; }; buildRustCrate = callPackage ../build-support/rust/build-rust-crate/test { }; importCargoLock = callPackage ../build-support/rust/test/import-cargo-lock { }; diff --git a/pkgs/test/nixpkgs-check-by-name/README.md b/pkgs/test/nixpkgs-check-by-name/README.md index 146cea0a64ba1..19865ca0952b5 100644 --- a/pkgs/test/nixpkgs-check-by-name/README.md +++ b/pkgs/test/nixpkgs-check-by-name/README.md @@ -4,27 +4,14 @@ This directory implements a program to check the [validity](#validity-checks) of It is being used by [this GitHub Actions workflow](../../../.github/workflows/check-by-name.yml). This is part of the implementation of [RFC 140](https://github.com/NixOS/rfcs/pull/140). -## API +## Interface -This API may be changed over time if the CI workflow making use of it is adjusted to deal with the change appropriately. - -- Command line: `nixpkgs-check-by-name <NIXPKGS>` -- Arguments: - - `<NIXPKGS>`: The path to the Nixpkgs to check - - `--version <VERSION>`: The version of the checks to perform. - - Possible values: - - `v0` (default) - - `v1` +The interface of the tool is shown with `--help`: +``` +cargo run -- --help +``` - See [validation](#validity-checks) for the differences. -- Exit code: - - `0`: If the [validation](#validity-checks) is successful - - `1`: If the [validation](#validity-checks) is not successful - - `2`: If an unexpected I/O error occurs -- Standard error: - - Informative messages - - Detected problems if validation is not successful +The interface may be changed over time only if the CI workflow making use of it is adjusted to deal with the change appropriately. ## Validity checks @@ -32,7 +19,7 @@ These checks are performed by this tool: ### File structure checks - `pkgs/by-name` must only contain subdirectories of the form `${shard}/${name}`, called _package directories_. -- The `name`'s of package directories must be unique when lowercased +- The `name`'s of package directories must be unique when lowercased. - `name` is a string only consisting of the ASCII characters `a-z`, `A-Z`, `0-9`, `-` or `_`. - `shard` is the lowercased first two letters of `name`, expressed in Nix: `shard = toLower (substring 0 2 name)`. - Each package directory must contain a `package.nix` file and may contain arbitrary other files. @@ -41,9 +28,21 @@ These checks are performed by this tool: - Each package directory must not refer to files outside itself using symlinks or Nix path expressions. ### Nix evaluation checks -- `pkgs.${name}` is defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`. - - **Only after --version v1**: If `pkgs.${name}` is not auto-called from `pkgs/by-name`, `args` must not be empty -- `pkgs.lib.isDerivation pkgs.${name}` is `true`. +- For each package directory, the `pkgs.${name}` attribute must be defined as `callPackage pkgs/by-name/${shard}/${name}/package.nix args` for some `args`. +- For each package directory, `pkgs.lib.isDerivation pkgs.${name}` must be `true`. + +### Ratchet checks + +Furthermore, this tool implements certain [ratchet](https://qntm.org/ratchet) checks. +This allows gradually phasing out deprecated patterns without breaking the base branch or having to migrate it all at once. +It works by not allowing new instances of the pattern to be introduced, but allowing already existing instances. +The existing instances are coming from `<BASE_NIXPKGS>`, which is then checked against `<NIXPKGS>` for new instances. +Ratchets should be removed eventually once the pattern is not used anymore. + +The current ratchets are: + +- New manual definitions of `pkgs.${name}` (e.g. in `pkgs/top-level/all-packages.nix`) with `args = { }` + (see [nix evaluation checks](#nix-evaluation-checks)) must not be introduced. ## Development @@ -86,6 +85,10 @@ Tests are declared in [`./tests`](./tests) as subdirectories imitating Nixpkgs w allowing the simulation of package overrides to the real [`pkgs/top-level/all-packages.nix`](../../top-level/all-packages.nix`). The default is an empty overlay. +- `base` (optional): + Contains another subdirectory imitating Nixpkgs with potentially any of the above structures. + This is used for [ratchet checks](#ratchet-checks). + - `expected` (optional): A file containing the expected standard output. The default is expecting an empty standard output. diff --git a/pkgs/test/nixpkgs-check-by-name/scripts/README.md b/pkgs/test/nixpkgs-check-by-name/scripts/README.md new file mode 100644 index 0000000000000..41b3012b7d953 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/scripts/README.md @@ -0,0 +1,26 @@ +# CI-related Scripts + +This directory contains scripts used and related to the CI running the `pkgs/by-name` checks in Nixpkgs. See also the [CI GitHub Action](../../../../.github/workflows/check-by-name.yml). + +## `./run-local.sh BASE_BRANCH [REPOSITORY]` + +Runs the `pkgs/by-name` check on the HEAD commit, closely matching what CI does. + +Note that this can't do exactly the same as CI, +because CI needs to rely on GitHub's server-side Git history to compute the mergeability of PRs before the check can be started. +In turn when running locally, we don't want to have to push commits to test them, +and we can also rely on the local Git history to do the mergeability check. + +Arguments: +- `BASE_BRANCH`: The base branch to use, e.g. master or release-23.11 +- `REPOSITORY`: The repository to fetch the base branch from, defaults to https://github.com/NixOS/nixpkgs.git + +## `./fetch-tool.sh BASE_BRANCH OUTPUT_PATH` + +Fetches the Hydra-prebuilt nixpkgs-check-by-name to use from the NixOS channel corresponding to the given base branch. + +This script is used both by [`./run-local.sh`](#run-local-sh-base-branch-repository) and CI. + +Arguments: +- `BASE_BRANCH`: The base branch to use, e.g. master or release-23.11 +- `OUTPUT_PATH`: The output symlink path for the tool diff --git a/pkgs/test/nixpkgs-check-by-name/scripts/fetch-tool.sh b/pkgs/test/nixpkgs-check-by-name/scripts/fetch-tool.sh new file mode 100755 index 0000000000000..19a48b6fb1fd5 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/scripts/fetch-tool.sh @@ -0,0 +1,47 @@ +#!/usr/bin/env bash +# Fetches the prebuilt nixpkgs-check-by-name to use from +# the NixOS channel corresponding to the given base branch + +set -o pipefail -o errexit -o nounset + +trace() { echo >&2 "$@"; } + +if (( $# < 2 )); then + trace "Usage: $0 BASE_BRANCH OUTPUT_PATH" + trace "BASE_BRANCH: The base branch to use, e.g. master or release-23.11" + trace "OUTPUT_PATH: The output symlink path for the tool" + exit 1 +fi +baseBranch=$1 +output=$2 + +trace -n "Determining the channel to use for PR base branch $baseBranch.. " +if [[ "$baseBranch" =~ ^(release|staging|staging-next)-([0-9][0-9]\.[0-9][0-9])$ ]]; then + # Use the release channel for all PRs to release-XX.YY, staging-XX.YY and staging-next-XX.YY + preferredChannel=nixos-${BASH_REMATCH[2]} +else + # Use the nixos-unstable channel for all other PRs + preferredChannel=nixos-unstable +fi + +# Check that the channel exists. It doesn't exist for fresh release branches +if curl -fSs "https://channels.nixos.org/$preferredChannel"; then + channel=$preferredChannel + trace "$channel" +else + # Fall back to nixos-unstable, makes sense for fresh release branches + channel=nixos-unstable + trace -e "\e[33mWarning: Preferred channel $preferredChannel could not be fetched, using fallback: $channel\e[0m" +fi + +trace -n "Fetching latest version of channel $channel.. " +# This is probably the easiest way to get Nix to output the path to a downloaded channel! +nixpkgs=$(nix-instantiate --find-file nixpkgs -I nixpkgs=channel:"$channel") +trace "$nixpkgs" + +# This file only exists in channels +trace -e "Git revision of channel $channel is \e[34m$(<"$nixpkgs/.git-revision")\e[0m" + +trace -n "Fetching the prebuilt version of nixpkgs-check-by-name.. " +nix-build -o "$output" "$nixpkgs" -A tests.nixpkgs-check-by-name -j 0 >/dev/null +realpath "$output" >&2 diff --git a/pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh b/pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh new file mode 100755 index 0000000000000..72d3e8dc3de39 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh @@ -0,0 +1,67 @@ +#!/usr/bin/env bash + +set -o pipefail -o errexit -o nounset + +trace() { echo >&2 "$@"; } + +tmp=$(mktemp -d) +cleanup() { + # Don't exit early if anything fails to cleanup + set +o errexit + + trace -n "Cleaning up.. " + + [[ -e "$tmp/base" ]] && git worktree remove --force "$tmp/base" + [[ -e "$tmp/merged" ]] && git worktree remove --force "$tmp/merged" + + rm -rf "$tmp" + + trace "Done" +} +trap cleanup exit + + +repo=https://github.com/NixOS/nixpkgs.git + +if (( $# != 0 )); then + baseBranch=$1 + shift +else + trace "Usage: $0 BASE_BRANCH [REPOSITORY]" + trace "BASE_BRANCH: The base branch to use, e.g. master or release-23.11" + trace "REPOSITORY: The repository to fetch the base branch from, defaults to $repo" + exit 1 +fi + +if (( $# != 0 )); then + repo=$1 + shift +fi + +if [[ -n "$(git status --porcelain)" ]]; then + trace -e "\e[33mWarning: Dirty tree, uncommitted changes won't be taken into account\e[0m" +fi +headSha=$(git rev-parse HEAD) +trace -e "Using HEAD commit \e[34m$headSha\e[0m" + +trace -n "Creating Git worktree for the HEAD commit in $tmp/merged.. " +git worktree add --detach -q "$tmp/merged" HEAD +trace "Done" + +trace -n "Fetching base branch $baseBranch to compare against.. " +git fetch -q "$repo" refs/heads/"$baseBranch" +baseSha=$(git rev-parse FETCH_HEAD) +trace -e "\e[34m$baseSha\e[0m" + +trace -n "Creating Git worktree for the base branch in $tmp/base.. " +git worktree add -q "$tmp/base" "$baseSha" +trace "Done" + +trace -n "Merging base branch into the HEAD commit in $tmp/merged.. " +git -C "$tmp/merged" merge -q --no-edit "$baseSha" +trace -e "\e[34m$(git -C "$tmp/merged" rev-parse HEAD)\e[0m" + +"$tmp/merged/pkgs/test/nixpkgs-check-by-name/scripts/fetch-tool.sh" "$baseBranch" "$tmp/tool" + +trace "Running nixpkgs-check-by-name.." +"$tmp/tool/bin/nixpkgs-check-by-name" --base "$tmp/base" "$tmp/merged" diff --git a/pkgs/test/nixpkgs-check-by-name/src/eval.rs b/pkgs/test/nixpkgs-check-by-name/src/eval.rs index 161d013374e7f..cd8c70472cf25 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/eval.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/eval.rs @@ -1,7 +1,7 @@ use crate::nixpkgs_problem::NixpkgsProblem; +use crate::ratchet; use crate::structure; use crate::validation::{self, Validation::Success}; -use crate::Version; use std::path::Path; use anyhow::Context; @@ -39,11 +39,10 @@ enum AttributeVariant { /// of the form `callPackage <package_file> { ... }`. /// See the `eval.nix` file for how this is achieved on the Nix side pub fn check_values( - version: Version, nixpkgs_path: &Path, package_names: Vec<String>, - eval_accessible_paths: Vec<&Path>, -) -> validation::Result<()> { + eval_accessible_paths: &[&Path], +) -> validation::Result<ratchet::Nixpkgs> { // Write the list of packages we need to check into a temporary JSON file. // This can then get read by the Nix evaluation. let attrs_file = NamedTempFile::new().context("Failed to create a temporary file")?; @@ -110,47 +109,58 @@ pub fn check_values( String::from_utf8_lossy(&result.stdout) ))?; - Ok(validation::sequence_(package_names.iter().map( - |package_name| { - let relative_package_file = structure::relative_file_for_package(package_name); + Ok( + validation::sequence(package_names.into_iter().map(|package_name| { + let relative_package_file = structure::relative_file_for_package(&package_name); let absolute_package_file = nixpkgs_path.join(&relative_package_file); - if let Some(attribute_info) = actual_files.get(package_name) { - let valid = match &attribute_info.variant { - AttributeVariant::AutoCalled => true, + if let Some(attribute_info) = actual_files.get(&package_name) { + let check_result = if !attribute_info.is_derivation { + NixpkgsProblem::NonDerivation { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } else { + Success(()) + }; + + let check_result = check_result.and(match &attribute_info.variant { + AttributeVariant::AutoCalled => Success(ratchet::Package { + empty_non_auto_called: ratchet::EmptyNonAutoCalled::Valid, + }), AttributeVariant::CallPackage { path, empty_arg } => { let correct_file = if let Some(call_package_path) = path { absolute_package_file == *call_package_path } else { false }; - // Only check for the argument to be non-empty if the version is V1 or - // higher - let non_empty = if version >= Version::V1 { - !empty_arg - } else { - true - }; - correct_file && non_empty - } - AttributeVariant::Other => false, - }; - if !valid { - NixpkgsProblem::WrongCallPackage { - relative_package_file: relative_package_file.clone(), - package_name: package_name.clone(), + if correct_file { + Success(ratchet::Package { + // Empty arguments for non-auto-called packages are not allowed anymore. + empty_non_auto_called: if *empty_arg { + ratchet::EmptyNonAutoCalled::Invalid + } else { + ratchet::EmptyNonAutoCalled::Valid + }, + }) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: relative_package_file.clone(), + package_name: package_name.clone(), + } + .into() + } } - .into() - } else if !attribute_info.is_derivation { - NixpkgsProblem::NonDerivation { + AttributeVariant::Other => NixpkgsProblem::WrongCallPackage { relative_package_file: relative_package_file.clone(), package_name: package_name.clone(), } - .into() - } else { - Success(()) - } + .into(), + }); + + check_result.map(|value| (package_name.clone(), value)) } else { NixpkgsProblem::UndefinedAttr { relative_package_file: relative_package_file.clone(), @@ -158,6 +168,9 @@ pub fn check_values( } .into() } - }, - ))) + })) + .map(|elems| ratchet::Nixpkgs { + packages: elems.into_iter().collect(), + }), + ) } diff --git a/pkgs/test/nixpkgs-check-by-name/src/main.rs b/pkgs/test/nixpkgs-check-by-name/src/main.rs index 4cabf8f446f56..18c950d0a6eb0 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/main.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/main.rs @@ -1,5 +1,6 @@ mod eval; mod nixpkgs_problem; +mod ratchet; mod references; mod structure; mod utils; @@ -9,37 +10,43 @@ use crate::structure::check_structure; use crate::validation::Validation::Failure; use crate::validation::Validation::Success; use anyhow::Context; -use clap::{Parser, ValueEnum}; +use clap::Parser; use colored::Colorize; use std::io; use std::path::{Path, PathBuf}; use std::process::ExitCode; /// Program to check the validity of pkgs/by-name +/// +/// This CLI interface may be changed over time if the CI workflow making use of +/// it is adjusted to deal with the change appropriately. +/// +/// Exit code: +/// - `0`: If the validation is successful +/// - `1`: If the validation is not successful +/// - `2`: If an unexpected I/O error occurs +/// +/// Standard error: +/// - Informative messages +/// - Detected problems if validation is not successful #[derive(Parser, Debug)] -#[command(about)] +#[command(about, verbatim_doc_comment)] pub struct Args { - /// Path to nixpkgs + /// Path to the main Nixpkgs to check. + /// For PRs, this should be set to a checkout of the PR branch. nixpkgs: PathBuf, - /// The version of the checks - /// Increasing this may cause failures for a Nixpkgs that succeeded before - /// TODO: Remove default once Nixpkgs CI passes this argument - #[arg(long, value_enum, default_value_t = Version::V0)] - version: Version, -} -/// The version of the checks -#[derive(Debug, Clone, PartialEq, PartialOrd, ValueEnum)] -pub enum Version { - /// Initial version - V0, - /// Empty argument check - V1, + /// Path to the base Nixpkgs to run ratchet checks against. + /// For PRs, this should be set to a checkout of the PRs base branch. + /// If not specified, no ratchet checks will be performed. + /// However, this flag will become required once CI uses it. + #[arg(long)] + base: Option<PathBuf>, } fn main() -> ExitCode { let args = Args::parse(); - match check_nixpkgs(&args.nixpkgs, args.version, vec![], &mut io::stderr()) { + match process(args.base.as_deref(), &args.nixpkgs, &[], &mut io::stderr()) { Ok(true) => { eprintln!("{}", "Validated successfully".green()); ExitCode::SUCCESS @@ -55,10 +62,11 @@ fn main() -> ExitCode { } } -/// Checks whether the pkgs/by-name structure in Nixpkgs is valid. +/// Does the actual work. This is the abstraction used both by `main` and the tests. /// /// # Arguments -/// - `nixpkgs_path`: The path to the Nixpkgs to check +/// - `base_nixpkgs`: Path to the base Nixpkgs to run ratchet checks against. +/// - `main_nixpkgs`: Path to the main Nixpkgs to check. /// - `eval_accessible_paths`: /// Extra paths that need to be accessible to evaluate Nixpkgs using `restrict-eval`. /// This is used to allow the tests to access the mock-nixpkgs.nix file @@ -68,33 +76,30 @@ fn main() -> ExitCode { /// - `Err(e)` if an I/O-related error `e` occurred. /// - `Ok(false)` if there are problems, all of which will be written to `error_writer`. /// - `Ok(true)` if there are no problems -pub fn check_nixpkgs<W: io::Write>( - nixpkgs_path: &Path, - version: Version, - eval_accessible_paths: Vec<&Path>, +pub fn process<W: io::Write>( + base_nixpkgs: Option<&Path>, + main_nixpkgs: &Path, + eval_accessible_paths: &[&Path], error_writer: &mut W, ) -> anyhow::Result<bool> { - let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( - "Nixpkgs path {} could not be resolved", - nixpkgs_path.display() - ))?; - - let check_result = if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { - eprintln!( - "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", - utils::BASE_SUBPATH - ); - Success(()) - } else { - match check_structure(&nixpkgs_path)? { - Failure(errors) => Failure(errors), - Success(package_names) => - // Only if we could successfully parse the structure, we do the evaluation checks - { - eval::check_values(version, &nixpkgs_path, package_names, eval_accessible_paths)? - } + // Check the main Nixpkgs first + let main_result = check_nixpkgs(main_nixpkgs, eval_accessible_paths, error_writer)?; + let check_result = main_result.result_map(|nixpkgs_version| { + // If the main Nixpkgs doesn't have any problems, run the ratchet checks against the base + // Nixpkgs + if let Some(base) = base_nixpkgs { + check_nixpkgs(base, eval_accessible_paths, error_writer)?.result_map( + |base_nixpkgs_version| { + Ok(ratchet::Nixpkgs::compare( + Some(base_nixpkgs_version), + nixpkgs_version, + )) + }, + ) + } else { + Ok(ratchet::Nixpkgs::compare(None, nixpkgs_version)) } - }; + })?; match check_result { Failure(errors) => { @@ -103,15 +108,45 @@ pub fn check_nixpkgs<W: io::Write>( } Ok(false) } - Success(_) => Ok(true), + Success(()) => Ok(true), } } +/// Checks whether the pkgs/by-name structure in Nixpkgs is valid. +/// +/// This does not include ratchet checks, see ../README.md#ratchet-checks +/// Instead a `ratchet::Nixpkgs` value is returned, whose `compare` method allows performing the +/// ratchet check against another result. +pub fn check_nixpkgs<W: io::Write>( + nixpkgs_path: &Path, + eval_accessible_paths: &[&Path], + error_writer: &mut W, +) -> validation::Result<ratchet::Nixpkgs> { + Ok({ + let nixpkgs_path = nixpkgs_path.canonicalize().context(format!( + "Nixpkgs path {} could not be resolved", + nixpkgs_path.display() + ))?; + + if !nixpkgs_path.join(utils::BASE_SUBPATH).exists() { + writeln!( + error_writer, + "Given Nixpkgs path does not contain a {} subdirectory, no check necessary.", + utils::BASE_SUBPATH + )?; + Success(ratchet::Nixpkgs::default()) + } else { + check_structure(&nixpkgs_path)?.result_map(|package_names| + // Only if we could successfully parse the structure, we do the evaluation checks + eval::check_values(&nixpkgs_path, package_names, eval_accessible_paths))? + } + }) +} + #[cfg(test)] mod tests { - use crate::check_nixpkgs; + use crate::process; use crate::utils; - use crate::Version; use anyhow::Context; use std::fs; use std::path::Path; @@ -197,10 +232,17 @@ mod tests { fn test_nixpkgs(name: &str, path: &Path, expected_errors: &str) -> anyhow::Result<()> { let extra_nix_path = Path::new("tests/mock-nixpkgs.nix"); + let base_path = path.join("base"); + let base_nixpkgs = if base_path.exists() { + Some(base_path.as_path()) + } else { + None + }; + // We don't want coloring to mess up the tests let writer = temp_env::with_var("NO_COLOR", Some("1"), || -> anyhow::Result<_> { let mut writer = vec![]; - check_nixpkgs(&path, Version::V1, vec![&extra_nix_path], &mut writer) + process(base_nixpkgs, &path, &[&extra_nix_path], &mut writer) .context(format!("Failed test case {name}"))?; Ok(writer) })?; diff --git a/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs new file mode 100644 index 0000000000000..c12f1ead25402 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/src/ratchet.rs @@ -0,0 +1,85 @@ +//! This module implements the ratchet checks, see ../README.md#ratchet-checks +//! +//! Each type has a `compare` method that validates the ratchet checks for that item. + +use crate::nixpkgs_problem::NixpkgsProblem; +use crate::structure; +use crate::validation::{self, Validation, Validation::Success}; +use std::collections::HashMap; + +/// The ratchet value for the entirety of Nixpkgs. +#[derive(Default)] +pub struct Nixpkgs { + /// The ratchet values for each package in `pkgs/by-name` + pub packages: HashMap<String, Package>, +} + +impl Nixpkgs { + /// Validates the ratchet checks for Nixpkgs + pub fn compare(optional_from: Option<Self>, to: Self) -> Validation<()> { + validation::sequence_( + // We only loop over the current attributes, + // we don't need to check ones that were removed + to.packages.into_iter().map(|(name, attr_to)| { + let attr_from = if let Some(from) = &optional_from { + from.packages.get(&name) + } else { + // This pretends that if there's no base version to compare against, all + // attributes existed without conforming to the new strictness check for + // backwards compatibility. + // TODO: Remove this case. This is only needed because the `--base` + // argument is still optional, which doesn't need to be once CI is updated + // to pass it. + Some(&Package { + empty_non_auto_called: EmptyNonAutoCalled::Invalid, + }) + }; + Package::compare(&name, attr_from, &attr_to) + }), + ) + } +} + +/// The ratchet value for a single package in `pkgs/by-name` +pub struct Package { + /// The ratchet value for the check for non-auto-called empty arguments + pub empty_non_auto_called: EmptyNonAutoCalled, +} + +impl Package { + /// Validates the ratchet checks for a single package defined in `pkgs/by-name` + pub fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { + EmptyNonAutoCalled::compare( + name, + optional_from.map(|x| &x.empty_non_auto_called), + &to.empty_non_auto_called, + ) + } +} + +/// The ratchet value of a single package in `pkgs/by-name` +/// for the non-auto-called empty argument check of a single. +/// +/// This checks that packages defined in `pkgs/by-name` cannot be overridden +/// with an empty second argument like `callPackage ... { }`. +#[derive(PartialEq, PartialOrd)] +pub enum EmptyNonAutoCalled { + Invalid, + Valid, +} + +impl EmptyNonAutoCalled { + /// Validates the non-auto-called empty argument ratchet check for a single package defined in `pkgs/by-name` + fn compare(name: &str, optional_from: Option<&Self>, to: &Self) -> Validation<()> { + let from = optional_from.unwrap_or(&Self::Valid); + if to >= from { + Success(()) + } else { + NixpkgsProblem::WrongCallPackage { + relative_package_file: structure::relative_file_for_package(name), + package_name: name.to_owned(), + } + .into() + } + } +} diff --git a/pkgs/test/nixpkgs-check-by-name/src/validation.rs b/pkgs/test/nixpkgs-check-by-name/src/validation.rs index e727938515218..b14bfb92eb2e3 100644 --- a/pkgs/test/nixpkgs-check-by-name/src/validation.rs +++ b/pkgs/test/nixpkgs-check-by-name/src/validation.rs @@ -58,6 +58,15 @@ impl<A> Validation<A> { Success(value) => Success(f(value)), } } + + /// Map a `Validation<A>` to a `Result<B>` by applying a function `A -> Result<B>` + /// only if there is a `Success` value + pub fn result_map<B>(self, f: impl FnOnce(A) -> Result<B>) -> Result<B> { + match self { + Failure(err) => Ok(Failure(err)), + Success(value) => f(value), + } + } } impl Validation<()> { diff --git a/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected new file mode 100644 index 0000000000000..ddcb2df46e5fb --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/no-by-name/expected @@ -0,0 +1 @@ +Given Nixpkgs path does not contain a pkgs/by-name subdirectory, no check necessary. diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix new file mode 100644 index 0000000000000..d369dd7228dca --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix new file mode 100644 index 0000000000000..d369dd7228dca --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/all-packages.nix @@ -0,0 +1,3 @@ +self: super: { + nonDerivation = self.callPackage ./pkgs/by-name/no/nonDerivation/package.nix { }; +} diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix new file mode 100644 index 0000000000000..2875ea6327ef2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/default.nix @@ -0,0 +1 @@ +import ../../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix new file mode 100644 index 0000000000000..a1b92efbbadb9 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/base/pkgs/by-name/no/nonDerivation/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix new file mode 100644 index 0000000000000..af25d1450122b --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/default.nix @@ -0,0 +1 @@ +import ../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected new file mode 100644 index 0000000000000..e69de29bb2d1d --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/expected diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix new file mode 100644 index 0000000000000..a1b92efbbadb9 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg-gradual/pkgs/by-name/no/nonDerivation/package.nix @@ -0,0 +1 @@ +{ someDrv }: someDrv diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix new file mode 100644 index 0000000000000..2875ea6327ef2 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/default.nix @@ -0,0 +1 @@ +import ../../mock-nixpkgs.nix { root = ./.; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md new file mode 100644 index 0000000000000..b0d2b34e338a8 --- /dev/null +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-empty-arg/base/pkgs/by-name/README.md @@ -0,0 +1 @@ +(this is just here so the directory can get tracked by git) diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix index 4fad280ae1c73..853c3a87db561 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-call-package/all-packages.nix @@ -1,3 +1,3 @@ self: super: { - nonDerivation = null; + nonDerivation = self.someDrv; } diff --git a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix index 4c521d2d44684..dc07f69b40ee4 100644 --- a/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix +++ b/pkgs/test/nixpkgs-check-by-name/tests/override-no-file/all-packages.nix @@ -1,3 +1,3 @@ self: super: { - nonDerivation = self.callPackage ({ }: { }) { }; + nonDerivation = self.callPackage ({ someDrv }: someDrv) { }; } diff --git a/pkgs/test/release/default.nix b/pkgs/test/release/default.nix new file mode 100644 index 0000000000000..f112ee6b92124 --- /dev/null +++ b/pkgs/test/release/default.nix @@ -0,0 +1,46 @@ +# Adapted from lib/tests/release.nix +{ pkgs-path ? ../../.. +, pkgs ? import pkgs-path {} +, lib ? pkgs.lib +, nix ? pkgs.nix +}: + +# +# This verifies that release-attrpaths-superset.nix does not encounter +# infinite recursion or non-tryEval-able failures. +# +pkgs.runCommand "all-attrs-eval-under-tryEval" { + nativeBuildInputs = [ + nix + pkgs.gitMinimal + ] ++ lib.optional pkgs.stdenv.isLinux pkgs.inotify-tools; + strictDeps = true; +} +'' + datadir="${nix}/share" + export TEST_ROOT=$(pwd)/test-tmp + export HOME=$(mktemp -d) + export NIX_BUILD_HOOK= + export NIX_CONF_DIR=$TEST_ROOT/etc + export NIX_LOCALSTATE_DIR=$TEST_ROOT/var + export NIX_LOG_DIR=$TEST_ROOT/var/log/nix + export NIX_STATE_DIR=$TEST_ROOT/var/nix + export NIX_STORE_DIR=$TEST_ROOT/store + export PAGER=cat + cacheDir=$TEST_ROOT/binary-cache + + nix-store --init + + cp -r ${pkgs-path + "/lib"} lib + cp -r ${pkgs-path + "/pkgs"} pkgs + cp -r ${pkgs-path + "/default.nix"} default.nix + cp -r ${pkgs-path + "/nixos"} nixos + cp -r ${pkgs-path + "/maintainers"} maintainers + cp -r ${pkgs-path + "/.version"} .version + cp -r ${pkgs-path + "/doc"} doc + echo "Running pkgs/top-level/release-attrpaths-superset.nix" + nix-instantiate --eval --strict --json pkgs/top-level/release-attrpaths-superset.nix -A names > /dev/null + + mkdir $out + echo success > $out/${nix.version} +'' |