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
Meta refactor #22277
Meta refactor #22277
Conversation
@vcunat, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @Profpatsch and @wmertens to be potential reviewers. |
Yes please! As a bonus, pkgs/stdenv/generic/default.nix is actually readable now, thank god! |
pkgs/stdenv/generic/default.nix
Outdated
in if !v.valid | ||
then throwEvalHelp (removeAttrs v ["valid"]) | ||
else true; | ||
pos' = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While we're at it, can we make this just the default arg for pos
, rather than using , pos ? null
and if pos != null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good; pushed.
pkgs/stdenv/generic/default.nix
Outdated
in | ||
|
||
lib.addPassthru | ||
(derivation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the indentation right here? I think derivation
is taking two arguments but they are indented at different levels.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the other argument is for addPassthru
, and derivation
takes a single argument only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, hah. This seems so obvious in hindsight.
Does this mean that Moving meta checking into a separate file is a good idea, though I'm not sure if it should be added to |
@edolstra: I left this as it was/is now, i.e. disallowed/broken packages are hidden but other packages that depend on such are shown. Fixing that would be easy and I had it so originally, but there would be a large performance price for that query. I used lib, as I originally thought it would be used by |
I’m not sure if this preserves semantics. Did you move the |
@Profpatsch: I don't get which |
@vcunat the one directly before |
@Profpatsch: if it's supposed to be this one
that's now done inside |
I suppose I'll have to look at the performance again. For
(taken from the |
Anything this is waiting on? I'm eager to it merged! |
I haven't got to looking into the nontrivial performance regression. There have been more pressing matters, even in nixpkgs. |
@vcunat Any time to look at this again? I think I'll need it for cross compilation soonish. |
lib/check-meta.nix
Outdated
else if !allowBroken && meta.platforms or null != null && !lib.lists.elem system meta.platforms then | ||
{ valid = false; reason = "broken"; errormsg = "is not supported on ‘${system}’"; } | ||
else { valid = true; }; | ||
# ^ checkMetaValidity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment probably isn't needed now this isn't in a huge scary file :).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When does null != null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have the precedence wrong, perhaps? Read it as (meta.platforms or null) != null
. It's a shorter way of doing meta ? platforms && meta.platforms != null
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the or
operator binds even closer than function application. NixOS/nix#629
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case, this discussion thread is on a stale diff. Current PR is: #27045
@Ericson2314: I've been no good on nix* time in the past months an most of it was taken just by trying to fixup channels. Still, I can't predict this well. You're interested in the laziness part? (d2e4af7 ATM) I don't yet know how exactly is cross-compilation evaluated now, even :-) |
The invalid meta.outputsToInstall has been blocking channel updates. https://mailman.science.uu.nl/pipermail/nix-dev/2017-June/023991.html
@vcunat I suppose the laziness is useful too, but mainly I want it because stdenv.generic is big and scary and this makes it less so :). I had some vague thoughts too about separating stdenv from |
Right, that no-lazy part is easy to do and I found no performance impact, so I think I can do that before this week ends. |
Wonderful, thanks! |
@vcunat I really shouldn't be looking a gift horse in the mouth :), but when you do this, if you could use http://github.com/obsidiansystems/nixpkgs/tree/ericson2314-nixpkgs-base as your base branch, that would be much appreciated. That branch is my last merged PR to nixpkgs, so it always points to an upstream commit. That way, if master is broken, I know PR starts from a known-good change which includes all my cross changes on top. Then I can I can continue working from your PR tip rather than the merge commit and all will be well. Alternatively, I'd be happy to pick up this PR if you are too busy---I think I should be able to figure out how to do the split without the laziness change. Certainly keeping the channels in a good state is important and tiresome work so you deserve a break! |
OK, shouldn't be a problem, as I see it's not too late behind master. |
I think querying such information requires a database. One design is to let Nix become a database engine. Another (my idea) is to treat Nix as a data source and load that data into something which is already a database engine (don't really care which one) and then query that. Building the database index is an operation that should be done in the background. Can someone tell me why my idea would be a bad idea? |
More like a DB that caches the evaluations done by nix. Many would appreciate that, apparently, and DBs seem common in packaging systems. Still, I don't think it's really related to this PR. |
First stage now on #27045. |
Allow querying meta on disallowed derivations – broken, unfree, etc. But keep checking of the conditions including dependencies when instantiating a derviation. Performance: this causes creating another copy of the attr-map. `nix-env -f . -qa` shows a roughly 2% increase in time and space. `nix-instantiate --eval --strict --show-trace ./maintainers/scripts/eval-release.nix` shows a higher increase (8-9%), but *real time* is not noticeably different, probably due to GC being parallelized.
To be clear, |
Motivation for this change
meta
on disallowed derivations – broken, unfree, etc. But keep checking of the conditions including dependencies when instantiating a derviation.Better separate the code.Moved to postgresql: fix nixos tests and add xml support #27405.I was slightly taken aback by the fact that
nix-env -qa
does not check dependencies of packages, but I was unable to fix that without a significant performance hit, so I left it as it was.I tested this fixes #21845, #22102, #7541, hopefully without any caveats.