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

ci/nixpkgs-vet: 0.1.2 -> 0.1.4 #339119

Merged
merged 2 commits into from
Sep 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions .github/CODEOWNERS
Validating CODEOWNERS rules …
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,10 @@
# CI
/.github/workflows @NixOS/Security @Mic92 @zowoq
/.github/workflows/check-nix-format.yml @infinisil
/ci @infinisil @NixOS/Security
/.github/workflows/nixpkgs-vet.yml @infinisil @philiptaron
/ci @infinisil @philiptaron @NixOS/Security

# Develompent support
# Development support
/.editorconfig @Mic92 @zowoq
/shell.nix @infinisil @NixOS/Security

Expand All @@ -43,6 +44,7 @@
/pkgs/top-level/stage.nix @Ericson2314
/pkgs/top-level/splice.nix @Ericson2314
/pkgs/top-level/release-cross.nix @Ericson2314
/pkgs/top-level/by-name-overlay.nix @infinisil @philiptaron
/pkgs/stdenv @philiptaron
/pkgs/stdenv/generic @Ericson2314
/pkgs/stdenv/generic/check-meta.nix @Ericson2314
Expand All @@ -58,12 +60,6 @@
/pkgs/pkgs-lib/formats/libconfig @h7x4
/pkgs/pkgs-lib/formats/hocon @h7x4

# pkgs/by-name
/pkgs/test/check-by-name @infinisil
/pkgs/by-name/README.md @infinisil
/pkgs/top-level/by-name-overlay.nix @infinisil
/.github/workflows/check-by-name.yml @infinisil

# Nixpkgs build-support
/pkgs/build-support/writers @lassulus @Profpatsch

Expand Down Expand Up @@ -91,6 +87,7 @@ nixos/modules/installer/tools/nix-fallback-paths.nix @NixOS/nix-team @raitobeza
/doc/README.md @infinisil
/nixos/README.md @infinisil
/pkgs/README.md @infinisil
/pkgs/by-name/README.md @infinisil
/maintainers/README.md @infinisil

# User-facing development documentation
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/check-nix-format.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ name: Check that Nix files are formatted

on:
pull_request_target:
# See the comment at the same location in ./check-by-name.yml
# See the comment at the same location in ./nixpkgs-vet.yml
types: [opened, synchronize, reopened, edited]
permissions:
contents: read
Expand Down
Original file line number Diff line number Diff line change
@@ -1,42 +1,30 @@
# Checks pkgs/by-name (see pkgs/by-name/README.md)
# using the nixpkgs-check-by-name tool (see https://github.com/NixOS/nixpkgs-check-by-name)
#
# When you make changes to this workflow, also update pkgs/test/check-by-name/run-local.sh adequately
# Checks pkgs/by-name (see pkgs/by-name/README.md) using the `nixpkgs-vet` tool (see https://github.com/NixOS/nixpkgs-vet)
# When you make changes to this workflow, please also update `ci/nixpkgs-vet.sh` to reflect the impact of your work to the CI.
name: Check pkgs/by-name

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh just noticed that this would also be good to change

on:
# Using pull_request_target instead of pull_request avoids having to approve first time contributors
# Using pull_request_target instead of pull_request avoids having to approve first time contributors.
pull_request_target:
# This workflow depends on the base branch of the PR,
# but changing the base branch is not included in the default trigger events,
# which would be `opened`, `synchronize` or `reopened`.
# Instead it causes an `edited` event, so we need to add it explicitly here
# While `edited` is also triggered when the PR title/body is changed,
# this PR action is fairly quick, and PR's don't get edited that often,
# so it shouldn't be a problem
# There is a feature request for adding a `base_changed` event:
# https://github.com/orgs/community/discussions/35058
# This workflow depends on the base branch of the PR, but changing the base branch is not included in the default trigger events, which would be `opened`, `synchronize` or `reopened`.
# Instead it causes an `edited` event, so we need to add it explicitly here.
# While `edited` is also triggered when the PR title/body is changed, this PR action is fairly quick, and PRs don't get edited **that** often, so it shouldn't be a problem.
# There is a feature request for adding a `base_changed` event: https://github.com/orgs/community/discussions/35058
types: [opened, synchronize, reopened, edited]

permissions: {}

# We don't use a concurrency group here, because the action is triggered quite often (due to the PR edit
# trigger), and contributers would get notified on any canceled run.
# There is a feature request for supressing notifications on concurrency-canceled runs:
# https://github.com/orgs/community/discussions/13015
# We don't use a concurrency group here, because the action is triggered quite often (due to the PR edit trigger), and contributors would get notified on any canceled run.
# There is a feature request for suppressing notifications on concurrency-canceled runs: https://github.com/orgs/community/discussions/13015

jobs:
check:
name: pkgs-by-name-check
# This needs to be x86_64-linux, because we depend on the tooling being pre-built in the GitHub releases
# This needs to be x86_64-linux, because we depend on the tooling being pre-built in the GitHub releases.
runs-on: ubuntu-latest
# This should take 1 minute at most, but let's be generous.
# The default of 6 hours is definitely too long
# This should take 1 minute at most, but let's be generous. The default of 6 hours is definitely too long.
timeout-minutes: 10
steps:
# This step has to be in this file,
# because it's needed to determine which revision of the repository to fetch,
# and we can only use other files from the repository once it's fetched.
# This step has to be in this file, because it's needed to determine which revision of the repository to fetch, and we can only use other files from the repository once it's fetched.
- name: Resolving the merge commit
env:
GH_TOKEN: ${{ github.token }}
Expand Down Expand Up @@ -99,27 +87,28 @@ jobs:
if: env.mergedSha
- name: Fetching the pinned tool
if: env.mergedSha
# Update the pinned version using pkgs/test/check-by-name/update-pinned-tool.sh
# Update the pinned version using ci/nixpkgs-vet/update-pinned-tool.sh
run: |
# The pinned version of the tooling to use
toolVersion=$(<pkgs/test/check-by-name/pinned-version.txt)
# Fetch the x86_64-linux-specific release artifact containing the Gzipped NAR of the pre-built tool
toolPath=$(curl -sSfL https://github.com/NixOS/nixpkgs-check-by-name/releases/download/"$toolVersion"/x86_64-linux.nar.gz \
# The pinned version of the tooling to use.
toolVersion=$(<ci/nixpkgs-vet/pinned-version.txt)

# Fetch the x86_64-linux-specific release artifact containing the gzipped NAR of the pre-built tool.
toolPath=$(curl -sSfL https://github.com/NixOS/nixpkgs-vet/releases/download/"$toolVersion"/x86_64-linux.nar.gz \
| gzip -cd | nix-store --import | tail -1)
# Adds a result symlink as a GC root

# Adds a result symlink as a GC root.
nix-store --realise "$toolPath" --add-root result
- name: Running nixpkgs-check-by-name
- name: Running nixpkgs-vet
if: env.mergedSha
env:
# Force terminal colors to be enabled. The library that
# nixpkgs-check-by-name uses respects: https://bixense.com/clicolors/
# Force terminal colors to be enabled. The library that `nixpkgs-vet` uses respects https://bixense.com/clicolors/
CLICOLOR_FORCE: 1
run: |
if result/bin/nixpkgs-check-by-name --base "$base" .; then
if result/bin/nixpkgs-vet --base "$base" .; then
exit 0
else
exitCode=$?
echo "To run locally: ./maintainers/scripts/check-by-name.sh $GITHUB_BASE_REF https://github.com/$GITHUB_REPOSITORY.git"
echo "If you're having trouble, ping @NixOS/nixpkgs-check-by-name"
echo "To run locally: ./ci/nixpkgs-vet.sh $GITHUB_BASE_REF https://github.com/$GITHUB_REPOSITORY.git"
echo "If you're having trouble, ping @NixOS/nixpkgs-vet"
exit "$exitCode"
fi
31 changes: 31 additions & 0 deletions ci/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,3 +10,34 @@ In order to ensure that the needed packages are generally available without buil
[`pinned-nixpkgs.json`](./pinned-nixpkgs.json) contains a pinned Nixpkgs version tested by Hydra.

Run [`update-pinned-nixpkgs.sh`](./update-pinned-nixpkgs.sh) to update it.

## `ci/nixpkgs-vet.sh BASE_BRANCH [REPOSITORY]`

Runs the [`nixpkgs-vet` tool](https://github.com/NixOS/nixpkgs-vet) on the HEAD commit, closely matching what CI does. 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 contributors are running this tool 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-24.05
- `REPOSITORY`: The repository from which to fetch the base branch. Defaults to <https://github.com/NixOS/nixpkgs.git>.

## `ci/nixpkgs-vet`

This directory contains scripts and files used and related to [`nixpkgs-vet`](https://github.com/NixOS/nixpkgs-vet/), which the CI uses to implement `pkgs/by-name` checks, along with many other Nixpkgs architecture rules.
See also the [CI GitHub Action](../.github/workflows/nixpkgs-vet.yml).

## `ci/nixpkgs-vet/update-pinned-tool.sh`

Updates the pinned [`nixpkgs-vet` tool](https://github.com/NixOS/nixpkgs-vet) in [`ci/nixpkgs-vet/pinned-version.txt`](./nixpkgs-vet/pinned-version.txt) to the latest [release](https://github.com/NixOS/nixpkgs-vet/releases).

Each release contains a pre-built `x86_64-linux` version of the tool which is used by CI.

This script currently needs to be called manually when the CI tooling needs to be updated.

Why not just build the tooling right from the PRs Nixpkgs version?

- Because it allows CI to check all PRs, even if they would break the CI tooling.
- Because it makes the CI check very fast, since no Nix builds need to be done, even for mass rebuilds.
- Because it improves security, since we don't have to build potentially untrusted code from PRs.
The tool only needs a very minimal Nix evaluation at runtime, which can work with [readonly-mode](https://nixos.org/manual/nix/stable/command-ref/opt-common.html#opt-readonly-mode) and [restrict-eval](https://nixos.org/manual/nix/stable/command-ref/conf-file.html#conf-restrict-eval).

12 changes: 5 additions & 7 deletions pkgs/test/check-by-name/run-local.sh → ci/nixpkgs-vet.sh
Original file line number Diff line number Diff line change
Expand Up @@ -61,13 +61,11 @@ 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"

trace -n "Reading pinned nixpkgs-check-by-name version from pinned-version.txt.. "
toolVersion=$(<"$tmp/merged/pkgs/test/check-by-name/pinned-version.txt")
trace -n "Reading pinned nixpkgs-vet version from pinned-version.txt.. "
toolVersion=$(<"$tmp/merged/ci/nixpkgs-vet/pinned-version.txt")
trace -e "\e[34m$toolVersion\e[0m"

trace -n "Building tool.. "
nix-build https://github.com/NixOS/nixpkgs-check-by-name/tarball/"$toolVersion" -o "$tmp/tool" -A build

trace "Running nixpkgs-check-by-name.."
"$tmp/tool/bin/nixpkgs-check-by-name" --base "$tmp/base" "$tmp/merged"
nix-build https://github.com/NixOS/nixpkgs-vet/tarball/"$toolVersion" -o "$tmp/tool" -A build
trace "Running nixpkgs-vet.."
"$tmp/tool/bin/nixpkgs-vet" --base "$tmp/base" "$tmp/merged"
1 change: 1 addition & 0 deletions ci/nixpkgs-vet/pinned-version.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
0.1.4
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ trace() { echo >&2 "$@"; }

SCRIPT_DIR=$( cd -- "$( dirname -- "${BASH_SOURCE[0]}" )" &> /dev/null && pwd )

repository=NixOS/nixpkgs-check-by-name
repository=NixOS/nixpkgs-vet
pin_file=$SCRIPT_DIR/pinned-version.txt

trace -n "Fetching latest release of $repository.. "
Expand Down
4 changes: 0 additions & 4 deletions maintainers/scripts/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,6 @@ What follows is a (very incomplete) overview of available scripts.

## Metadata

### `check-by-name.sh`

An alias for `pkgs/test/check-by-name/run-local.sh`, see [documentation](../../pkgs/test/check-by-name/README.md).

### `get-maintainer.sh`

`get-maintainer.sh [selector] value` returns a JSON object describing
Expand Down
2 changes: 1 addition & 1 deletion maintainers/scripts/check-by-name.sh
philiptaron marked this conversation as resolved.
Show resolved Hide resolved
8 changes: 4 additions & 4 deletions pkgs/by-name/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -110,16 +110,16 @@ There's some limitations as to which packages can be defined using this structur

## Validation

CI performs [certain checks](https://github.com/NixOS/nixpkgs-check-by-name?tab=readme-ov-file#validity-checks) on the `pkgs/by-name` structure.
This is done using the [`nixpkgs-check-by-name` tool](https://github.com/NixOS/nixpkgs-check-by-name).
CI performs [certain checks](https://github.com/NixOS/nixpkgs-vet?tab=readme-ov-file#validity-checks) on the `pkgs/by-name` structure.
This is done using the [`nixpkgs-vet` tool](https://github.com/NixOS/nixpkgs-vet).

You can locally emulate the CI check using

```
$ ./maintainers/scripts/check-by-name.sh master
$ ./ci/nixpkgs-vet.sh master
```

See [here](../../.github/workflows/check-by-name.yml) for more info.
See [here](../../.github/workflows/nixpkgs-vet.yml) for more info.

## Recommendation for new packages with multiple versions

Expand Down
2 changes: 1 addition & 1 deletion pkgs/by-name/ss/ssh-openpgp-auth/daemon.nix
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Ideally, this file would have been placed in
# pkgs/by-name/ss/sshd-openpgp-auth/package.nix, but since `./generic.nix` is
# outside of the directory, the nixpkgs-check-by-name test will fail the CI. So
# outside of the directory, the `nixpkgs-vet` test will fail the CI. So
# we call this file in all-packages.nix like in the old days.
{ callPackage }:

Expand Down
31 changes: 0 additions & 31 deletions pkgs/test/check-by-name/README.md

This file was deleted.

Loading