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

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

Merged
merged 8 commits into from
Dec 21, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Dec 16, 2023

Description of changes

After #272395 extended the nixpkgs-check-by-name tool to have gradual migration checking, aka ratchet checks, this PR now enables them by passing the --base flag to the tool.

Furthermore, this is a really good opportunity to make the CI check runnable locally, solving #266931 as well as possible, replacing #266937.
Fundamentally it's impossible to make it fully reproducible because the base branch fetching/merging can't happen in the same way, but this PR makes everything be shared that can be shared between CI and the local run.

It's not perfect, but better than nothing.

Things done

  • Tested in CI: Introduce empty arg in a PR tweag/nixpkgs#81
  • Tested locally:
    $ ./pkgs/test/nixpkgs-check-by-name/scripts/run-local.sh by-name-reproducible https://github.com/tweag/nixpkgs.git
    Using HEAD commit cf06cb1cd4cf0f0e92fa3bd3ce894c1255fae8a1
    Creating Git worktree for the HEAD commit in /tmp/tmp.osLpsrjqvl/merged.. Done
    Fetching base branch by-name-reproducible to compare against.. cc0fe3b4b9f268e53fd0c1ce706f91a8d6af1373
    Creating Git worktree for the base branch in /tmp/tmp.osLpsrjqvl/base.. Done
    Merging base branch into the HEAD commit in /tmp/tmp.osLpsrjqvl/merged.. cf06cb1cd4cf0f0e92fa3bd3ce894c1255fae8a1
    Determining the channel to use for PR base branch by-name-reproducible.. nixos-unstable
    Fetching latest version of channel nixos-unstable.. /nix/store/k87dn8r5rr9nsa297my0h3z02hx62mnq-source
    Git revision of channel nixos-unstable is 91a00709aebb3602f172a0bf47ba1ef013e34835
    Fetching the prebuilt version of nixpkgs-check-by-name.. /nix/store/381kv4kh7sx2a43w8vcbqcy67msvyfzq-nixpkgs-check-by-name
    Running nixpkgs-check-by-name..
    pkgs.vcpkg-tool: This attribute is manually defined (most likely in pkgs/top-level/all-packages.nix), which is only allowed if the definition is of the form `pkgs.callPackage pkgs/by-name/vc/vcpkg-tool/package.nix { ... }` with a non-empty second argument.
    Validation failed, see above errors
    Cleaning up.. Done
    
  • Updated contributor docs and code comments
  • Clean commit history

Add a 👍 reaction to pull requests you find important.

@infinisil infinisil requested a review from a user December 16, 2023 02:21
@infinisil infinisil added this to the RFC 140 milestone Dec 16, 2023
@infinisil infinisil force-pushed the by-name-reproducible branch from 96acead to 1025f21 Compare December 19, 2023 02:07
@infinisil infinisil changed the title Make pkgs/by-name check as reproducible as possible, and prepare for --base Make pkgs/by-name check as reproducible as possible, and pass --base Dec 19, 2023
@infinisil infinisil force-pushed the by-name-reproducible branch from 1025f21 to 63a27c1 Compare December 19, 2023 22:21
@infinisil infinisil changed the title Make pkgs/by-name check as reproducible as possible, and pass --base pkgs/by-name: Enable gradual migration checks and add run-local.sh Dec 19, 2023
@infinisil infinisil force-pushed the by-name-reproducible branch 2 times, most recently from 3f3080f to cc0fe3b Compare December 19, 2023 22:31
@infinisil infinisil marked this pull request as ready for review December 19, 2023 22:35
Due to the check soon depending on the base branch (see `--base`),
the CI check can't reasonably share all code with a local check.
We can still make a script to run it locally, just not sharing all code.
Now that we have a script to run the check locally,
there's no real need to output the information to reproduce anymore,
which allows cleaning up the CI workflow.

Furthermore, this prepares the CI workflow to be passed `--base`, as
introduced recently.
This enables the ratchet checks for pkgs/by-name, allowing gradual
migrations!
@infinisil infinisil force-pushed the by-name-reproducible branch from cc0fe3b to 1968bee Compare December 20, 2023 16:38
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Dec 21, 2023
infinisil and others added 2 commits December 22, 2023 00:04
- trace function, avoids littering `echo >&2` all throughout
- Avoid `eval`, remove unneeded shellcheck

Co-Authored-By: Victor Engmark <[email protected]>
@infinisil infinisil force-pushed the by-name-reproducible branch from b62b605 to e130ee3 Compare December 21, 2023 23:06
@infinisil
Copy link
Member Author

Thanks for the detailed review @l0b0! I now squashed all the script improvements into e130ee3, adding you as a co-author :)

@infinisil
Copy link
Member Author

infinisil commented Dec 21, 2023

I'll merge this, because:

This allows me to proceed with finalising the next step for RFC 140:

@infinisil infinisil merged commit 02b3c06 into NixOS:master Dec 21, 2023
4 of 5 checks passed
@infinisil infinisil deleted the by-name-reproducible branch January 5, 2024 00:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants