NixOS/nixfmt

Test on Nixpkgs

infinisil opened this issue · 13 comments

infinisil commented

Every PR should report how it influenced the formatting of Nixpkgs.

We have a working script for this in https://github.com/piegamesde/nixfmt/blob/master/.github/workflows/main.yml / https://github.com/piegamesde/nixfmt/blob/master/sync-pr.sh

piegamesde commented

Currently, that script pushes against some branch in my personal fork. Which is obviously not a permanent solution. My proposal would be to maybe just print the diff in the CI log instead of actually pushing the commits somewhere. The alternative would be to have the script create and manage branches automatically for all PRs. Not sure how feasible that is.

infinisil commented

The script already handles that pretty well! It uses the nixpkgs repo for the same user as the nixfmt repo, and creates/updates branches nixfmt-<pr number>. It should be usable with minimal adjustments if any :)

The one thing that needs to be improved is integration with PRs. It doesn't let the PR know about the branch that is being managed. It also doesn't remove the branch after it's done.

infinisil commented

Experimenting a bit with a separate NixOS organisation. Just linking this for future reference

infinisil commented

Note: We should run it with --verify

Should furthermore check that nix-instantiate --parse is preserved

infinisil commented

(discussed in the team meeting today:)

  • Easiest: Create separate organisation with just a Nixpkgs fork, use it just to view the diffs, no permissions to the main org necessary
  • Do we need to bump Nixpkgs in CI occasionally?
    • No, the script handles that, uses latest Nixpkgs commit before the first PR's commit
  • How to link to Nixpkgs diff? Either via GitHub comment, but status check would be best
nixos-discourse commented

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-03-19/41845/1

gabyx commented

@infinisil: Great effort and very good the sooner that gets finished. Is there a way we can help?

  • I am wondering a bit about how much effort this endeavour produces, isnt the CI part pretty straight forward?

For a PR

  1. Use the formatter on the main branch (you dont want to build it from the PRs branch, somebody could change it...)
  2. Format the files changed in the PR git --no-pager diff --name-only target...source (target=main) or some sort of this.
  3. Check if any diffs happened after format with git diff --quiet --exit-code:
    • yes: fail workflow, and report maybe some details in a special comment.
    • no: succeed the workflow, and maybe report some details in a special comment.

I certainly think I miss something, given the vast amount of done work already. 🤣

infinisil commented

We don't want CI for Nixpkgs, but rather for nixfmt, such that any PR to nixfmt gives you a diff of how the formatting changes in Nixpkgs.

(discussed in the team meeting today)

I just realised that we can get rid of most of the complexity of the sync-pr.sh script, since we don't really care about incremental diffs between individual commits in a single PR. Having a single diff for the entire PR is good enough.

  • Create a pull_request_target PR action to run for all PRs
  • Build nixfmt from the base branch of the PR
  • Format Nixpkgs with the base version
  • Build nixfmt from the checkout of the PR (watch out to leak secrets)
  • Format Nixpkgs from the PR branch
  • Ideally: Clicking on the Details of the Status Check brings you to a GitHub diff view of the changes (maybe provide an option to select base/PR diff or original/base, etc.)
  • Because we don't want to give the bot permission to push to Nixpkgs, we should use a GitHub machine account, e.g. like I did in here: https://github.com/NixOS/nixpkgs-check-by-name/blob/8c2ad6a1ed5248ed28eff2ceeba96fb3ebe5d5d6/.github/workflows/update.yml#L22-L36
  • I'll set the secrets for @infinixbot to this repo so that this works
    MACHINE_USER_PAT is now set as ${{ secrets.MACHINE_USER_PAT }}, it should be able to push commits to https://github.com/infinixbot/nixpkgs
nixos-discourse commented

This issue has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/formatting-team-meeting-2024-04-02/42658/1

piegamesde commented

since we don't really care about incremental diffs between individual commits in a single PR. Having a single diff for the entire PR is good enough

Actually, having a per-commit diff was really valuable to me, especially during development. I regularly do incremental changes and see if they change the output for example

  • 👍1
infinisil commented

@piegamesde I think that would be better done locally for faster feedback, this way you also don't need to wait for CI (though you'll also have to let your own machine churn). I'd say CI for PRs needs to be primarily for others to see/verify the changes on a large scale, which doesn't need to be per-diff, especially with smaller PRs.

infinisil commented

I'm now working on this. It turns out that it's not easily possible to create a custom link in the status checks, seems like one needs a GitHub App for that. And considering that, there's really no downside to doing the full sync-pr.sh script to compute the diff for each commit separately.

infinisil commented

Now done with #186 :)