nixpkgs-check-by-name: make all github-driven checks locally reproducible · Pull Request #266937 · NixOS/nixpkgs

ghost · 2023-11-11T23:51:59Z

Description of changes

This commit moves the chunks of shellcode in .github/workflows/check-by-name.yml into individual scripts.

What remains in .github/workflows/check-by-name.yml are three-line scripts which:

Dump the environment to the console
Set -x so the following command is printed in properly-escaped form, which developers can simply paste to their own shell to reproduce the command
exec() into the file containing the shellcode which previously comprised that step.

This ensures that if a test fails, the PR author doesn't have to rummage around in the bowels of github in order to reproduce the failure locally.

Fixes #266930
Fixes #266931

Things done

…ible This commit moves the chunks of shellcode in .github/workflows/check-by-name.yml into individual scripts. What remains in .github/workflows/check-by-name.yml are three-line scripts which: 1. Dump the environment to the console 2. Set `-x` so the following command is printed in properly-escaped form, which developers can simply paste to their own shell to reproduce the command 3. exec() into the file containing the shellcode which previously comprised that step. This ensures that if a test fails, the PR author doesn't have to rummage around in the bowels of github in order to reproduce the failure locally.

infinisil

Not sure about this. The commands in the action aren't actually reproducible since they depend on the base branch and the channel. However, all the information to reproduce it is already printed decently in a markdown for each check: https://github.com/NixOS/nixpkgs/actions/runs/6837521847#summary-18593634536

How about adding a command like

pkgs/test/nixpkgs-check-by-name/reproduce.sh <base SHA> <tooling SHA>

Which then tests HEAD in the current Nixpkgs the same way as the action does. Ideally the action should also just use this command then.

Then the markdown can be changed to just tell users to run the command with specific flags.

ghost · 2023-11-12T03:58:33Z

The commands in the action aren't actually reproducible since they depend on the base branch and the channel.

These are passed in environment variables, which are printed.

the action should also just use this command then.

That is the most important part. I'm on board as long as that is true.

ghost · 2023-11-12T03:59:40Z

With latest push, using #266956 as an example:

$ pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/reproduce.sh \
  c705dccf910d66f8f9ca10ba130e8dcd6a2e6063 \
  85f1ba3e51676fa8cc604a3d863d729026a6b8eb \
  09da534bcec2169b6408c4bdea98ce271efb5487
/git/work/pr/by-name/legible/nixpkgs-for-tooling-binaries
Fetching pre-built nixpkgs-check-by-name from channel  at revision 85f1ba3e51676fa8cc604a3d863d729026a6b8eb
/nix/store/y78kdkj80p8d6h84bkvkilshjqhzgyv0-nixpkgs-check-by-name
Checking whether the check succeeds on the base branch
Validated successfully
Checking whether the check would succeed after merging this pull request
Validated successfully
| Nixpkgs version | Check result |
| --- | --- |
| Latest base commit | :heavy_check_mark: |
| After merging this PR | :heavy_check_mark: |

The check succeeds on both the base branch and after merging this PR
### Details
- nixpkgs-check-by-name tool binaries:
  - Channel:
  - Tooling binaries built at nixpkgs commit: [85f1ba3e51676fa8cc604a3d863d729026a6b8eb](https://github.com//commit/85f1ba3e51676fa8cc604a3d863d729026a6b8eb)
  - Store path: `/nix/store/y78kdkj80p8d6h84bkvkilshjqhzgyv0-nixpkgs-check-by-name`
- Tested Nixpkgs:
  - Base branch:
  - Latest base branch commit: [c705dccf910d66f8f9ca10ba130e8dcd6a2e6063](https://github.com//commit/c705dccf910d66f8f9ca10ba130e8dcd6a2e6063)
  - Latest PR commit: [](https://github.com//commit/)
  - Merge commit: [09da534bcec2169b6408c4bdea98ce271efb5487](https://github.com//commit/09da534bcec2169b6408c4bdea98ce271efb5487)

requested changes implemented, re-review requested.

ghost · 2023-11-12T04:01:16Z

The most annoying part is that, for not-yet-merged PRs, the mergeSha will only exist on github and github will not let you fetch it since it's ephemeral.

But at least this is a step forward.

ghost · 2023-11-12T04:02:07Z

It's easiest to review by reading 8187fe5 by itself. The other commits just copy things between various files, making it difficult to review the entire PR as a single diff.

infinisil

I've got some suggestions, but I think we can make something like this work. Also be sure to update the contributor docs too.

infinisil · 2023-11-13T00:06:13Z

.github/workflows/check-by-name.yml

-          echo "mergedSha=$mergedSha" >> "$GITHUB_ENV"
+          env
+          set -x
+          exec pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/check-mergeability.sh


This is very GitHub Actions specific. Locally you'd want to test your current working tree, not the PR's HEAD, so this should stay here.

infinisil · 2023-11-13T00:28:29Z

.github/workflows/check-by-name.yml

+          # nixpkgs.
+          env
+          set -x
+          exec pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/determine-pr-hashes.sh


This should also stay, it's GitHub-specific. Locally it's rarely the case to have a merge commit with the right parent commits.

infinisil · 2023-11-13T00:33:12Z

.github/workflows/check-by-name.yml

+          # nixpkgs.
+          env
+          set -x
+          exec pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/determine-channel-for-dependencies.sh


It's not great how there's all of these separate files. The main reason these steps are split up so that it's easier to see where time is spent (e.g. see https://github.com/NixOS/nixpkgs/actions/runs/6844035438/job/18607346212). But that's not very important, I'd rather have this and all steps after as a single script.

Also we shouldn't make env necessary. If we have a script to reproduce it locally, the script shouldn't depend on GitHub-specific variables.

I also don't think set -x is necessary, the code is very verbose and already prints everything useful.

infinisil · 2023-11-13T00:42:53Z

pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/reproduce.sh

+toolingBinariesSha="$2"
+mergedSha="$3"
+
+nixpkgs_for_tooling_binaries=$(nix-instantiate --eval --expr "builtins.fetchTarball \"https://github.com/nixos/nixpkgs/archive/$toolingBinariesSha.tar.gz\"" | tr -d '"')


This duplicates the tooling Nixpkgs download, it should re-use the one it already fetched from the channels. Otherwise this adds another ~15 seconds to the CI time.

infinisil · 2023-11-13T00:43:46Z

pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/fetch-latest-version-of-channel.sh

+echo "nixpkgs=$nixpkgs" >> "$GITHUB_ENV"
+echo "toolingBinariesSha=$toolingbinariesSha" >> "$GITHUB_ENV"


You can remove all GITHUB_ENV assignments if we only have a single step and script.

infinisil · 2023-11-13T00:48:38Z

pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/reproduce.sh

+# Usage: pkgs/test/nixpkgs-check-by-name/reproduce.sh <base SHA> <tooling SHA> <merged SHA>
+
+# TODO(amjoseph): allow omitting the final argument, since it is
+# often a commit which exists only in github (and is difficult to
+# `git fetch` from github before the PR is merged).


Ideally we'd have two scripts:

./reproduce.sh <tooling SHA> <base SHA> <merged SHA>: Reproducible from the args as here (I'd order the args this way).

./reproduce-latest.sh <base branch name>: Tests the current HEAD against the latest commit of the base branch name, effectively replicating what a new CI run should do if you committed and pushed the current code. This should first determine the arguments for ./reproduce.sh and then call it.

Can you implement something like that?

infinisil · 2023-11-13T00:50:58Z

pkgs/test/nixpkgs-check-by-name/workflows/check-by-name/reproduce.sh

+    echo "  - Tooling binaries built at nixpkgs commit: [$toolingBinariesSha](https://github.com/${GITHUB_REPOSITORY}/commit/$toolingBinariesSha)"
+    echo "  - Store path: \`$(realpath result)\`"
+    echo "- Tested Nixpkgs:"
+    echo "  - Base branch: $BASE_SHA"
+    echo "  - Latest base branch commit: [$baseSha](https://github.com/${GITHUB_REPOSITORY}/commit/$baseSha)"
+    echo "  - Latest PR commit: [$headSha](https://github.com/${GITHUB_REPOSITORY}/commit/$headSha)"
+    echo "  - Merge commit: [$mergedSha](https://github.com/${GITHUB_REPOSITORY}/commit/$mergedSha)"
+} | tee -a "${GITHUB_STEP_SUMMARY:-/dev/null}"


We should avoid all GitHub-specific variables from such a script, such that you can run it locally without knowing these variables.

infinisil · 2023-12-16T02:27:33Z

To make this actually work it has to be implemented a bit differently, I'm doing that here: #274591

infinisil · 2023-12-21T23:20:12Z

Effectively done with #274591

github-actions bot added the 6.topic: policy discussion Discuss policies to work in and around Nixpkgs label Nov 11, 2023

ghost marked this pull request as ready for review November 11, 2023 23:52

ghost requested a review from infinisil as a code owner November 11, 2023 23:52

infinisil previously requested changes Nov 12, 2023

View reviewed changes

Adam Joseph added 3 commits November 11, 2023 19:39

rename run-nixpkgs-check-by-name.sh to reproduce.sh

692fc8e

give $rev a more descriptive name

81cb101

implement @infinisil feedback from #266937 (review)

8187fe5

ghost requested a review from infinisil November 12, 2023 03:59

infinisil added this to the RFC 140 milestone Nov 13, 2023

infinisil requested changes Nov 13, 2023

View reviewed changes

infinisil mentioned this pull request Dec 16, 2023

pkgs/by-name: Enable gradual migration checks and add run-local.sh #274591

Merged

4 tasks

infinisil closed this Dec 21, 2023

ghost deleted the pr/by-name/legible branch January 23, 2024 06:47

Artturin mentioned this pull request Jan 29, 2024

List of @amjoseph-nixpkgs Pull requests before account removal #284796

Open

		echo "nixpkgs=$nixpkgs" >> "$GITHUB_ENV"
		echo "toolingBinariesSha=$toolingbinariesSha" >> "$GITHUB_ENV"

Uh oh!

Comments

Conversation

ghost commented Nov 11, 2023

Description of changes

Things done

Uh oh!

infinisil left a comment

Choose a reason for hiding this comment

Uh oh!

ghost commented Nov 12, 2023

Uh oh!

ghost commented Nov 12, 2023

Uh oh!

ghost commented Nov 12, 2023

Uh oh!

ghost commented Nov 12, 2023

Uh oh!

infinisil left a comment

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

infinisil Nov 13, 2023

Choose a reason for hiding this comment

Uh oh!

infinisil commented Dec 16, 2023

Uh oh!

infinisil commented Dec 21, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

infinisil Nov 13, 2023 •

edited

Loading