search

NixOS / ofborg

@ofborg tooling automation https://monitoring.ofborg.org/dashboard/db/ofborg
https://ofborg.org
MIT License
232 stars 166 forks source link

eval/nixpkgs,outpaths: fix alias checking #645

Closed lilyinstarlight closed 1 year ago

lilyinstarlight commented 1 year ago

This basically does an approach like #594 but also adds an eval step to check whether the package set still shallow evals with aliases enabled

This does seem to catch at least typos in aliases.nix and should hopefully catch whatever else we care about in there. This does not increase ofborg eval time since it effectively changes default outpath calculation and replaces packages-list-no-aliases by packages-list-with-aliases

Alternative to #643 Closes #575 Supersedes #572 #539

vcunat commented 1 year ago

Why do we even keep the package-list-with-aliases task? I assumed we'd just drop it.

lilyinstarlight commented 1 year ago

Why do we even keep the package-list-with-aliases task? I assumed we'd just drop it.

I believe the whole reason #594 was closed rather than merged to begin with is because then aliases.nix would not longer be checked for errors (see https://github.com/NixOS/ofborg/pull/594#issuecomment-1104460309)

Should we just merge it with the check right above it to only check that the package list with aliases.nix evaluates fine? Or is there value is checking the JSON package list both with and without aliases?

vcunat commented 1 year ago

Ah, I see. Checking that does make sense.

cole-h commented 1 year ago

Why do we even keep the package-list-with-aliases task? I assumed we'd just drop it.

I believe the whole reason #594 was closed rather than merged to begin with is because then aliases.nix would not longer be checked for errors (see #594 (comment))

Should we just merge it with the check right above it to only check that the package list with aliases.nix evaluates fine? Or is there value is checking the JSON package list both with and without aliases?

Let's keep it as-is for now.

I like this much better than https://github.com/NixOS/ofborg/pull/643 (provided it works... :crossed_fingers:). I'll make some time to get this deployed tomorrow.

Thanks for the PR!

lilyinstarlight commented 1 year ago

I like this much better than #643 (provided it works... :crossed_fingers:).

I did attempt to test it locally and while I could confirm that the with-aliases check works, I apparently don't have enough memory (32G ram + 16G swap!) to fully eval outpaths.nix

But I did manage to get it to eval far enough to fail one of the problematic alias PRs that ofborg missed, so it does look like it should be good. Thank you!

I'll close the other PR and mark this one ready-for-review since I've done a little testing now and this one seems to be more acceptable

vcunat commented 1 year ago

Yes, these things can eat a lot, as I warned. See e.g. https://github.com/NixOS/nixpkgs/issues/227945

cole-h commented 1 year ago

Alright, testing this out dirtily, and if it holds well for the next ~hour, I'll merge this and make it stick. Thanks!

cole-h commented 1 year ago

Seems to work great. Thanks!