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

Conversation

philiptaron
Copy link
Contributor

@philiptaron philiptaron commented Sep 2, 2024

Motivation for change

The nixpkgs-check-by-name tool is expanding its scope to vet more changes across Nixpkgs.

Description of changes

  • Rename things from nixpkgs-check-by-name to nixpkgs-vet.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

As discussed in PMs, probably best not to add pkgs.nixpkgs-vet:

  • It would be confusing because it's not the actual version of the tool that should be used to check CI. Instead ./maintainers/scripts/nixpkgs-vet.sh should be used, which fetches the correct version (though it also needs to build it)
  • CI won't be able to use the version from Nixpkgs, nor reuse its Hydra build results
  • We'd be duplicating the Nix expression here, they could easily drift apart (and indeed did already here!)
  • Nixpkgs-vet should be considered downstream from Nixpkgs. With the automated weekly update action we have in the Nixpkgs-vet repo, we'll be notified if any dependency breaks the build. I don't think there's a need for package maintainers to try to prevent build failures in Nixpkgs already.
  • It's also just less confusing, less recursive references :)

Adding libSrc for the other purposes sounds fine though

Other than that and the couple comments, I think this looks good!

pkgs/test/check-by-name/pinned-version.txt Outdated Show resolved Hide resolved
maintainers/scripts/check-by-name.sh Show resolved Hide resolved
@philiptaron
Copy link
Contributor Author

Dropped commits making nixpkgs-vet a package. While I disagree with @infinisil's reasoning, it's not worth blocking this update on that disagreement.

@infinisil
Copy link
Member

infinisil commented Sep 3, 2024

Ohh, we should totally move pkgs/test/nixpkgs-vet to ci/nixpkgs-vet actually. It really doesn't fit in pkgs/test, but ci didn't exist back then. Might be best to do it in this PR so we don't have to do this keeping-around-some-old-files dance multiple times

@philiptaron
Copy link
Contributor Author

Ohh, we should totally move pkgs/test/nixpkgs-vet to ci/nixpkgs-vet actually. It really doesn't fit in pkgs/test, but ci didn't exist back then. Might be best to do it in this PR so we don't have to do this keeping-around-some-old-files dance multiple times

I don't see any reason to make a new symlink in maintainers/scripts in this case; running ci/nixpkgs-vet.sh is extremely self-documenting.

@infinisil
Copy link
Member

Sounds good to me!

@philiptaron philiptaron marked this pull request as draft September 3, 2024 20:38
@philiptaron philiptaron force-pushed the nixpkgs-vet branch 2 times, most recently from f12a88b to 2f3c8b0 Compare September 3, 2024 20:48
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks!

Everything gets moved into the `ci/` top-level directory.

We keep behind `maintainers/scripts/check-by-name.sh` and `pkgs/test/check-by-name/pinned-version.txt` as they are going to cause CI errors and confusion until we get all the way through the various channels.
They'll be removed in about a week or so.
@philiptaron philiptaron marked this pull request as ready for review September 3, 2024 20:53
@philiptaron
Copy link
Contributor Author

OK, I finished futzing with ci/README.md.

@philiptaron philiptaron merged commit e1d313d into NixOS:master Sep 3, 2024
12 of 13 checks passed
@philiptaron philiptaron changed the title nixpkgs-vet: init at 0.1.4 ci/nixpkgs-vet: 0.1.2 -> 0.1.4 Sep 3, 2024
@philiptaron
Copy link
Contributor Author

Should we backport this to 24.05? I think we should.

#
# 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

@infinisil
Copy link
Member

I don't think we need to bother backporting, because anything that only goes to release branches won't affect Nixpkgs long-term

@infinisil
Copy link
Member

I guess it might be confusing if different checks are enforced for different PRs though

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.

2 participants