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

When updating nixpkgs-check-by-name, it should be run on Nixpkgs #256789

Closed
infinisil opened this issue Sep 23, 2023 · 5 comments · Fixed by #281390
Closed

When updating nixpkgs-check-by-name, it should be run on Nixpkgs #256789

infinisil opened this issue Sep 23, 2023 · 5 comments · Fixed by #281390

Comments

@infinisil
Copy link
Member

When the nixpkgs-check-by-name tooling is updated, it should be run on Nixpkgs in the same PR. This also relates to #256788.

This could be achieved using passthru.tests on the tests.nixpkgs-check-by-name derivation which copies Nixpkgs to the store and then runs the tooling on it in a derivation. This would then automatically be run when a commit with the tests.nixpkgs-check-by-name: prefix is in the PR. This is however fairly slow and expensive due to the Nixpkgs copy.

@infinisil infinisil added this to the RFC 140 milestone Sep 23, 2023
@roberth
Copy link
Member

roberth commented Sep 24, 2023

This is however fairly slow and expensive due to the Nixpkgs copy.

OfBorg performs checkouts all the time. I don't think this is significant.

To run it locally, this style of test runner is not great, but we can refer readers to the instructions for a manual run.

@infinisil
Copy link
Member Author

OfBorg performs checkouts all the time. I don't think this is significant.

It's not just a local checkout though, if we use derivations to run the check, then Nixpkgs also needs to be copied into the store. Should probably be fine though, other parts of Nixpkgs also copy Nixpkgs into the store.

@infinisil
Copy link
Member Author

I don't think this should be done after all. With the latest idea here, the latest version of the tooling in Nixpkgs needs to stay independent of the pinned version used for CI.

@Ericson2314
Copy link
Member

Agree that for the PR checking, the currently implemented approach is fine. However, I think it is still good to have the "simple" version that doesn't refer to other Nixpkgs / channels as a tests attribute.

The point of this is not to catch CIs, but to just sanity check with hydra after the fact that everything worked out the way it should. The impure tricky business with channels is still necessary, but the idea is that it is an optimization and the simple self-contained tests.<blah> version is the reference implementation that ultimately defines the behavior we want.

@infinisil
Copy link
Member Author

infinisil commented Jan 16, 2024

Implemented in #256789, turns out this would've been really useful 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants