Skip to content

eval/nixpkgs: make no-aliases checks use deeper eval#643

Closed
lilyinstarlight wants to merge 1 commit intoNixOS:releasedfrom
lilyinstarlight:fix/alias-checking
Closed

eval/nixpkgs: make no-aliases checks use deeper eval#643
lilyinstarlight wants to merge 1 commit intoNixOS:releasedfrom
lilyinstarlight:fix/alias-checking

Conversation

@lilyinstarlight
Copy link
Member

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 NixOS/nixpkgs#230188 (review) and NixOS/nixpkgs#236329 (comment), 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

@K900 K900 mentioned this pull request Jun 9, 2023
12 tasks
@vcunat
Copy link
Member

vcunat commented Jun 9, 2023

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

@K900
Copy link

K900 commented Jun 9, 2023

We had two of those today alone.

NixOS/nixpkgs#236833
NixOS/nixpkgs#236828

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

@cole-h
Copy link
Member

cole-h commented Jun 9, 2023

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

@lilyinstarlight
Copy link
Member Author

I wonder if we could resurrect #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
Copy link
Member

cole-h commented Jun 9, 2023

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
Copy link
Member

vcunat commented Jun 10, 2023

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

@lilyinstarlight
Copy link
Member Author

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
Copy link
Member Author

Closing in favor of #645

@lilyinstarlight lilyinstarlight deleted the fix/alias-checking branch June 15, 2023 14:52
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.

4 participants