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

File ratchet checks #144

Merged
merged 3 commits into from
Jan 29, 2025
Merged

File ratchet checks #144

merged 3 commits into from
Jan 29, 2025

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jan 8, 2025

This introduces the boilerplate to support ratchet (and non-ratchet) checks for all Nix files in Nixpkgs, which can be used as a basis for #142

This includes #143

Needs some code comments probably before ready for merging
Added


This work is funded by Tweag and Antithesis

For a future change we need a version that gives access to the Success value from both arguments.
@infinisil infinisil marked this pull request as ready for review January 14, 2025 21:04
@infinisil
Copy link
Member Author

infinisil commented Jan 14, 2025

After very minor changes and adding code comments, I think this PR is ready. While it doesn't introduce any functionality by itself, it's going to be very useful to have this for #142 and similar features. In particular #135 could also be based on this.

The CI failure is unrelated, fixed by #146
Nevermind, I just can't read error messages, the problem was that

--ignore tests/symlink-invalid/pkgs/by-name/fo/foo/foo
--ignore tests/multiple-failures/pkgs/by-name/A/fo@/foo
needed to be updated after 712d97c 😆

@infinisil infinisil requested a review from a team January 14, 2025 21:17
A future commit introduces checks on all Nix files, but this is problematic if the base Nixpkgs
version is part of the main Nixpkgs version
Copy link
Contributor

@philiptaron philiptaron left a comment

Choose a reason for hiding this comment

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

No blocking comments, just a few clarifying questions. Thanks for your patience in waiting for a review.

src/eval.rs Show resolved Hide resolved
src/validation.rs Show resolved Hide resolved
src/ratchet.rs Show resolved Hide resolved
src/ratchet.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
Comment on lines +17 to +18
// Noop for now, only boilerplate to make it easier to add future file-based checks
Ok(Success(ratchet::File {}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Or is this the place that #142 will go?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is where the logic goes yes :)

src/files.rs Show resolved Hide resolved
src/files.rs Show resolved Hide resolved
For now without any checks, but this makes introducing ones fairly easy
@philiptaron philiptaron merged commit 30709cf into main Jan 29, 2025
3 checks passed
@philiptaron philiptaron deleted the file-ratchet-check branch January 29, 2025 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants