stdenv: Refactor meta checks #27045

vcunat · 2017-07-02T12:46:31Z

Just a first stage of #22277 ATM, doing almost nothing but separating the meta-checking code.

I think I now understand why the previous PR and this one have a slight performance regression, and I'll look into fixing it, hopefully before we merge this.

@Ericson2314: I assume you don't want to change the meta-checking code significantly for now and just want it out of the way. Still, the performance fixes will need some small changes to stdenv's default.nix.

mention-bot · 2017-07-02T12:46:33Z

@vcunat, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @copumpkin and @Ericson2314 to be potential reviewers.

zimbatm · 2017-07-02T12:51:56Z

lib/check-meta.nix

+in rec {
+
+  # Extend a derivation with checks for brokenness, license, etc.
+  # Throw a descriptive error when the check fails; return derivaitonArg otherwise.


typo: derivationArg

zimbatm · 2017-07-02T12:52:51Z

lib/check-meta.nix

+    attrs = mkDerivationArg; # TODO: probably get rid of passing this one
+
+    # See discussion at https://github.com/NixOS/nixpkgs/pull/25304#issuecomment-298385426
+    # for why this defaults to false, but I (@copumpkin) want to default it to true soon.


maybe add a date to the comment to give a notion of "soon" :)

We'd have to ask @copumpkin about that.

I dunno, maybe just take out the comment since I don't think people will want this turned on by default.

zimbatm · 2017-07-02T12:54:34Z

lib/check-meta.nix

+    isUnfree = licenses: lib.lists.any (l:
+      !l.free or true || l == "unfree" || l == "unfree-redistributable") licenses;
+
+    # Alow granular checks to allow only some unfree packages


typo: Allow

zimbatm · 2017-07-02T13:00:13Z

lib/check-meta.nix

+    #   allowUnfree = false;
+    #   allowUnfreePredicate = (x: pkgs.lib.hasPrefix "flashplayer-" x.name);
+    # }
+    allowUnfreePredicate = config.allowUnfreePredicate or (x: false);


Would it make sense to also allow a blacklist implementation? Right now it's only possible to go from false -> true.

zimbatm · 2017-07-02T13:01:43Z

lib/check-meta.nix

+
+    showLicense = license: license.shortName or "unknown";
+
+    pos_str = meta.position or "«unknown-file»";


what is the reason to switch to use snake_case here and below?

zimbatm · 2017-07-02T13:06:19Z

pkgs/stdenv/generic/default.nix

+          {
+            inherit config meta derivationArg;
+            mkDerivationArg = attrs;
+            platform = system; # TODO: cross-compilation?


who can help resolve that TODO?

In the meantime I noticed this probably means targetPlatform.system, and that's probably OK. (I'd have rather chosen hostPlatform.system but it hardly makes a difference.)

Stdenv is like a compiler in that that the target platform of the stdenv is the host platform of things made with it's mkDerivation. I'm going to make that less horribly confusing with a refactor following this.

vcunat · 2017-07-02T17:37:58Z

I plan to fix the various typos etc. in followup commit(s); most of it is just unchanged in these commits anyway.

zimbatm · 2017-07-02T18:45:54Z

sounds good, my comments apply to the old code and thus can be addressed later.

0xABAB · 2017-07-02T20:31:59Z

lib/check-meta.nix

+    # Check if a derivation is valid, that is whether it passes checks for
+    # e.g brokenness or license.
+    #
+    # Return { valid: Bool } and additionally


and additionally should be xor (feel free to use human or)

It's more of a:

data Result = Valid | Invalid { reason :: String, ErrorMsg :: String }

Where valid: Bool is the tag in the tagged union, and thus must always be present.

0xABAB · 2017-07-02T20:35:00Z

lib/check-meta.nix

+      else if !allowBroken && attrs.meta.platforms or null != null && !lib.lists.elem platform attrs.meta.platforms then
+        { valid = false; reason = "broken"; errormsg = "is not supported on ‘${platform}’"; }
+      else if !(hasAllowedInsecure attrs) then
+        { valid = false; reason = "insecure"; errormsg = "is marked as insecure"; }


Probably out of scope, but it would be a good place to reference some optional field with details about why it is marked as insecure.

Ericson2314 · 2017-07-02T20:38:58Z

@vcunat Per #22277 (comment) should we not put it in lib? Otherwise anything more we need to do? How bad is the performance stuff? I wouldn't be necessarily be opposed to fixing it later (with the typos / overhaul) either.

0xABAB · 2017-07-02T20:39:20Z

pkgs/stdenv/generic/default.nix

+      meta = { }
+          # If the packager hasn't specified `outputsToInstall`, choose a default,
+          # which is the name of `p.bin or p.out or p`;
+          # if he has specified it, it will be overridden below in `// meta`.


Just write if specified, it will be ...

vcunat · 2017-07-03T06:05:08Z

@Ericson2314: oh, right, let's move it to stdenv/generic/; I need to re-check the full previous thread as I don't remember it all anymore. Performance is explained a little in that commit's message.

Ericson2314 · 2017-07-05T20:46:59Z

lib/check-meta.nix

+  # Extend a derivation with checks for brokenness, license, etc.
+  # Throw a descriptive error when the check fails; return derivaitonArg otherwise.
+  # Note: no dependencies are checked in this step.
+  addMetaChecks = { config, platform, meta, derivationArg, mkDerivationArg }: let


This should be system not platform, because while those terms are almost use interchangeably in nixpkgs, the two-part dash-separated string name is always called system.

In the original code is was called system, so I that's why I bring this up now---once I fix for cross, it will all be gone anyways.

I looked at it from the other side – called it platform because of being matched to meta.platforms, but I don't have any strong opinion about it.

Fair. but that one is the odd one out. I'll probably end generalizing that, rather than renaming it, because it's not possible to enumerate every conceivable compatible platform.

Ericson2314 · 2017-07-05T21:21:17Z

@vcunat I fixed both things I mentioned at https://github.com/obsidiansystems/nixpkgs/tree/meta-refactor-2. Also I rebased it onto the latest nixpkgs-unstable (which my ericson2314-nixpkgs-base will soon point to).

I made it not reevaluate lib (by simply passing it in), which seems to be sharing enough to eliminate most of the performance gap. Either way, I don't think that should prevent merge. I trust it can be made as good or better than before with little effort.

Ericson2314 · 2017-07-07T15:42:01Z

@vcunat do you mind if I merge my rebase so I can merge #27179 ? I'm soon to have less time at work to do cross stuff :).

This just moves some expressions around in preparation to further changes.

Suggested by Ericson2314.

vcunat · 2017-07-07T16:08:01Z

@Ericson2314: OK. I see you've moved the file and I assume there's been no other change in meta checks.

Ericson2314 · 2017-07-07T16:15:10Z

@vcunat well passing the lib as I wrote above. But yes, those are the only two. I'll fix the name issue you mentioned, and then I think I can push to your rebase branch to merge from here, sound good?

Only cosmetic changes are done otherwise. Real refactoring is left for later. There's a small slow-down on my machine: $ time nix-env -qa -P >/dev/null gets from ~2.8 to ~3.5 seconds (negligible change in RAM). That's most likely caused by sharing less computation between different mkDerivation calls, and I plan to improve that soon.

vcunat · 2017-07-07T16:33:21Z

@Ericson2314: sounds good.

Ericson2314 · 2017-07-07T16:35:33Z

Oh, and I did a quick final sanity check no hashes were changed with nix-env -f . -qa --out-path

edolstra · 2017-07-07T16:58:32Z

Did the slowdown mentioned in e8e5745 get resolved? Going from ~2.8s to ~3.5s is not a small slowdown!

Ericson2314 · 2017-07-07T17:04:57Z

@edolstra I think so. Not reevaluated lib seemed to narrow the gap for me. Also, there's more low hanging fruit of non-closure lambda lifting to be done in the follow-up PR.

danbst · 2018-02-12T09:34:02Z

Probably not. I'm looking at https://hydra.nixos.org/job/nixpkgs/trunk/metrics/metric/nix-env.qa.time , 7 july, increased from 3.9 to 5.1

vcunat · 2018-02-12T09:55:32Z

Right, it hasn't been (at least not by me). I have a WIP on that, and I really want to catch it before 18.03.

vcunat mentioned this pull request Jul 2, 2017

Meta refactor #22277

Closed

zimbatm reviewed Jul 2, 2017

View reviewed changes

0xABAB reviewed Jul 2, 2017

View reviewed changes

Ericson2314 requested changes Jul 5, 2017

View reviewed changes

Ericson2314 mentioned this pull request Jul 6, 2017

Define {build,host,target}Platform in stdenv #27179

Merged

vcunat added 3 commits July 7, 2017 12:02

stdenv: refactor (no change in semantics)

7fdf18e

This just moves some expressions around in preparation to further changes.

stdenv: simple refactor to get rid of pos'

5afcdc8

Suggested by Ericson2314.

lib.lists.mutuallyExclusive: add function

dfc004e

Ericson2314 force-pushed the meta-refactor-2-rebased branch from f4eacca to e8e5745 Compare July 7, 2017 16:33

Ericson2314 changed the title ~~stdenv: refactor meta checks~~ stdenv: Refactor meta checks Jul 7, 2017

Ericson2314 merged commit f2f50aa into NixOS:master Jul 7, 2017

vcunat deleted the meta-refactor-2-rebased branch July 7, 2017 16:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stdenv: Refactor meta checks #27045

stdenv: Refactor meta checks #27045

vcunat commented Jul 2, 2017

mention-bot commented Jul 2, 2017

zimbatm Jul 2, 2017

zimbatm Jul 2, 2017

vcunat Jul 2, 2017

copumpkin Jul 5, 2017

zimbatm Jul 2, 2017

zimbatm Jul 2, 2017

zimbatm Jul 2, 2017

zimbatm Jul 2, 2017

Ericson2314 Jul 2, 2017

vcunat Jul 2, 2017

Ericson2314 Jul 2, 2017

vcunat commented Jul 2, 2017

zimbatm commented Jul 2, 2017

0xABAB Jul 2, 2017

copumpkin Jul 5, 2017

0xABAB Jul 2, 2017

Ericson2314 commented Jul 2, 2017

0xABAB Jul 2, 2017

vcunat commented Jul 3, 2017

Ericson2314 Jul 5, 2017

vcunat Jul 5, 2017

Ericson2314 Jul 5, 2017 •

edited

Loading

Ericson2314 commented Jul 5, 2017 •

edited

Loading

Ericson2314 commented Jul 7, 2017 •

edited

Loading

vcunat commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017 •

edited

Loading

vcunat commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017

edolstra commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017

danbst commented Feb 12, 2018

vcunat commented Feb 12, 2018 •

edited

Loading


		showLicense = license: license.shortName or "unknown";

		pos_str = meta.position or "«unknown-file»";

stdenv: Refactor meta checks #27045

stdenv: Refactor meta checks #27045

Conversation

vcunat commented Jul 2, 2017

mention-bot commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vcunat commented Jul 2, 2017

zimbatm commented Jul 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 commented Jul 2, 2017

Choose a reason for hiding this comment

vcunat commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ericson2314 Jul 5, 2017 • edited Loading

Choose a reason for hiding this comment

Ericson2314 commented Jul 5, 2017 • edited Loading

Ericson2314 commented Jul 7, 2017 • edited Loading

vcunat commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017 • edited Loading

vcunat commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017

edolstra commented Jul 7, 2017

Ericson2314 commented Jul 7, 2017

danbst commented Feb 12, 2018

vcunat commented Feb 12, 2018 • edited Loading

Ericson2314 Jul 5, 2017 •

edited

Loading

Ericson2314 commented Jul 5, 2017 •

edited

Loading

Ericson2314 commented Jul 7, 2017 •

edited

Loading

Ericson2314 commented Jul 7, 2017 •

edited

Loading

vcunat commented Feb 12, 2018 •

edited

Loading