Meta refactor by vcunat · Pull Request #22277 · NixOS/nixpkgs

vcunat · 2017-01-29T20:22:38Z

Motivation for this change

Allow querying 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.

mention-bot · 2017-01-29T20:22:40Z

@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.

Ericson2314 · 2017-01-29T20:46:30Z

Yes please! As a bonus, pkgs/stdenv/generic/default.nix is actually readable now, thank god!

Ericson2314 · 2017-01-29T20:55:48Z

pkgs/stdenv/generic/default.nix

-             in if !v.valid
-               then throwEvalHelp (removeAttrs v ["valid"])
-               else true;
+      pos' =


While we're at it, can we make this just the default arg for pos, rather than using , pos ? null and if pos != null?

Sounds good; pushed.

Ericson2314 · 2017-01-29T20:56:25Z

pkgs/stdenv/generic/default.nix

+    in
+
+      lib.addPassthru
+        (derivation


Is the indentation right here? I think derivation is taking two arguments but they are indented at different levels.

No, the other argument is for addPassthru, and derivation takes a single argument only.

Oh, hah. This seems so obvious in hindsight.

edolstra · 2017-01-30T09:57:39Z

Does this mean that nix-env -qa will now show disallowed/broken packages? The intent was that they don't show up.

Moving meta checking into a separate file is a good idea, though I'm not sure if it should be added to lib (since lib is more for generic functions rather than specific functions used in only one place).

vcunat · 2017-01-30T10:45:14Z

@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 buildEnv and perhaps others as well, but later I realized that they use stdenv, too... so currently I know no other place that would utilize the functions. If you like, we can move the file to pkgs/stdenv/generic/check-meta.nix or similar and not expose the functions.

Profpatsch · 2017-01-30T20:36:35Z

I’m not sure if this preserves semantics.

Did you move the assert?

vcunat · 2017-01-30T20:46:11Z

@Profpatsch: I don't get which assert exactly you mean, but the whole checking code was moved into a separate file.

Profpatsch · 2017-01-31T21:12:07Z

@vcunat the one directly before mkDerivation in the old version.

vcunat · 2017-01-31T21:33:12Z

@Profpatsch: if it's supposed to be this one

# Throw an error if trying to evaluate an non-valid derivation

that's now done inside addMetaCheckInner... and it's delayed to evaluating the name. (Your old PR did this when evaluating the builder, but that one's not evaluated at all during nix-env -qa by default.)

vcunat · 2017-02-04T19:07:49Z

I suppose I'll have to look at the performance again. For nix-env -qa it seemed OK but eval-release shows a significant performance hit:

$ time nix-instantiate --eval --strict --show-trace ./maintainers/scripts/eval-release.nix > /dev/null
36.16user 0.71system 0:24.42elapsed 150%CPU (0avgtext+0avgdata 2578940maxresident)k

$ time nix-instantiate --eval --strict --show-trace ./maintainers/scripts/eval-release.nix > /dev/null
46.40user 0.89system 0:32.41elapsed 145%CPU (0avgtext+0avgdata 3273876maxresident)k

(taken from the tarball job; similar to Hydra's evaluation)

Ericson2314 · 2017-02-16T19:44:21Z

Anything this is waiting on? I'm eager to it merged!

vcunat · 2017-02-16T20:52:26Z

I haven't got to looking into the nontrivial performance regression. There have been more pressing matters, even in nixpkgs.

Ericson2314 · 2017-06-23T20:39:07Z

@vcunat Any time to look at this again? I think I'll need it for cross compilation soonish.

Ericson2314 · 2017-06-23T20:40:35Z

lib/check-meta.nix

+      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


This comment probably isn't needed now this isn't in a huge scary file :).

When does null != null?

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

Yes, the or operator binds even closer than function application. NixOS/nix#629

In any case, this discussion thread is on a stale diff. Current PR is: #27045

vcunat · 2017-06-26T05:36:29Z

@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

Ericson2314 · 2017-06-28T04:59:03Z

@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 mkDerivation, and this tidyness would make thinking about that easier.

vcunat · 2017-06-28T10:55:47Z

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.

Ericson2314 · 2017-06-28T13:25:47Z

Wonderful, thanks!

Ericson2314 · 2017-06-28T18:21:38Z

@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!

vcunat · 2017-06-28T18:34:44Z

OK, shouldn't be a problem, as I see it's not too late behind master.

0xABAB · 2017-06-28T22:26:32Z

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?

vcunat · 2017-06-29T06:30:01Z

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.

vcunat · 2017-07-02T12:47:07Z

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.

vcunat · 2018-01-14T20:52:20Z

To be clear, meta and most other attributes can now (67e8392) be queried on disallowed derivations.

vcunat added the 9.needs: community feedback This needs feedback from more community members. label Jan 29, 2017

vcunat mentioned this pull request Jan 29, 2017

make mkDerivation lazier #11094

Closed

Ericson2314 reviewed Jan 29, 2017

View reviewed changes

Ericson2314 approved these changes Jan 29, 2017

View reviewed changes

domenkozar mentioned this pull request Feb 7, 2017

Unfree license blocks me from querying an attribute edolstra/nix-repl#37

Closed

vcunat mentioned this pull request Feb 23, 2017

nixpkgs: allow packages to be marked insecure (reverted) #22890

Merged

4 tasks

This was referenced May 19, 2017

Put apple-source-releases in Hydra #25200

Closed

enable hydra jobs for some of the darwin only packages #25930

Closed

Ericson2314 reviewed Jun 23, 2017

View reviewed changes

vcunat referenced this pull request Jun 26, 2017

qtinstaller: fix broken meta

ce8178e

The invalid meta.outputsToInstall has been blocking channel updates. https://mailman.science.uu.nl/pipermail/nix-dev/2017-June/023991.html

vcunat mentioned this pull request Jul 2, 2017

stdenv: Refactor meta checks #27045

Merged

vcunat mentioned this pull request Dec 30, 2017

stdenv: perform checks only when evaluating .drv and .out #33057

Merged

3 tasks

vcunat force-pushed the meta-refactor branch from cc4bedd to 37e7b22 Compare January 1, 2018 10:11

vcunat requested a review from edolstra as a code owner January 1, 2018 10:11

GrahamcOfBorg added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Jan 1, 2018

vcunat added this to the 18.03 milestone Jan 1, 2018

vcunat closed this in 67e8392 Jan 14, 2018

Uh oh!

Conversation

vcunat commented Jan 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Motivation for this change

Uh oh!

mention-bot commented Jan 29, 2017

Uh oh!

Ericson2314 commented Jan 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

edolstra commented Jan 30, 2017

Uh oh!

vcunat commented Jan 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Profpatsch commented Jan 30, 2017

Uh oh!

vcunat commented Jan 30, 2017

Uh oh!

Profpatsch commented Jan 31, 2017

Uh oh!

vcunat commented Jan 31, 2017

Uh oh!

vcunat commented Feb 4, 2017

Uh oh!

Ericson2314 commented Feb 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcunat commented Feb 16, 2017

Uh oh!

Ericson2314 commented Jun 23, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcunat commented Jun 26, 2017

Uh oh!

Ericson2314 commented Jun 28, 2017

Uh oh!

vcunat commented Jun 28, 2017

Uh oh!

Ericson2314 commented Jun 28, 2017

Uh oh!

Ericson2314 commented Jun 28, 2017

Uh oh!

vcunat commented Jun 28, 2017

Uh oh!

0xABAB commented Jun 28, 2017

Uh oh!

vcunat commented Jun 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vcunat commented Jul 2, 2017

Uh oh!

vcunat commented Jan 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

vcunat commented Jan 29, 2017 •

edited

Loading

vcunat commented Jan 30, 2017 •

edited

Loading

Ericson2314 commented Feb 16, 2017 •

edited

Loading

vcunat commented Jun 29, 2017 •

edited

Loading