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: make no-aliases checks use deeper eval #643

Closed lilyinstarlight closed 1 year ago

lilyinstarlight commented 1 year ago

Right now alias usage is only caught more or less for callPackage args, but if an alias is accessed as (for example) buildPackages.pkgconfig in the derivation, then ofborg will show all green for the package-list-no-aliases check despite the derivation actually failing on systems with allowAliases = false

This solves the problem by having ofborg eval derivation output paths instead of just a shallow eval package list to ensure there is no alias usage in the derivation (it won't catch things like passthru.pkg-config = buildPackages.pkgconfig but that's probably acceptable)

I'm open to renaming the check to package-outputs-no-aliases to reflect the change in functionality

A few examples of what this would have caught are https://github.com/NixOS/nixpkgs/pull/230188#pullrequestreview-1415772214 and https://github.com/NixOS/nixpkgs/pull/236329#issuecomment-1580292218, but there are a lot more as it seems to be a recurring issue

The difference in eval timing went from ~8 seconds to ~63 seconds for that check on my laptop and so it is a fairly expensive change, but other than something like #594, I don't know a better way to reliably catch this problem

vcunat commented 1 year ago

I think this needs checking that increased RAM usage can be handled by what's currently running it.

K900 commented 1 year ago

We had two of those today alone.

https://github.com/NixOS/nixpkgs/pull/236833 https://github.com/NixOS/nixpkgs/pull/236828

We have to check this, even if the costs are steep.

cole-h commented 1 year ago

Thanks for looking into this!

I really don't like the idea of blowing up eval time (and likely memory usage)... Although I'm sure the beastly machines we have can handle the increased memory usage, the eval time increase has the potential to make the queue a lot more unmanageable than it currently is...

I wonder if we could resurrect https://github.com/NixOS/ofborg/pull/594 and instead add an eval that specifically checks that aliases.nix still evals (as far as I understand, that was the main possible-issue, that we wouldn't check aliases.nix for evaluation if we disallow aliases everywhere)... But if the current situation of just allowAliases = false in one check doesn't satisfy this, maybe that won't either......

I'll do some pondering when I have a free moment, but it's likely we'll just have to eat the eval-time cost until we ban aliases in Nixpkgs :sweat_smile:

lilyinstarlight commented 1 year ago

I wonder if we could resurrect https://github.com/NixOS/ofborg/pull/594 and instead add an eval that specifically checks that aliases.nix still evals (as far as I understand, that was the main possible-issue, that we wouldn't check aliases.nix for evaluation if we disallow aliases everywhere)...

That should work! And is probably a much better idea since it would reduce eval time and fix issues like aliases showing up in calculated attr changes. I don't know that I'll have time to shift this PR to doing that for a few weeks, though

cole-h commented 1 year ago

IMO we should probably leave this PR as is in case globally disabling allowAliases doesn't actually catch the things we want to catch. In that case, we'll probably have to move forward with this and eat the increased eval time. (Hopefully it doesn't come to that.)

vcunat commented 1 year ago

Yes, though I believe that disabling aliases in the part that computes rebuild amount should catch enough cases.

lilyinstarlight commented 1 year ago

I've opened #645 with the suggestion (it turned out to be quite simple). If that PR looks better, I'll close this PR in favor of that one and mark that one ready for review

lilyinstarlight commented 1 year ago

Closing in favor of #645