stdenv: Refactor meta checks by vcunat · Pull Request #27045 · NixOS/nixpkgs

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

typo: derivationArg

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

lib/check-meta.nix

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

typo: Allow

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

lib/check-meta.nix

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

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

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

pkgs/stdenv/generic/default.nix

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

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

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

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

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

Uh oh!

Conversation

vcunat commented Jul 2, 2017

Uh oh!

mention-bot commented Jul 2, 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!

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!

Choose a reason for hiding this comment

Uh oh!

vcunat commented Jul 2, 2017

Uh oh!

zimbatm commented Jul 2, 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!

Ericson2314 commented Jul 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vcunat commented Jul 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

vcunat commented Jul 7, 2017

Uh oh!

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

Uh oh!

Uh oh!

vcunat commented Jul 7, 2017

Uh oh!

Ericson2314 commented Jul 7, 2017

Uh oh!

edolstra commented Jul 7, 2017

Uh oh!

Ericson2314 commented Jul 7, 2017

Uh oh!

danbst commented Feb 12, 2018

Uh oh!

vcunat commented Feb 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

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