diff --git a/.github/workflows/eval.yml b/.github/workflows/eval.yml index b037b29c9558..7c218d60a0bc 100644 --- a/.github/workflows/eval.yml +++ b/.github/workflows/eval.yml @@ -202,7 +202,6 @@ jobs: - name: Compare against the target branch env: - AUTHOR_ID: ${{ github.event.pull_request.user.id }} TARGET_SHA: ${{ inputs.mergedSha }} run: | git -C nixpkgs/trusted diff --name-only "$TARGET_SHA" \ @@ -212,7 +211,6 @@ jobs: nix-build nixpkgs/trusted/ci --arg nixpkgs ./nixpkgs/trusted-pinned -A eval.compare \ --arg combinedDir ./combined \ --arg touchedFilesJson ./touched-files.json \ - --argstr githubAuthorId "$AUTHOR_ID" \ --out-link comparison cat comparison/step-summary.md >> "$GITHUB_STEP_SUMMARY" diff --git a/.github/workflows/labels.yml b/.github/workflows/labels.yml index 74838ebb52ae..e3dfe2642e50 100644 --- a/.github/workflows/labels.yml +++ b/.github/workflows/labels.yml @@ -39,10 +39,6 @@ jobs: update: runs-on: ubuntu-24.04-arm if: github.event_name != 'schedule' || github.repository_owner == 'NixOS' - env: - # TODO: Remove after 2026-03-04, when Node 24 becomes the default. - # https://github.blog/changelog/2025-09-19-deprecation-of-node-20-on-github-actions-runners/ - FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: "true" steps: - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 with: diff --git a/ci/eval/compare/default.nix b/ci/eval/compare/default.nix index d0820618a0a0..eabc28af4d36 100644 --- a/ci/eval/compare/default.nix +++ b/ci/eval/compare/default.nix @@ -48,7 +48,6 @@ in { combinedDir, touchedFilesJson, - githubAuthorId, }: let # Usually we expect a derivation, but when evaluating in multiple separate steps, we pass @@ -155,22 +154,19 @@ let # Only set this label when no other label with indication for staging has been set. # This avoids confusion whether to target staging or batch this with kernel updates. lib.last (lib.sort lib.lessThan (lib.attrValues rebuildCountByKernel)) <= 500; - # Set the "11.by: package-maintainer" label to whether all packages directly - # changed are maintained by the PR's author. - "11.by: package-maintainer" = - maintainers ? ${githubAuthorId} - && lib.all (lib.flip lib.elem maintainers.${githubAuthorId}) ( - lib.flatten (lib.attrValues maintainers) - ); }; } ); - maintainers = callPackage ./maintainers.nix { } { - changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs); - changedpathsjson = touchedFilesJson; - removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs); - }; + inherit + (callPackage ./maintainers.nix { } { + changedattrs = lib.attrNames (lib.groupBy (a: a.name) changedPackagePlatformAttrs); + changedpathsjson = touchedFilesJson; + removedattrs = lib.attrNames (lib.groupBy (a: a.name) removedPackagePlatformAttrs); + }) + maintainers + packages + ; in runCommand "compare" { @@ -180,7 +176,11 @@ runCommand "compare" cmp-stats ]; maintainers = builtins.toJSON maintainers; - passAsFile = [ "maintainers" ]; + packages = builtins.toJSON packages; + passAsFile = [ + "maintainers" + "packages" + ]; } '' mkdir $out @@ -223,4 +223,5 @@ runCommand "compare" fi cp "$maintainersPath" "$out/maintainers.json" + cp "$packagesPath" "$out/packages.json" '' diff --git a/ci/eval/compare/maintainers.nix b/ci/eval/compare/maintainers.nix index 9f19a4c382eb..7f945f2c387f 100644 --- a/ci/eval/compare/maintainers.nix +++ b/ci/eval/compare/maintainers.nix @@ -105,9 +105,9 @@ let ) attrsWithModifiedFiles; byMaintainer = lib.groupBy (ping: toString ping.id) listToPing; - - packagesPerMaintainer = lib.mapAttrs ( - maintainer: packages: map (pkg: pkg.packageName) packages - ) byMaintainer; in -packagesPerMaintainer +{ + maintainers = lib.mapAttrs (_: lib.catAttrs "packageName") byMaintainer; + + packages = lib.catAttrs "packageName" listToPing; +} diff --git a/ci/eval/default.nix b/ci/eval/default.nix index 75b2eb9cba5d..b753c42c2aef 100644 --- a/ci/eval/default.nix +++ b/ci/eval/default.nix @@ -281,9 +281,6 @@ let chunkSize ? 5000, quickTest ? false, baseline, - # Which maintainer should be considered the author? - # Defaults to nixpkgs-ci which is not a maintainer and skips the check. - githubAuthorId ? "nixpkgs-ci", # What files have been touched? Defaults to none; use the expression below to calculate it. # ``` # git diff --name-only --merge-base master HEAD \ @@ -307,7 +304,7 @@ let }; comparisonReport = compare { combinedDir = combine { diffDir = diffs; }; - inherit touchedFilesJson githubAuthorId; + inherit touchedFilesJson; }; in comparisonReport; diff --git a/ci/github-script/labels.js b/ci/github-script/labels.js index 2d7bafb6c5ae..a435e6e68bf5 100644 --- a/ci/github-script/labels.js +++ b/ci/github-script/labels.js @@ -3,9 +3,98 @@ module.exports = async ({ github, context, core, dry }) => { const { DefaultArtifactClient } = require('@actions/artifact') const { readFile, writeFile } = require('node:fs/promises') const withRateLimit = require('./withRateLimit.js') + const { classify } = require('../supportedBranches.js') const artifactClient = new DefaultArtifactClient() + async function downloadMaintainerMap(branch) { + let run + + const commits = ( + await github.rest.repos.listCommits({ + ...context.repo, + sha: branch, + // We look at 10 commits to find a maintainer map, but this is an arbitrary number. The + // head commit might not have a map, if the queue was bypassed to merge it. This happens + // frequently on staging-esque branches. The branch with the highest chance of getting + // 10 consecutive bypassing commits is the stable staging-next branch. Luckily, this + // also means that the number of PRs open towards that branch is very low, so falling + // back to slightly imprecise maintainer data from master only has a marginal effect. + per_page: 10, + }) + ).data + + for (const commit of commits) { + const run = ( + await github.rest.actions.listWorkflowRuns({ + ...context.repo, + workflow_id: 'merge-group.yml', + status: 'success', + exclude_pull_requests: true, + per_page: 1, + head_sha: commit.sha, + }) + ).data.workflow_runs[0] + if (!run) continue + + const artifact = ( + await github.rest.actions.listWorkflowRunArtifacts({ + ...context.repo, + run_id: run.id, + name: 'maintainers', + }) + ).data.artifacts[0] + if (!artifact) continue + + await artifactClient.downloadArtifact(artifact.id, { + findBy: { + repositoryName: context.repo.repo, + repositoryOwner: context.repo.owner, + token: core.getInput('github-token'), + }, + path: path.resolve(path.join('branches', branch)), + expectedHash: artifact.digest, + }) + + return JSON.parse( + await readFile( + path.resolve(path.join('branches', branch, 'maintainers.json')), + 'utf-8', + ), + ) + } + + // We get here when none of the 10 commits we looked at contained a maintainer map. + // For the master branch, we don't have any fallback options, so we error out. + // For other branches, we select a suitable fallback below. + if (branch === 'master') throw new Error('No maintainer map found.') + + const { stable, version } = classify(branch) + + const release = `release-${version}` + if (stable && branch !== release) { + // Only fallback to the release branch from *other* stable branches. + // Explicitly avoids infinite recursion. + return await getMaintainerMap(release) + } else { + // Falling back to master as last resort. + // This can either be the case for unstable staging-esque or wip branches, + // or for the primary stable branch (release-XX.YY). + return await getMaintainerMap('master') + } + } + + // Simple cache for maintainer maps to avoid downloading the same artifacts + // over and over again. Ultimately returns a promise, so the result must be + // awaited for. + const maintainerMaps = {} + function getMaintainerMap(branch) { + if (!maintainerMaps[branch]) { + maintainerMaps[branch] = downloadMaintainerMap(branch) + } + return maintainerMaps[branch] + } + async function handlePullRequest({ item, stats }) { const log = (k, v) => core.info(`PR #${item.number} - ${k}: ${v}`) @@ -177,21 +266,44 @@ module.exports = async ({ github, context, core, dry }) => { expectedHash: artifact.digest, }) - const maintainers = new Set( - Object.keys( - JSON.parse( - await readFile(`${pull_number}/maintainers.json`, 'utf-8'), - ), - ).map((m) => Number.parseInt(m, 10)), - ) - const evalLabels = JSON.parse( await readFile(`${pull_number}/changed-paths.json`, 'utf-8'), ).labels + // TODO: Get "changed packages" information from list of changed by-name files + // in addition to just the Eval results, to make this work for these packages + // when Eval results have expired as well. + let packages + try { + packages = JSON.parse( + await readFile(`${pull_number}/packages.json`, 'utf-8'), + ) + } catch (e) { + if (e.code !== 'ENOENT') throw e + // TODO: Remove this fallback code once all old artifacts without packages.json + // have expired. This should be the case in ~ February 2026. + packages = Array.from( + new Set( + Object.values( + JSON.parse( + await readFile(`${pull_number}/maintainers.json`, 'utf-8'), + ), + ).flat(1), + ), + ) + } + + const maintainers = await getMaintainerMap(pull_request.base.ref) + Object.assign(prLabels, evalLabels, { - '12.approved-by: package-maintainer': - maintainers.intersection(approvals).size > 0, + '11.by: package-maintainer': + packages.length && + packages.every((pkg) => + maintainers[pkg].includes(pull_request.user.id), + ), + '12.approved-by: package-maintainer': packages.some((pkg) => + maintainers[pkg].some((m) => approvals.has(m)), + ), }) }